GNOME Bugzilla – Bug 710373
Switch to monitor should only toggle between monitors
Last modified: 2013-11-20 19:35:31 UTC
The switch now includes the composite area of all displays now. After extensive use I can say it's almost always in the way and renders the quick switch shortcut less efficient. We should only toggle phyiscal displays with this. Maybe a short 500ms OSD indication of the target display would be helpful too. I'd use input-tablet-symbolic along with 'Wacom XY mapped here' label.
The first part would be modifying switch_monitor() in gsd-wacom-manager.c. The OSD would need to use gnome-shell's showOSD method (it's already used in the media-keys plugin at least).
Created attachment 259927 [details] [review] wacom: Show OSD when remapping device to a monitor through the pad bindings This helps having a clue of which monitor got mapped to the device, other than waving the pen to check where the pointer ends up. Now an small, short-timed OSD pops up on the monitor that got mapped to the device.
Created attachment 259928 [details] [review] wacom: Skip GSD_WACOM_SET_ALL_MONITORS on the switch monitor pad action Having an intermediate setting where the tablet is mapped to all monitors comes up as a bit unintuitive, specially on 2 monitor setups, where one click is required to switch to the other monitor, but then 2 clicks are required to switch back.
Created attachment 259930 [details] [review] Include an icon in the OSD too
Review of attachment 259927 [details] [review]: No. The OSD needs to use the showOSD() D-Bus API from gnome-shell, as mentioned in the bug earlier. That might need to be extended to allow showing the OSD on a particular monitor though.
Review of attachment 259928 [details] [review]: ::: plugins/wacom/gsd-wacom-manager.c @@ +1348,2 @@ /* Select next monitor */ + current_monitor = (current_monitor + 1) % n_monitors; I prefer the more readable original. - current_monitor = GSD_WACOM_SET_ALL_MONITORS; + current_monitor = 0;
Review of attachment 259930 [details] [review]: As per original review.
(In reply to comment #5) > Review of attachment 259927 [details] [review]: > > No. The OSD needs to use the showOSD() D-Bus API from gnome-shell, as mentioned > in the bug earlier. > > That might need to be extended to allow showing the OSD on a particular monitor > though. Yes, that's partly why I took this way, it struck me as rarely useful to have per monitor OSDs there. Besides, keeping the obscured parts close to a corner sounded like a plus to me in this context. I'm playing now with the gnome-shell OSD though
(In reply to comment #8) > (In reply to comment #5) > > Review of attachment 259927 [details] [review] [details]: > > > > No. The OSD needs to use the showOSD() D-Bus API from gnome-shell, as mentioned > > in the bug earlier. > > > > That might need to be extended to allow showing the OSD on a particular monitor > > though. > > Yes, that's partly why I took this way, it struck me as rarely useful to have > per monitor OSDs there. Besides, keeping the obscured parts close to a corner > sounded like a plus to me in this context. I'm playing now with the gnome-shell > OSD though The problem being that all the other OSDs use gnome-shell, which can make sure that the OSDs are themed similarly, and don't overlap each other (hides the previous one for example). We used to have OSDs client side, we don't anymore.
Created attachment 260230 [details] [review] common: Add GsdShellProxy This is an object that abstracts communication with gnome-shell, also provides methods for the currently used actions in g-s-d.
Created attachment 260231 [details] [review] media-keys: Use GsdShellProxy All communication with the shell now goes through this object, both for OSDs and requesting focus on the shell search entry.
Created attachment 260232 [details] [review] wacom: Show OSD when remapping device to a monitor through the pad bindings This helps having a clue of which monitor got mapped to the device, other than waving the pen to check where the pointer ends up. Now gnome-shell is requested to show an OSD on the monitor the device gets mapped to.
Created attachment 260233 [details] [review] wacom: Skip GSD_WACOM_SET_ALL_MONITORS on the switch monitor pad action Having an intermediate setting where the tablet is mapped to all monitors comes up as a bit unintuitive, specially on 2 monitor setups, where one click is required to switch to the other monitor, but then 2 clicks are required to switch back.
I've filed bug #712664 to gnome-shell, in order to add a "monitor" setting to ShowOSD, that patch would be needed too in order to have the OSD shown on the right monitor. One thing worth pointing out in this new patch for the wacom plugin is that only the device name is requested to be shown on the OSD, tablet names are usually already long enough to trigger ellipsizing, so any extra translatable text will be likely either not shown, or will push the device name out of the visible text. With pretty vanilla font sizes, I do see strings like "Wacom Bamboo 16..." or "Wacom Intuos 5S...", so there's enough provided context, unless someone has two too similar tablets I guess, but it's the user who's physically pressing the button anyway...
Review of attachment 260230 [details] [review]: gnome-settings-daemon/* already contains wrappers for the shell's screensaver proxy, and gnome-session. The wrapper should probably be added there instead. And gdbus-codegen :)
Review of attachment 260231 [details] [review]: That looks fine pending the wrapper changes.
Review of attachment 260232 [details] [review]: Looks fine, pending the proxy changes. ::: plugins/wacom/gsd-wacom-manager.c @@ +1368,3 @@ + + monitor_num = gsd_wacom_device_get_display_monitor (device); + screen = gdk_screen_get_default (); Assign screen after the monitor_num check, it might be unused.
Review of attachment 260233 [details] [review]: Looks good! s/specially/especially/
Comment on attachment 260233 [details] [review] wacom: Skip GSD_WACOM_SET_ALL_MONITORS on the switch monitor pad action Committed with the log typo change
Created attachment 260334 [details] [review] main: Generate org.gnome.Shell proxy This will be used across multiple plugins, so put it together with the screensaver and session proxies.
Created attachment 260335 [details] [review] common: Add helper function to GsdShell Currently only one helper function is provided to interface with ShowOSD, it takes care of creating the GVariant with all provided arguments.
Created attachment 260336 [details] [review] media-keys: Use GsdShell proxy All communication with the shell now goes through this object, both for OSDs and requesting focus on the shell search entry. One notable change is that ownership of the shell DBus name is now tracked through GsdShell::g-name-owner.
Created attachment 260337 [details] [review] wacom: Show OSD when remapping device to a monitor through the pad bindings This helps having a clue of which monitor got mapped to the device, other than waving the pen to check where the pointer ends up. Now gnome-shell is requested to show an OSD on the monitor the device gets mapped to.
Review of attachment 260334 [details] [review]: Looks good otherwise. ::: gnome-settings-daemon/gnome-settings-bus.c @@ +95,3 @@ +gnome_settings_bus_get_shell_proxy (void) +{ + static GsdShell *shell_proxy; Assign to NULL (even if the compiler already does that for us). ::: gnome-settings-daemon/org.gnome.Shell.xml @@ +29,3 @@ + --> + <interface name="org.gnome.Shell"> + <method name="Eval"> I would remove all the functions that we don't actually use. Might not be many...
Review of attachment 260335 [details] [review]: Looks good.
Review of attachment 260336 [details] [review]: Looks fine.
Review of attachment 260337 [details] [review]: Looks fine otherwise. ::: plugins/wacom/gsd-wacom-manager.c @@ +1372,3 @@ + gint monitor_num; + + if (manager->priv->shell_proxy == NULL) Check that after the conditional that exit this function?
I've applied those last suggestions and pushed to master