GNOME Bugzilla – Bug 712664
Allow specifying monitor for the OSD to be shown
Last modified: 2013-11-20 17:05:25 UTC
As discussed on g-s-d bug #710373 , there is a potential interest to show the OSD on an specific monitor. The usecase on that bug is tablets, so the user can know which monitor a tablet is mapped to, although it might also be useful for other per-monitor settings, like hdmi sound and whatnot. I'm attaching a patch to add such setting
Created attachment 260229 [details] [review] osdWindow: add setMonitor() to allow changing the monitor This is also exposed in the ShowOSD DBus method, the "monitor" parameter may contain an integer to indicate the monitor number. If the value is not provided or <0 is used, the monitor is shown on the primary monitor as usually. This way, the OSD can be used to notify upon events that solely apply to one monitor, like tablet mapping as discussed in https://bugzilla.gnome.org/show_bug.cgi?id=710373.
Review of attachment 260229 [details] [review]: Looks generally good, but there are still places left where the monitor is hardcoded. ::: js/ui/osdWindow.js @@ +209,3 @@ + }, + + setMonitor: function(index) { You should probably replace the constraint code in the constructor; also, in _monitorsChanged we still assume that the OSD is placed on the primary monitor, so we should keep track of the actual monitor used (this will also allow to only update the constraint when the requested monitor actually changes) ::: js/ui/shellDBus.js @@ +134,3 @@ params[param] = params[param].deep_unpack(); + let monitor_index = -1; Style nit: camelCase in JS code
Created attachment 260255 [details] [review] osdWindow: add setMonitor() to allow changing the monitor This is also exposed in the ShowOSD DBus method, the "monitor" parameter may contain an integer to indicate the monitor number. If the value is not provided or <0 is used, the monitor is shown on the primary monitor as usually. This way, the OSD can be used to notify upon events that solely apply to one monitor, like tablet mapping as discussed in https://bugzilla.gnome.org/show_bug.cgi?id=710373.
Thanks for the review! This last patch should contain all suggestions, comments below (In reply to comment #2) > Review of attachment 260229 [details] [review]: > > so we should keep track of the actual monitor used (this will also allow to > only update the constraint when the requested monitor actually changes) I kept the relocating/resizing code called invariably there, because that function could still be called when the monitor resolution changes AFAICS, so the popup size might still need changing if shown, although the chances are slim :) > > ::: js/ui/shellDBus.js > @@ +134,3 @@ > params[param] = params[param].deep_unpack(); > > + let monitor_index = -1; > > Style nit: camelCase in JS code Doh! my bad, fixed too
Review of attachment 260255 [details] [review]: One comment left, otherwise looks good to me. ::: js/ui/osdWindow.js @@ +216,3 @@ + + setMonitor: function(index) { + let constraint; When I suggested to short-circuit the case of setting the same monitor twice, I was only referring to this function, not _monitorsChanged() (where this would indeed be wrong). Something along the lines of: if (this._currentMonitor == index) return; (or something more fancy like (this._currentMonitor < 0 && index < 0 || this._currentMonitor == index))
(In reply to comment #5) > When I suggested to short-circuit the case of setting the same monitor twice, I > was only referring to this function, not _monitorsChanged() (where this would > indeed be wrong). Something along the lines of: > > if (this._currentMonitor == index) > return; > > (or something more fancy like (this._currentMonitor < 0 && index < 0 || > this._currentMonitor == index)) Oh I see. I made that last change and pushed to master, thanks!