GNOME Bugzilla – Bug 641810
MessageTray: factor out focus grabbing from Notification into a separate class
Last modified: 2011-02-10 16:59:43 UTC
That way it can be used when other components of the message tray need to grab focus, such as the summary bubble with multiple notifications or the summary item's right click menu.
Created attachment 180369 [details] [review] MessageTray: factor out focus grabbing from Notification into a separate class
Created attachment 180370 [details] [review] MessageTray: factor out focus grabbing from Notification into a separate class A small fix-up to remove an unused argument from the FocusGrabber constructor.
Comment on attachment 180370 [details] [review] MessageTray: factor out focus grabbing from Notification into a separate class Do we really need FocusGrabber? Can't it just be moved into MessageTray? >+ Main.messageTray.focusGrabber.connect('focus-grabbed', Lang.bind(this, >+ function (focusGrabber, actor) { >+ if (actor != this.actor) >+ return; >+ >+ if (this._buttonBox) >+ this._buttonBox.get_children()[0].grab_key_focus(); >+ })); So, if grabFocus() did actor.navigate_focus(null, Gtk.DirectionType.TAB_FORWARD, false); at the end, then that would automatically focus the first widget inside actor that has the can_focus property set, and then I think you don't need the focus-grabbed signal connection either here or in telepathyClient. (Which probably means you don't need the signal either.) (Well, this also requires that the actor passed to grabFocus() is an StWidget.)
Created attachment 180439 [details] [review] MessageTray: factor out focus grabbing from Notification into a separate class Calling navigate_focus() as you suggested seems to work. MessageTray already has many member variables and functions, so I think it's good to separate out a self-containing set of the member variables and functions related to focus grabbing in a separate class.
Comment on attachment 180439 [details] [review] MessageTray: factor out focus grabbing from Notification into a separate class > MessageTray already has many member variables and functions, so I think it's > good to separate out a self-containing set of the member variables and > functions related to focus grabbing in a separate class. True. OK, but it should be entirely an internal detail of MessageTray, not visible to the rest of the shell. (Ie, you should underscore-prefix this.focusGrabber). It's also inelegant to have FocusGrabber using Main.messageTray to call back into its "parent" object. It would be cleaner to have FocusGrabber emit 'lock', 'unlock', and 'escape' signals, and have MessageTray catch and act on those signals.
Created attachment 180514 [details] [review] MessageTray: factor out focus grabbing from Notification into a separate class Made this._focusGrabber a private variable in MessageTray and switched to using signals to do locking, unlocking, and escaping the tray. Made _lock(), _unlock(), and _escapeTray() private functions of the message tray since they are no longer used from outside of it. Realized that we should not be unsetting this.actor in FocusGrabber when we are ungrabbing focus while toggling focus grab mode, so added this._togglingFocusGrabMode to identify this case.
Created attachment 180521 [details] [review] MessageTray: factor out focus grabbing from Notification into a separate class Removed some stray parentheses.
Created attachment 180525 [details] [review] MessageTray: factor out focus grabbing from Notification into a separate class Another small fix - call Lang.bind() for signal callbacks.
Created attachment 180549 [details] [review] MessageTray: factor out focus grabbing from Notification into a separate class Another small fix - remove stray curly braces.
Created attachment 180550 [details] [review] MessageTray: factor out focus grabbing from Notification into a separate class Actually upload full patch...