GNOME Bugzilla – Bug 771098
Show OSD on tablet mode switches
Last modified: 2017-02-10 23:07:28 UTC
The Wacom Cintiq 22HD doesn't have LEDs to indicate the current mode and this may be true of future devices (we don't have official confirmation). For those devices (possibly all device?) gsd should show an OSD whenever the mode switches. Possible mockup by jimmac: https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/system-settings/tablets/mode-switching-osd.png
Moving this to mutter/gnome-shell, since most of the tablet magic is there now.
Created attachment 342523 [details] [review] clutter: Add current group mode on pad events This info can be obtained here right away, so we don't need backend-dependent paths in the upper layers.
Created attachment 342524 [details] [review] clutter: Add functions to find out mode switch buttons, and their group This is done in the upper layers through backend-dependent ways, seems better to let ClutterInputDevice provide this information.
Created attachment 342525 [details] [review] clutter: Add function to find out the number of modes for a pad group This is obtained in backend-dependent ways in the upper layers, seems better to let ClutterInputDevice provide this information.
Created attachment 342526 [details] [review] backends: Have meta_input_settings_handle_pad_button() take an event As all the relevant backends are expected to provide ClutterPadButtonEvents, it makes no sense to split the information, plus all other event fields are now available and might be needed in the future.
Created attachment 342527 [details] [review] wayland: Clean up MetaWaylandTabletPadGroup Using the clutter counterparts, some backend-specific code can be removed from here.
Created attachment 342528 [details] [review] core: Add MetaDisplay::show-osd signal And add specific private methods to notify about tablet mapping and mode switches. The signal allows the mutter side to trigger OSDs in a generic way.
Created attachment 342529 [details] [review] backends: Add action label to mode switch buttons
Created attachment 342530 [details] [review] backends: Notify tablet mapping changes in the UI This takes over the older code in g-s-d.
Created attachment 342531 [details] [review] wayland: Notify tablet mode switches This will show a fancy OSD so the change is immediately visible.
Created attachment 342532 [details] [review] windowManager: Handle MetaDisplay::show-osd signal Propagate as-is to the OsdWindowManager.
Review of attachment 342523 [details] [review]: straightforward patch, looks good to me.
Review of attachment 342524 [details] [review]: also straightforward, looks good to me.
Review of attachment 342525 [details] [review]: lgtm
Review of attachment 342526 [details] [review]: yep
Review of attachment 342527 [details] [review]: nod
Review of attachment 342528 [details] [review]: using more context than I'm familiar with but looks good
Review of attachment 342529 [details] [review]: not sure that comment is sufficient for translators, but otherwise LGTM
Review of attachment 342530 [details] [review]: ack
Review of attachment 342531 [details] [review]: yay!
Review of attachment 342532 [details] [review]: sorry, don't know enough about the context of this one
Review of attachment 342532 [details] [review]: ::: js/ui/windowManager.js @@ +923,3 @@ + global.display.connect('show-osd', Lang.bind(this, function (display, message, iconName, monitorIndex) { + let icon = Gio.Icon.new_for_string(iconName); + Main.osdWindowManager.show(monitorIndex, icon, message, null); It's a bit odd to have signal and method take the same parameters, but in reverse order. I'd also go with "label" instead of "message" to make it clear that the string is supposed to be short.
(In reply to Florian Müllner from comment #22) > Review of attachment 342532 [details] [review] [review]: > > ::: js/ui/windowManager.js > @@ +923,3 @@ > + global.display.connect('show-osd', Lang.bind(this, function > (display, message, iconName, monitorIndex) { > + let icon = Gio.Icon.new_for_string(iconName); > + Main.osdWindowManager.show(monitorIndex, icon, message, null); > > It's a bit odd to have signal and method take the same parameters, but in > reverse order. I'd also go with "label" instead of "message" to make it > clear that the string is supposed to be short. Indeed, I made the argument order match on the mutter side and renamed the variable.
Attachment 342523 [details] pushed as 8bcdf9b - clutter: Add current group mode on pad events Attachment 342524 [details] pushed as 9d38ffa - clutter: Add functions to find out mode switch buttons, and their group Attachment 342525 [details] pushed as d6fc41b - clutter: Add function to find out the number of modes for a pad group Attachment 342526 [details] pushed as fff7da2 - backends: Have meta_input_settings_handle_pad_button() take an event Attachment 342527 [details] pushed as 5af848d - wayland: Clean up MetaWaylandTabletPadGroup Attachment 342528 [details] pushed as 07ce981 - core: Add MetaDisplay::show-osd signal Attachment 342529 [details] pushed as efd9d46 - backends: Add action label to mode switch buttons Attachment 342530 [details] pushed as ff453c1 - backends: Notify tablet mapping changes in the UI Attachment 342531 [details] pushed as 7e3fbfb - wayland: Notify tablet mode switches
Attachment 342532 [details] pushed as c3cdbd0 - windowManager: Handle MetaDisplay::show-osd signal