[haiku-development] Re: third mouse mode & mouse preflet

Hello,

Axel Dörfler wrote:
First of all, there are a number of (mostly coding style) issues with your patch - I only mention each problem exemplarily:
As you might've guessed, I haven't really taken the time to apply the coding style guidelines, as this is still a work in progress (which is not really an excuse). I do appreciate the effort you made to point out the errors however, and I will try to fix them all. 
+++ 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).
I will rename B_FOCUS_MOUSE to B_CLICK_TO_FOCUS_MOUSE. B_FOCUS_FOLLOWS_MOUSE is indeed better. Do I change B_ACTIVATE_MOUSE to B_CLICK_TO_ACTIVATE_MOUSE or back to B_NORMAL_MOUSE? Should I also keep the assigned enum values? 
+       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). 
Let's make it AS_SET_WINDOWS_ACCEPT_FIRST_CLICK then.
BTW you seem to have forgotten those syslog.h inclusions almost everywhere. 
I will remove these before submitting the final patch (unless I forget :).

+++ 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. 
Still? I would think that would have changed by now :)
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.
I do agree that the way it is implemented now is not very clean. I did it like this because it would ensure that the behavior of the existing mouse modes would not change. In this case the definition of "Activate" indeed needs to be changed to: "give a window focus, and if in the click-to-activate or focus-follows-mouse modes, also bring it to front" (case A).
It might be cleaner to call SetFocusWindow everywhere when in click-to-focus mode, but this would break backwards-compatibility quite badly. Code would have to be made aware of the extra mouse mode in *a lot* of places (Tracker, for one) (case B). This, I don't think would be a good idea. Applications should not care about the mouse mode, even Tracker. The Deskbar is the one exception I would make.
The thing is, the new mouse mode is fundamentally different from the existing ones. Because of the existing legacy code, it cannot easily be inserted in Haiku without changing the definition of Activate. I have discussed this with Stephan and he came up with a "golden rule" to detect when a window really needs to be brought to front when in the click-to-focus mode. It does feel a bit "icky" and I'm not convinced it will always lead to the wanted effect. Ideally, new applications should differentiate between focus and activate. This is starting to sound a lot like case B above, however. I don't know if such an API change is justifiable for yet another mouse mode...
After all that critic, I must say I like the patch in general, and the functionality it adds :-) 
Thanks. I thought is was never going to end :)

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.
Great. Now I only need to know where in the code the frontmost window is activated. I assume that's also in the Desktop class? I'll have a look at that later. 

Cheers,
Brecht

Other related posts: