GNOME Bugzilla – Bug 743744
Add an OSD monitor labeler exposed on DBus
Last modified: 2015-02-05 12:46:09 UTC
This DBus API is intended to be used by gnome-control-center's displays panel to show monitor labels. Each output (i.e. hardware monitor) identified by its org.gnome.Mutter.DisplayConfig API ID has at most one label. On mirrored setups, all the labels for outputs corresponding to the same logical monitor (i.e. showing the same contents in the same mode) are shown together. At most, only one DBus client at a time is allowed to show labels.
Created attachment 295807 [details] [review] Add an OSD monitor labeler exposed on DBus
Created attachment 296099 [details] [review] Add an OSD monitor labeler exposed on DBus -- Changed the DBus API to receive a{uv} instead of a{us} as suggested by Bastien in https://bugzilla.gnome.org/show_bug.cgi?id=743743#c4 .
Review of attachment 296099 [details] [review]: ::: js/ui/osdMonitorLabeler.js @@ +28,3 @@ + let str = labels.reduce(function(previous, current) { + return previous + ' ' + current; + }); What is the use case for multiple labels per monitor? It doesn't look like the old code supported that, and it's not obvious either how you'd get this with the new code. Also if there's a single value, then the call to reduce() will leave the original type untouched and using 'str' to construct the label will fail for non-string values. This is all a bit confusing, so can we just have a single 'label' parameter of type string here? Even if the DBus API must support multiple labels, I'd still prefer those details not leaking into OsdMonitorLabel and do the reducing/type conversion in MonitorLabeler. @@ +35,3 @@ + Main.uiGroup.add_child(this._actor); + + this._actor.get_parent().set_child_above_sibling(this._actor, null); Any reason for not using Main.uiGroup directly? It's not like anything would have changed the container since the previous line ... @@ +77,3 @@ + this._clientWatchId = 0; + this._osdLabels = []; + this._monitorLabels = new Map(); This is immediately left to GC in _reset() - maybe just set to null here or leave it undefined? @@ +98,3 @@ + return true; + else + return false; Doesn't look more readable to me than a simple return (this._client == client); but up to you ::: js/ui/osdWindow.js @@ +3,3 @@ const Clutter = imports.gi.Clutter; const GLib = imports.gi.GLib; +const Gio = imports.gi.Gio; Left-over ::: js/ui/shellDBus.js @@ +53,3 @@ + <arg type="a{uv}" direction="in" name="params" /> \ +</method> \ +<method name="HideMonitorLabels" /> \ I'd put this close to the related ShowOSD method rather than mixing it with the properties @@ +249,3 @@ + let [dict] = params; + Main.osdMonitorLabeler.show(sender, dict); + invocation.return_value(null); So far we don't explicitly return from DBus methods with no "out" parameters, which is fine as far as I can tell. If it's not, maybe do a small cleanup patch first to add the returns.
Created attachment 296138 [details] [review] Add an OSD monitor labeler exposed on DBus -- (In reply to comment #3) > + let str = labels.reduce(function(previous, current) { > + return previous + ' ' + current; > + }); > What is the use case for multiple labels per monitor? It doesn't > look like the old code supported that, and it's not obvious either > how you'd get this with the new code. It's explained in the commit message: it's for mirrored setups. I added a comment in the code now as well. The current g-c-c code just doesn't show anything on mirrored setups but that seems weird to me and I thought we could do better. Particularly if you consider e.g. a triple display case where two of them are mirrored: the current code doesn't show any labels at all. With this we'd show [ 1 2 ] [ 3 ] > Also if there's a single > value, then the call to reduce() will leave the original type > untouched and using 'str' to construct the label will fail for > non-string values. Not sure what to do about that given that Bastien prefers a variant for the label in the DBus API. In any case, since this is just to be used by g-c-c, if that ever happened it'd be a g-c-c bug. > This is all a bit confusing, so can we just have > a single 'label' parameter of type string here? Even if the DBus API > must support multiple labels, I'd still prefer those details not > leaking into OsdMonitorLabel and do the reducing/type conversion in > MonitorLabeler. Fair enough, I moved that part to MonitorLabel and just pass the finished label to OsdMonitorLabel. > Any reason for not using Main.uiGroup directly? It's not like > anything would have changed the container since the previous line > ... c&p the code from osdWindow.js, anyway I changed to use Main.uiGroup directly now. > + this._monitorLabels = new Map(); > This is immediately left to GC in _reset() - maybe just set to null > here or leave it undefined? Yeah, I'm setting it to null now. > Doesn't look more readable to me than a simple > return (this._client == client); Sure. > +const Gio = imports.gi.Gio; > > Left-over Ups, removed. > +<method name="HideMonitorLabels" /> \ > I'd put this close to the related ShowOSD method rather than mixing > it with the properties Ok. > + invocation.return_value(null); > So far we don't explicitly return from DBus methods with no "out" > parameters, which is fine as far as I can tell. If it's not, maybe > do a small cleanup patch first to add the returns. OK, I just removed them. Thanks
Review of attachment 296138 [details] [review]: (In reply to comment #4) > It's explained in the commit message: it's for mirrored setups. I > added a comment in the code now as well. Oh, I overlooked the commit message - still, it's something I can see myself stumbling over in the future, so not having to digg out the corresponding commit makes sense to me. > > Any reason for not using Main.uiGroup directly? It's not like > > anything would have changed the container since the previous line > > ... > > c&p the code from osdWindow.js, anyway I changed to use Main.uiGroup > directly now. Touché - let's blame the idiot who wrote osdWindow :-) ::: js/ui/osdMonitorLabeler.js @@ +132,3 @@ + labels.reduce(function(p, c) { + return p + ' ' + c; + }))); Could use a toString() for safety (e.g. where 'v' contains '<1>' rather than '<"1">') ... I'd probably split this as let label = labels.sort().reduce(function(p, c) ...); osdLabels.push(new OsdMonitorLabel(monitor, label.toString())); but keeping the existing split and just adding toString() after the reduce() obviously works as well.
(In reply to comment #3) > ::: js/ui/osdMonitorLabeler.js > @@ +28,3 @@ > + let str = labels.reduce(function(previous, current) { > + return previous + ' ' + current; > + }); labels.join(' '); ? (Though count me in for the single-label crowd)
(In reply to comment #6) > labels.join(' '); ? Oh, that's clearly better, not least because it will always return a string!
(In reply to comment #4) > The current g-c-c code just doesn't show anything on mirrored setups > but that seems weird to me and I thought we could do > better. Particularly if you consider e.g. a triple display case where > two of them are mirrored: the current code doesn't show any labels at > all. With this we'd show > > [ 1 2 ] [ 3 ] Why can't g-c-c pass "1 2" as their label on their side? The thing where we have multiple "monitors" and "outputs" and "CRTCs" and everything else seems overly ridiculous. Keep in mind that RandR 1.6 is getting a Monitors type to handle multi-panel monitors, so we should probably wait to see what Dave comes up with for a rewrite of the DBus API.
Created attachment 296187 [details] [review] Add an OSD monitor labeler exposed on DBus -- (In reply to comment #6) > labels.join(' '); ? Ah right, much better, thanks!
(In reply to comment #8) > Why can't g-c-c pass "1 2" as their label on their side? Because how do you identify "virtual monitors"? There's no single ID that we can reliably use on both wayland and X sessions from the client side. GDK has a monitor concept but it maps to MetaOutputs too and they're even ordered differently by GDK's wayland and X backends. AFAICT, only the output IDs that mutter exports in the DisplayConfig API are unambiguous enough that we can use them to map to mutter's internal "virtual monitors". > The thing where we > have multiple "monitors" and "outputs" and "CRTCs" and everything else seems > overly ridiculous. Isn't it? And it doesn't help that on the client side, GDK has its own names for these things which don't really match mutter's objects semantics either. > Keep in mind that RandR 1.6 is getting a Monitors type to handle multi-panel > monitors, so we should probably wait to see what Dave comes up with for a > rewrite of the DBus API. I'm aware but we'll cross that bridge when it happens. This is private API so we can change it whenever we want to.
Review of attachment 296187 [details] [review]: LGTM
Attachment 296187 [details] pushed as 1900468 - Add an OSD monitor labeler exposed on DBus