GNOME Bugzilla – Bug 771067
Implement Pad OSD in gnome-shell
Last modified: 2016-11-04 15:17:33 UTC
g-s-d implementation of the pad OSD can't work on wayland (GDK_WINDOW_POPUP window with device grabs). This needs moving so gnome-shell can implement the OSD on wayland, although should/could also be used on X11.
Created attachment 335130 [details] [review] ui: Add PadOsd This is an implementation of the pad OSD that's been previously present in gnome-settings-daemon. Since things are moving closer to the compositor, it makes sense to have this implemented as shell UI.
Review of attachment 335130 [details] [review]: Some bits here are a bit evil, but unfortunately look necessary :-( So overall this looks OK ... ::: data/Makefile.am @@ +117,3 @@ $(keys_DATA) \ $(dist_theme_files) \ + pad-osd.css \ Do we need to sass-ify that? (for HighContrast/classic) ::: js/ui/padOsd.js @@ +37,3 @@ + width: 120 }); + this.actor.connect('captured-event', Lang.bind(this, this._onCapturedEvent)); + this.actor.connect('destroy', Lang.bind(this, this.destroy)); Recursion! @@ +41,3 @@ + + _onCapturedEvent: function (actor, event) { + if (event.type() == Clutter.EventType.KEY_PRESS) { Nit: Using an early return to cut down on indentation looks cleaner: if (event.type() != Clutter.EventType.KEY_PRESS) return Clutter.EVENT_PROPAGATE; @@ +48,3 @@ + event.get_state()); + this.actor.set_text(str); + this.emit('keybinding', str); 'keybinding' and 'action' (in ActionComboBox below) don't sound very signal-ish - 'keybinding-entered'/'action-selected' maybe? @@ +75,3 @@ + this.actor.set_child(box); + + this._label = new St.Label({ width: 150 }); Probably better with a 'combo-box-label' style class and an 'em' width (so that it takes text-scaling etc. into account) @@ +86,3 @@ + + /* Order matches GDesktopPadButtonAction enum */ + this._actions = [_('Application defined'), This would be cleaner and less error-prone as either a _getActionLabel(action) method: for (let action in GDesktopEnums.PadButtonAction) this._editMenu.addAction(this._getActionLabel(action), () => { this._onActionSelected(action); }); or as a map: this._actionLabels = new Map(); this._actionLabels.set(GDesktopEnums.PadButtonAction.NONE, _("Application defined")); this._actionLabels.set(...) for (let [action, label] of this._actionLabels.entries()) this._editMenu.addAction(label, () => { this._onActionSelected(action); }); @@ +89,3 @@ + _('Show on-screen help'), + _('Switch monitor'), + _('Assign keystroke')]; Also coding style nit: Double quotes for translatable strings @@ +151,3 @@ + + this._doneButton = new St.Button ({ label: _('Done'), + width: 100, Mmmh, why a fixed width instead of left/right padding via CSS? @@ +396,3 @@ + activateButton: function (button) { + this._activeButtons.push(button); + this._invalidateSvg (); Wouldn't this look nicer if the transition was animated? (/me hides) @@ +421,3 @@ + this._deviceRemovedId = deviceManager.connect('device-removed', Lang.bind(this, this._onDeviceRemoved)); + + this.actor = new Shell.GenericContainer({ style_class: 'pad-osd-window', Mmmh, Shell.GenericContainer was a work-around when we couldn't inherit from GObject to implement custom actors, so it's a bit sad to see it in new code. I wonder though if it's actually needed - it looks like the actor could be a vertical BoxLayout: [StBoxLayout] |__titleLabel | |__tipLabel | |__[StWidget] | |__ padDiagram | | | |__ buttonLabel | | | |__ ... | | | |__ actionEditor | |__ editButton StWidget is a group, so you should be able to position the children by setting a fixed position (which would be relative to the parent, whose size is determined by the padDiagram - so basically _allocateChild() without the box parameter) @@ +426,3 @@ + y: 0, + width: global.screen_width, + height: global.screen_height }); Why are you setting position and size, right before constraining the actor to the monitor? @@ +428,3 @@ + height: global.screen_height }); + this.actor.connect('allocate', Lang.bind(this, this._allocate)); + this.actor.connect('destroy', Lang.bind(this, this.destroy)); This would be cleaner as: this.actor.connect('destroy', Lang.bind(this, this._onDestroy)); ... destroy: function() { this.actor.destroy(); }, _onDestroy: function() { // everything else that's currently in destroy() } @@ +457,3 @@ + y_align: Clutter.ActorAlign.CENTER }); + this._titleLabel = new St.Label(); + this._titleLabel.clutter_text.set_markup('<span size="larger"><b>' + padDevice.get_device_name() + '</b></span>'); Or this._titleLabel = new St.Label({ style: 'font-size: larger; font-weight: bold; }); // better via style-class/CSS ::: js/ui/windowManager.js @@ +952,3 @@ + if (this._currentPadOsd != null) { + if (this._currentPadOsd.padDevice == device) + this._currentPadOsd.destroy(); If I understand this (and the signal-emitting code in mutter) correctly, when we get a request to show the OSD while we are already doing so, we close the current one if it's for the same device, but ignore the request otherwise? I'm confused, so let's say this could use a comment :-)
(In reply to Florian Müllner from comment #2) > Review of attachment 335130 [details] [review] [review]: > > Some bits here are a bit evil, but unfortunately look necessary :-( Tell me about it :) > > So overall this looks OK ... > > ::: data/Makefile.am > @@ +117,3 @@ > $(keys_DATA) \ > $(dist_theme_files) \ > + pad-osd.css \ > > Do we need to sass-ify that? > (for HighContrast/classic) Would be nice, but I guess not many people needing HC will use this feature though. fwiw the theming is basically an adaption of the hardcoded UI in g-s-d, so we're at least not regressing... This is the only undone point, I consider this lower urgency. > > ::: js/ui/padOsd.js > @@ +37,3 @@ > + width: 120 }); > + this.actor.connect('captured-event', Lang.bind(this, > this._onCapturedEvent)); > + this.actor.connect('destroy', Lang.bind(this, this.destroy)); > > Recursion! Oops, and undeeded. Removed :) > > @@ +41,3 @@ > + > + _onCapturedEvent: function (actor, event) { > + if (event.type() == Clutter.EventType.KEY_PRESS) { > > Nit: > Using an early return to cut down on indentation looks cleaner: > > if (event.type() != Clutter.EventType.KEY_PRESS) > return Clutter.EVENT_PROPAGATE; Done. > > @@ +48,3 @@ > + > event.get_state()); > + this.actor.set_text(str); > + this.emit('keybinding', str); > > 'keybinding' and 'action' (in ActionComboBox below) don't sound very > signal-ish - 'keybinding-entered'/'action-selected' maybe? Right, I've gone for keybinding-edited/action-selected. > > @@ +75,3 @@ > + this.actor.set_child(box); > + > + this._label = new St.Label({ width: 150 }); > > Probably better with a 'combo-box-label' style class and an 'em' width (so > that it takes text-scaling etc. into account) Indeed. > > @@ +86,3 @@ > + > + /* Order matches GDesktopPadButtonAction enum */ > + this._actions = [_('Application defined'), > > This would be cleaner and less error-prone as either a > _getActionLabel(action) method: > > for (let action in GDesktopEnums.PadButtonAction) > this._editMenu.addAction(this._getActionLabel(action), > () => { this._onActionSelected(action); }); > > or as a map: > > this._actionLabels = new Map(); > this._actionLabels.set(GDesktopEnums.PadButtonAction.NONE, > _("Application defined")); > this._actionLabels.set(...) > > for (let [action, label] of this._actionLabels.entries()) > this._editMenu.addAction(label, () => { > this._onActionSelected(action); }); I've gone for this last option :). > > @@ +89,3 @@ > + _('Show on-screen help'), > + _('Switch monitor'), > + _('Assign keystroke')]; > > Also coding style nit: > Double quotes for translatable strings Oh sure. Did that throughout the patch. > > @@ +151,3 @@ > + > + this._doneButton = new St.Button ({ label: _('Done'), > + width: 100, > > Mmmh, why a fixed width instead of left/right padding via CSS? Indeed, now use CSS here and in a few other places. > > @@ +396,3 @@ > + activateButton: function (button) { > + this._activeButtons.push(button); > + this._invalidateSvg (); > > Wouldn't this look nicer if the transition was animated? (/me hides) Ahem :P, let's hope nobody files it as a regression... > > @@ +421,3 @@ > + this._deviceRemovedId = deviceManager.connect('device-removed', > Lang.bind(this, this._onDeviceRemoved)); > + > + this.actor = new Shell.GenericContainer({ style_class: > 'pad-osd-window', > > Mmmh, Shell.GenericContainer was a work-around when we couldn't inherit from > GObject to implement custom actors, so it's a bit sad to see it in new code. > > I wonder though if it's actually needed - it looks like the actor could be a > vertical BoxLayout: > > [StBoxLayout] > |__titleLabel > | > |__tipLabel > | > |__[StWidget] > | |__ padDiagram > | | > | |__ buttonLabel > | | > | |__ ... > | | > | |__ actionEditor > | > |__ editButton > > StWidget is a group, so you should be able to position the children by > setting a fixed position (which would be relative to the parent, whose size > is determined by the padDiagram - so basically _allocateChild() without the > box parameter) Right, AFAICS I still need to do some dynamic allocation though, since the PadDiagram content size is determined after the widget size is first allocated, so I can't figure out the label locations earlier than that. Besides the dynamic allocation detail, I've changed the actor hierarchy to be similar to how you describe, with the only difference that the padDiagram is now the parent container for all labels and the actionEditor. > > @@ +426,3 @@ > + y: 0, > + width: > global.screen_width, > + height: > global.screen_height }); > > Why are you setting position and size, right before constraining the actor > to the monitor? Right, this is unneeded. I removed it :). > > @@ +428,3 @@ > + height: > global.screen_height }); > + this.actor.connect('allocate', Lang.bind(this, this._allocate)); > + this.actor.connect('destroy', Lang.bind(this, this.destroy)); > > This would be cleaner as: > > this.actor.connect('destroy', Lang.bind(this, this._onDestroy)); > > ... > > destroy: function() { > this.actor.destroy(); > }, > > _onDestroy: function() { > // everything else that's currently in destroy() > } Done. > > @@ +457,3 @@ > + y_align: Clutter.ActorAlign.CENTER > }); > + this._titleLabel = new St.Label(); > + this._titleLabel.clutter_text.set_markup('<span size="larger"><b>' > + padDevice.get_device_name() + '</b></span>'); > > Or > this._titleLabel = new St.Label({ style: 'font-size: larger; font-weight: > bold; }); // better via style-class/CSS Indeed. > > ::: js/ui/windowManager.js > @@ +952,3 @@ > + if (this._currentPadOsd != null) { > + if (this._currentPadOsd.padDevice == device) > + this._currentPadOsd.destroy(); > > If I understand this (and the signal-emitting code in mutter) correctly, > when we get a request to show the OSD while we are already doing so, we > close the current one if it's for the same device, but ignore the request > otherwise? > > I'm confused, so let's say this could use a comment :-) Oh, just removed that entire condition (and attaching a related mutter patch). It was the initial code preventing multiple pad OSDs to be opened. I think it's preferable to let mutter handle that, since it holds a reference to the pad OSD actor.
Created attachment 338206 [details] [review] ui: Add PadOsd This is an implementation of the pad OSD that's been previously present in gnome-settings-daemon. Since things are moving closer to the compositor, it makes sense to have this implemented as shell UI.
Created attachment 338207 [details] [review] theme: Add CSS for the pad OSD
Created attachment 338208 [details] [review] core: Ensure there is an unique pad OSD actor We kind of rely on the ::show-pad-osd handler to destroy the previous actor. Just prevent the emission of multiple signals till the actor has been destroyed.
Created attachment 338209 [details] [review] padOsd: Add theming for the pad OSD.
Created attachment 338418 [details] [review] ui: Add PadOsd This is an implementation of the pad OSD that's been previously present in gnome-settings-daemon. Since things are moving closer to the compositor, it makes sense to have this implemented as shell UI.
Review of attachment 338208 [details] [review]: One question, and nit in the commit message: "a unique" instead of "an unique" ::: src/core/display.c @@ +3113,2 @@ if (display->current_pad_osd) + return; Do we need to check whether the request is for the same device as the previous one?
Review of attachment 338209 [details] [review]: ::: _common.scss @@ +625,3 @@ +.pad-osd-window { + padding: 32px; + background-color: transparentize(black, 0.8); Might be possible to reuse some existing color constant like $osd_bg_color here, but that's really up to the designers ...
Review of attachment 338418 [details] [review]: (In reply to Carlos Garnacho from comment #3) > > Do we need to sass-ify that? > > (for HighContrast/classic) > > Would be nice, but I guess not many people needing HC will use this feature > though. Yeah. Still leaves classic, but users of that should probably stick to input devices from 1995 anyway :-p > [...] I consider this lower urgency. Agreed. ::: js/ui/padOsd.js @@ +69,3 @@ + + this._label = new St.Label({ style_class: 'combo-box-label', + style: 'width: 15em' }); Sorry, I meant: add a style class here, then have .combo-box-label { width: 15em; } in the (s)css. @@ +143,3 @@ + + this._doneButton = new St.Button({ label: _("Done"), + style_class: 'button', Oh, I didn't think of the existing .button class - awesome! @@ +546,3 @@ + y_expand: true, + vertical: true, + reactive: true }); Mmh, does the actor need to be reactive? @@ +591,3 @@ + } + + this._buttonBox = new St.Widget({ layout_manager: new Clutter.BinLayout(), Nit: buttonBox isn't needed later, so can be a local variable @@ +716,3 @@ + let actor = this.actor; + this.actor = null; + actor.destroy(); This looks fishy - we get here if this.actor has been destroyed, and now we are destroying it again. While we guard this against infinite recursion, it looks like we'll end up trying to pop the same modal twice. Doesn't a simple: this.actor = null; this.emit('closed'); work (that is, without checking for this.actor at all)? If not, I think if (!this.actor) return; at the beginning of the function would be the better option ...
Review of attachment 338207 [details] [review]: Please squash with the main patch.
(In reply to Florian Müllner from comment #9) > Review of attachment 338208 [details] [review] [review]: > > One question, and nit in the commit message: "a unique" instead of "an > unique" > > ::: src/core/display.c > @@ +3113,2 @@ > if (display->current_pad_osd) > + return; > > Do we need to check whether the request is for the same device as the > previous one? There's IMO no hard need. By having a single "current" OSD, you have to actively dismiss it before raising another one, be it for the same pad device or for another. This also means you don't get multiple ::show-pad-osd calls for the same pad device, as this pad events are grabbed by the OSD while it's shown. This deserves a comment anyway, I've added one locally. (In reply to Florian Müllner from comment #11) > Review of attachment 338418 [details] [review] [review]: > > (In reply to Carlos Garnacho from comment #3) > > > Do we need to sass-ify that? > > > (for HighContrast/classic) > > > > Would be nice, but I guess not many people needing HC will use this feature > > though. > > Yeah. Still leaves classic, but users of that should probably stick to input > devices from 1995 anyway :-p And it's been these colors since "classic" was "future" anyway :). > > > > [...] I consider this lower urgency. > > Agreed. > > ::: js/ui/padOsd.js > @@ +69,3 @@ > + > + this._label = new St.Label({ style_class: 'combo-box-label', > + style: 'width: 15em' }); > > Sorry, I meant: add a style class here, then have > > .combo-box-label { width: 15em; } Oh sure. > > in the (s)css. > > @@ +143,3 @@ > + > + this._doneButton = new St.Button({ label: _("Done"), > + style_class: 'button', > > Oh, I didn't think of the existing .button class - awesome! > > @@ +546,3 @@ > + y_expand: true, > + vertical: true, > + reactive: true }); > > Mmh, does the actor need to be reactive? Turns out it does... otherwise I can click through on the the top bar actors while the OSD is "above". > > @@ +591,3 @@ > + } > + > + this._buttonBox = new St.Widget({ layout_manager: new > Clutter.BinLayout(), > > Nit: buttonBox isn't needed later, so can be a local variable > > @@ +716,3 @@ > + let actor = this.actor; > + this.actor = null; > + actor.destroy(); > > This looks fishy - we get here if this.actor has been destroyed, and now we > are destroying it again. While we guard this against infinite recursion, it > looks like we'll end up trying to pop the same modal twice. Doesn't a simple: > > this.actor = null; > this.emit('closed'); > > work (that is, without checking for this.actor at all)? If not, I think Right, this is a leftover of the previous iteration, that will work :).
Comment on attachment 338208 [details] [review] core: Ensure there is an unique pad OSD actor Attachment 338208 [details] pushed as 30fa764 - core: Ensure there is an unique pad OSD actor
Comment on attachment 338209 [details] [review] padOsd: Add theming for the pad OSD. Attachment 338209 [details] pushed as ec992ca - padOsd: Add theming for the pad OSD.
\o/. Merged CSS with the main patch as suggested and pushed to master (incl. fixes). Attachment 338418 [details] pushed as e006b9b - ui: Add PadOsd