After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 771067 - Implement Pad OSD in gnome-shell
Implement Pad OSD in gnome-shell
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2016-09-08 16:42 UTC by Carlos Garnacho
Modified: 2016-11-04 15:17 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
ui: Add PadOsd (34.52 KB, patch)
2016-09-08 16:45 UTC, Carlos Garnacho
none Details | Review
ui: Add PadOsd (34.19 KB, patch)
2016-10-21 18:33 UTC, Carlos Garnacho
none Details | Review
theme: Add CSS for the pad OSD (741 bytes, patch)
2016-10-21 18:33 UTC, Carlos Garnacho
committed Details | Review
core: Ensure there is an unique pad OSD actor (1.09 KB, patch)
2016-10-21 18:34 UTC, Carlos Garnacho
committed Details | Review
padOsd: Add theming for the pad OSD. (632 bytes, patch)
2016-10-21 18:35 UTC, Carlos Garnacho
committed Details | Review
ui: Add PadOsd (34.83 KB, patch)
2016-10-25 14:30 UTC, Carlos Garnacho
committed Details | Review

Description Carlos Garnacho 2016-09-08 16:42:24 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.
Comment 1 Carlos Garnacho 2016-09-08 16:45:39 UTC
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.
Comment 2 Florian Müllner 2016-09-09 19:11:03 UTC
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 :-)
Comment 3 Carlos Garnacho 2016-10-21 18:32:30 UTC
(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.
Comment 4 Carlos Garnacho 2016-10-21 18:33:29 UTC
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.
Comment 5 Carlos Garnacho 2016-10-21 18:33:36 UTC
Created attachment 338207 [details] [review]
theme: Add CSS for the pad OSD
Comment 6 Carlos Garnacho 2016-10-21 18:34:03 UTC
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.
Comment 7 Carlos Garnacho 2016-10-21 18:35:01 UTC
Created attachment 338209 [details] [review]
padOsd: Add theming for the pad OSD.
Comment 8 Carlos Garnacho 2016-10-25 14:30:35 UTC
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.
Comment 9 Florian Müllner 2016-11-04 13:53:48 UTC
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?
Comment 10 Florian Müllner 2016-11-04 13:53:54 UTC
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 ...
Comment 11 Florian Müllner 2016-11-04 13:53:57 UTC
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 ...
Comment 12 Florian Müllner 2016-11-04 13:54:02 UTC
Review of attachment 338207 [details] [review]:

Please squash with the main patch.
Comment 13 Carlos Garnacho 2016-11-04 14:53:00 UTC
(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 14 Carlos Garnacho 2016-11-04 15:00:06 UTC
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 15 Carlos Garnacho 2016-11-04 15:13:57 UTC
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.
Comment 16 Carlos Garnacho 2016-11-04 15:17:23 UTC
\o/. Merged CSS with the main patch as suggested and pushed to master (incl. fixes).

Attachment 338418 [details] pushed as e006b9b - ui: Add PadOsd