[haiku-development] Re: third mouse mode & mouse preflet
- From: "Axel Dörfler" <axeld@xxxxxxxxxxxxxxxx>
- To: haiku-development@xxxxxxxxxxxxx
- Date: Mon, 30 Mar 2009 12:00:00 +0200 CEST
Brecht Machiels <brecht@xxxxxxxxxxx> wrote:
> I also added a Belgian keymap with a point instead of a comma in the
> numeric keypad.
That really should not be part of the patch; you can select which
directories to include in the patch with svn, like:
svn diff src/servers src/preferences headers ... > mypatch
First of all, there are a number of (mostly coding style) issues with
your patch - I only mention each problem exemplarily:
> +++ headers/os/interface/InterfaceDefs.h (working copy)
> enum mode_mouse {
> - B_NORMAL_MOUSE = 0,
> - B_FOCUS_FOLLOWS_MOUSE = 1,
> - B_WARP_MOUSE = 3,
> - B_INSTANT_WARP_MOUSE = 7
> + B_ACTIVATE_MOUSE = 0,
> + B_FOCUS_MOUSE = 1,
> + B_AUTOFOCUS_MOUSE = 2,
> };
It's not a good idea to change the meaning of existing constants, as
this breaks compatibility.
Also, I think B_ACTIVATE_MOUSE, and B_FOCUS_MOUSE are both not very
descriptive; together with B_AUTOFOCUS_MOUSE they even mix different
concepts (click vs. move).
> + AS_SET_MOUSE_CLICKTHROUGH,
Since "click through" are two words, there should be an underscore
between CLICK and THROUGH.
I'm also not sure if it's a good idea to change existing vocabulary
(like "accepts first click" vs. "click through" - I find the former
clearer, too, or "focus follows mouse" vs. "autofocus", although I
think they are equally clear).
> +++ src/kits/interface/MenuBar.cpp (working copy)
> #include <TokenSpace.h>
> +#include <InterfaceDefs.h>
>
> #include "BMCPrivate.h"
>
> +#include <syslog.h>
>
> using BPrivate::gDefaultTokens;
Two spaces between groups, ie. between the syslog.h include and the
using.
The more generic the header the earlier it comes (besides the header
counterpart); ie. syslog.h is POSIX, and that is more generic than the
Be API, so it comes first.
Also, headers within the same group are sorted alphabetically
(InterfaceDefs.h).
BTW you seem to have forgotten those syslog.h inclusions almost
everywhere.
> + if (focus_follows_mouse() &&
> + (!window->IsActive() || !window->IsFront())) {
Operators usually go to the start of the new line:
+ if (focus_follows_mouse()
+ && (!window->IsActive() || !window->IsFront())) {
> +++ src/kits/interface/Region.cpp (working copy)
> }
>
> +/*! \brief Compares this region to another (by value).
> + \param other the BRegion to compare to.
> + \return \ctrue if the two regions are the same, \cfalse otherwise.
> +*/
> +bool
> +BRegion::operator==(const BRegion &other) const
> +{
> [...]
> +}
>
> /*! \brief Set the region to contain just the given BRect.
Two spaces between functions.
> +++ src/kits/interface/InterfaceDefs.cpp (working copy)
> +mouse_clickthrough()
> +{
> + // Gets the clickthrough status
> + bool clickthrough = false;
click through are still two words: mouse_click_through(), and
clickThrough.
> +++ src/servers/app/DefaultDecorator.cpp (working copy)
> - if (buttons == B_SECONDARY_MOUSE_BUTTON)
> + if (buttons & B_SECONDARY_MOUSE_BUTTON) {
> return DEC_MOVETOBACK;
> + }
No superfluous parenthesis. Prefer ((a & b) != 0).
+++ src/servers/app/Desktop.cpp (working copy)
> +Desktop::ActivateWindowToFront(Window* window)
> {
> STRACE(("ActivateWindow(%p, %s)\n", window, window ? window->
> Title() : "<none>"));
>
> @@ -1725,6 +1748,7 @@
> }
> }
>
> +
> _BringWindowsToFront(windows, kWorkingList, true);
No two blank lines within content.
> - SetFocusWindow(fSettings->FocusFollowsMouse() ?
> - WindowAt(fLastMousePosition) : NULL);
> + if (fSettings->MouseMode() != B_FOCUS_MOUSE)
> + SetFocusWindow(fSettings->FocusFollowsMouse() ?
> + WindowAt(fLastMousePosition) : NULL);
Use {} here to cover the multi-line term. The ? at the end is actually
also a coding style violation, but that wasn't you ;-)
I don't like the ActivateWindowToFront() name, I'm also not sure if the
new code always works correctly in case of subset windows. Also the
description of the new ActivateWindow() is not really fitting - it's a
wrapper to the ActivateWindowToFront() as it's a wrapper to
SetFocusWindow(); only what it does matters.
"Activate" is defined in the Be API as "bring a window to front and
give it focus", so the nomenclature in the app_server should mirror
that, and should not use a different one.
> +++ src/preferences/mouse/SettingsView.cpp (working copy)
> + const mode_autofocus autoFocusModes[] = {B_NORMAL_AUTOFOCUS,
> B_WARP_AUTOFOCUS,
> +
> B_INSTANT_WARP_AUTOFOCUS};
The indentation is all messed up: it's always only a single tab. Don't
try to match up with an opening bracket or anything like that.
> +++ src/preferences/mouse/Mouse.rdef (working copy)
> @@ -97,208 +97,245 @@
> #endif // HAIKU_TARGET_PLATFORM_HAIKU
>
> resource(601, "double_click_bmap") #'bits' array {
> - $"424D06070000000000003A000000280000000E0000001E000000010018000000"
Instead of changing bitmaps, we should switch to vector icons.
After all that critic, I must say I like the patch in general, and the
functionality it adds :-)
> There are still some things to do however. These come with an equal
> amount of questions :)
> 1) fix annoying behavior of click-to-focus mode in certain situations
> When a window is not frontmost and one of its menu-items is clicked,
> the
> front-most window is focused/activated. In Firefox (not a BMenu, I
> assume), clicking the menu-item does not even perform the action (go
> back for example). This is of course not the desired behavior for the
> click-to-focus mode. Haiku should ideally remember the order in which
> windows were focused so that the previously-focused window can be
> focused again. Unfortunately, I cannot seem to find where in the
> source
> code this can be changed. Help! The same is true when closing a
> window.
> I imagine this might even be the same code?
Actually this is something the Desktop class should already take care
of: there is a fFocusList member which does that exactly.
> 2) I would like to make the left half of the mouse preflet less wide.
> Is
> that possible using the layout kit?
Sure, you can use weights to divide the contents, or use explicit
sizes.
Using the preferred size from the view doesn't seem to be something the
layout engine is trying to take into account, for some reason.
> 3) In order to be able to hide autofocus options when another mouse
> mode
> is selected, I will need to merge some classes of the preflet (I
> forgot
> the details, it's been too long). I assume there's no objection to
> this? :)
Sounds a bit weird, but do whatever you think you need to do :-)
Bye,
Axel.
Other related posts:
