GNOME Bugzilla – Bug 640583
St: drop StClickable, add some functionality to StButton
Last modified: 2011-02-07 17:47:04 UTC
StClickable/StButton didn't support keyboard-triggered right-clicking, so I started to add that, and decided it was stupid to add it to both classes. Initially I was going to make StButton be a subclass of StClickable, but there didn't seem to be any real point, so I just killed off StClickable. The two big behavioral differences between StClickable and StButton *that we were actually making use of* were that StClickable reacts to all mouse buttons as opposed to just the left button, and there's a special hack in dnd.js for StClickable to make the AppIcon menu-or-drag behavior work. The latter was easy to fix, the former was a bit messier, especially because of multiple meanings of the word "button". Anyway, there's an "all_buttons" property now, to tell StButton to react to all mouse buttons (so we didn't have to go in and make every existing user *not* react to middle and right clicks), but better names/ideas are welcome. Oh, and the patch also adds keyboard-triggered right clicks, via the Menu key or Shift-F10 (which matches Gtk).
Created attachment 179329 [details] [review] St: drop StClickable, add some functionality to StButton For historical reasons, we had both StClickable and StButton, which were nearly identical. StButton was more widely-used, so keep that and port all StClickable users to that.
Review of attachment 179329 [details] [review]: Looks mostly good to me. ::: js/ui/appDisplay.js @@ +320,3 @@ _init : function(app, iconParams) { this.app = app; + this.actor = new St.Button({ style_class: 'app-well-app', Hmmm, this matches the current behavior but using 'clicked' for button 3 here is a little problematical - menus are supposed to come up on press, not on release - so only button 2 is really the use case for all_buttons. ::: src/st/st-button.c @@ +547,3 @@ + * + * Checks if the button emits #StButton::clicked for all mouse + * buttons or just for button 1. Hmm, I think wanting ::clicked for anything but button 1 is pretty rare. But we do have one actual legitimate example - which is the app launchers triggering on button 2. I think I'd suggest going with: typedef enum { ST_BUTTON_ONE = 1 << 0, ST_BUTTON_TWO = 1 << 1, ST_BUTTON_THREE = 1 << 2 } StButtonMask; st_button_set_button_mask (StButtonMask mask); to better encapsulate the idea of wanting to get ::clicked on a different button in a few cases. It also has a slightly less dreadful name - st_button_get_all_buttons() should return all StButtons, right?
Oh, and thanks for fixing this! The split has been annoying since we pulled in ST and added StClickable.
Oh, and obviously since I don't think clicked(button=3) is the right way to activate a menu from the mouse, I'm dubious about having Alt-F10 fire that. What if we added a separate ::show-menu (or whatever) signal?
(In reply to comment #2) > Hmmm, this matches the current behavior but using 'clicked' for button 3 here > is a little problematical Yeah, I wasn't sure if that was intentional here; we're already connecting to button-press-event there anyway, so it would have been fairly easy to have done this right...
Created attachment 179885 [details] [review] AppIcon: right-click menu should appear on press, not click
Created attachment 179886 [details] [review] St: drop StClickable, add some functionality to StButton For historical reasons, we had both StClickable and StButton, which were nearly identical. StButton was more widely-used, so keep that and port all StClickable users to that.
(In reply to comment #4) > Oh, and obviously since I don't think clicked(button=3) is the right way to > activate a menu from the mouse, I'm dubious about having Alt-F10 fire that. > What if we added a separate ::show-menu (or whatever) signal? I have added this as well, but I ended up adding it on StWidget (since we presumably want it to work from StEntry and maybe other things as well), so it doesn't really go with these patches.
Review of attachment 179885 [details] [review]: Looks fine and definitely feels more natural.
Review of attachment 179886 [details] [review]: A bug I note here is that key press and key releases aren't necessarily paired - so control-alt-tab to the activities button, press and hold space, and then arrow key off, and the activities button will stick in the down state. So that probably means cancelling the key-down state on focus out and tracking it separately from button-one-down. Otherwise, looks good to me.
Created attachment 180304 [details] [review] St: drop StClickable, add some functionality to StButton the only change is the addition of st_button_key_focus_out()
Review of attachment 180304 [details] [review]: Look good
Attachment 179885 [details] pushed as c256fa9 - AppIcon: right-click menu should appear on press, not click Attachment 180304 [details] pushed as c86a977 - St: drop StClickable, add some functionality to StButton