GNOME Bugzilla – Bug 652063
Emit ::output-connected and ::output-disconnected when screen outputs come and go
Last modified: 2011-06-09 19:46:12 UTC
Created attachment 189414 [details] [review] patch for review If you're using GnomeRRScreen, it's likely you care about outputs, rather than the screen itself. These signals should be useful to the gnome-settings-daemon xrandr plugin too. --- We need this for the color plugin NOTE: I've got problems testing this as my T510 doesn't seem to want to emit events when the outputs change. I'll boot an old kernel at some stage, as I'm sure it used to work. At this stage I've just got to unload patches.
Review of attachment 189414 [details] [review]: I like the patch, in principle. So far, the users of GnomeRR have simply re-scanned the list of outputs to see what happened on every change. They are rather passive users. What if the color manager simply had this logic by itself? Or do you see this being used elsewhere? ::: libgnome-desktop/gnome-rr.c @@ +565,3 @@ +static GnomeRROutput * +find_output (GnomeRROutput **haystack, GnomeRROutput *needle) Please make this GnomeRROutput * find_output_by_id (GnomeRROutput **haystack, guint32 id) To make it clear that you are not simply matching the output pointers, but rather the actual IDs. @@ +656,3 @@ + /* work out if any outputs have changed connected state */ + compare_infos (screen->priv->info, info); The code that calls GnomeRR is a) highly contrived; b) really knowledgeable of what it is doing, so the following may not be very important... ... but do we need to pay attention to whether the signals are emitted before/after the point where we actually plug the new "info" into the screen->priv?
Created attachment 189448 [details] [review] patch for review (In reply to comment #1) > So far, the users of GnomeRR have simply re-scanned the list of outputs to see > what happened on every change. They are rather passive users. > > What if the color manager simply had this logic by itself? Or do you see this > being used elsewhere? Sure, GCM used to have a whole xrandr interface .c file, but hadess thought the functionality should go into gnome-desktop where it can be useful to other programs. It's also a case that doing this in the client, so to speak, would mean comparing the output states of the "old" outputs, which are not valid as soon as the 'changed' signal is emitted. This means we have to keep a separate cache of the xrandr output outputs and output states in the client, which is just eww. > ::: libgnome-desktop/gnome-rr.c > @@ +565,3 @@ > > +static GnomeRROutput * > +find_output (GnomeRROutput **haystack, GnomeRROutput *needle) > > Please make this > > GnomeRROutput * > find_output_by_id (GnomeRROutput **haystack, guint32 id) > > To make it clear that you are not simply matching the output pointers, but > rather the actual IDs. Fixed. > @@ +656,3 @@ > > + /* work out if any outputs have changed connected state */ > + compare_infos (screen->priv->info, info); > > The code that calls GnomeRR is a) highly contrived; b) really knowledgeable of > what it is doing, so the following may not be very important... > > ... but do we need to pay attention to whether the signals are emitted > before/after the point where we actually plug the new "info" into the > screen->priv? Yes, I was careful of that. Because GnomeRROutput are not proper GObjects, I couldn't lifecycle them like I wanted to. This is why they are treated as pointers in the signal emission. As I understand it, the signal emission is done sync (i.e. not in idle) and so the old->outputs[i] for example would be passed to the client *before* the screen_info_free() got done in screen_update() Maybe I have to make it explicit in a doc comment that you can't just g_object_ref() the GnomeRROutput and handle it in an idle handler or something. Advice welcome. Richard.
Or GnomeRROutput could be made into a GObject if it fixes lifecycle issues. If that's not possible, adding a mention that the GnomeRROutput "object" is only valid during the signal handler's lifetime, it would be enough to avoid making mistakes.
Review of attachment 189448 [details] [review]: ::: libgnome-desktop/gnome-rr.c @@ +600,3 @@ + continue; + } + if (gnome_rr_output_is_connected (tmp) && I'd like the tests to be if (condition (old) && condition (new)) i.e. first the old output, then the new output. For extra brownie points, make "id" be "old_id" and "tmp" be "new" or something. And I think you mean CONNECTED in the signal here, not DISCONNECTED. @@ +618,3 @@ + { + /* output created */ + if (gnome_rr_output_is_connected (old->outputs[i])) Pretty sure you mean new->outputs[i] here. @@ +657,3 @@ + /* work out if any outputs have changed connected state */ + compare_infos (screen->priv->info, info); Hmm, would diff_outputs_and_emit_signals() be a better name for that function?
(In reply to comment #2) > soon as the 'changed' signal is emitted. This means we have to keep a separate > cache of the xrandr output outputs and output states in the client, which is > just eww. Eww indeed :) OK, let's have this in GnomeRR. > Yes, I was careful of that. Because GnomeRROutput are not proper GObjects, I > couldn't lifecycle them like I wanted to. This is why they are treated as > pointers in the signal emission. As I understand it, the signal emission is > done sync (i.e. not in idle) and so the old->outputs[i] for example would be > passed to the client *before* the screen_info_free() got done in > screen_update() > > Maybe I have to make it explicit in a doc comment that you can't just > g_object_ref() the GnomeRROutput and handle it in an idle handler or something. Yes, please add docs to that effect. I don't think we need to GObject-ify these structs just yet (see bug #630913 for the initial pass of objectifying GnomeRR). I'm not sure if gnome-shell is happy enough with introspection as it is for GnomeRR right now.
Created attachment 189534 [details] [review] patch for review (In reply to comment #4) > Review of attachment 189448 [details] [review]: > > ::: libgnome-desktop/gnome-rr.c > @@ +600,3 @@ > + continue; > + } > + if (gnome_rr_output_is_connected (tmp) && > > I'd like the tests to be > > if (condition (old) && > condition (new)) > > i.e. first the old output, then the new output. For extra brownie points, make > "id" be "old_id" and "tmp" be "new" or something. Both fixed. Brownie please! > And I think you mean CONNECTED in the signal here, not DISCONNECTED. Fixed. > @@ +618,3 @@ > + { > + /* output created */ > + if (gnome_rr_output_is_connected (old->outputs[i])) > > Pretty sure you mean new->outputs[i] here. Ahh, good catch, thanks. > @@ +657,3 @@ > > + /* work out if any outputs have changed connected state */ > + compare_infos (screen->priv->info, info); > > Hmm, would diff_outputs_and_emit_signals() be a better name for that function? Agreed. Fixed. (In reply to comment #5) > Yes, please add docs to that effect. I don't think we need to GObject-ify > these structs just yet (see bug #630913 for the initial pass of objectifying > GnomeRR). Sure, I've added signal gtk-doc markup. New review please. Thanks. Richard.
Created attachment 189571 [details] [review] test patch Actually upload the new patch. Sorry!
Review of attachment 189571 [details] [review]: ::: libgnome-desktop/gnome-rr.c @@ +603,3 @@ + if (gnome_rr_output_is_connected (output_new) && + !gnome_rr_output_is_connected (old->outputs[i])) + { Looks like you have the case reversed here. In this first loop we are looking for what got disconnected; you want if (gnome_rr_output_is_connected (old->outputs[i]) && !gnome_rr_output_is_connected (output_new)) emit ("disconnected") @@ +920,3 @@ + * is not a good idea to query the output data this in an async or + * idle function. + **/ Proofreading and some changes - please fix the "disconnected" one similarly: * This signal is emitted when a display device is connected to a * port, or a port is hotplugged with an active output. The latter * can happen if a laptop is docked, and the dock provides a new * active output. * * The @output value is not a GObject. The returned @output value can * only assume to be valid during the emission of the signal (i.e. within * your signal handler only), as it may change later when the @screen * is modified due to an event from the X server, or due to another * place in the application modifying the @screen and the @output. * Therefore, deal with changes to the @output right in your signal * handler, instead of keeping the @output reference for an async or * idle function. */ @@ +956,1 @@ } Make those changes and go ahead and push; no further review is needed. Excellent work; thanks :)
(In reply to comment #6) > Both fixed. Brownie please! I forgot! Here's your nomz: @ @ @
(In reply to comment #8) > Review of attachment 189571 [details] [review]: > > ::: libgnome-desktop/gnome-rr.c > @@ +603,3 @@ > + if (gnome_rr_output_is_connected (output_new) && > + !gnome_rr_output_is_connected (old->outputs[i])) > + { > > Looks like you have the case reversed here. In this first loop we are looking > for what got disconnected; you want > > if (gnome_rr_output_is_connected (old->outputs[i]) && > !gnome_rr_output_is_connected (output_new)) > > emit ("disconnected") Oops, thanks. > @@ +920,3 @@ > + * is not a good idea to query the output data this in an async or > + * idle function. > + **/ > > Proofreading and some changes Fixed, thanks. (In reply to comment #9) > (In reply to comment #6) > > > Both fixed. Brownie please! > > I forgot! Here's your nomz: @ @ @ Nom nom nom. Thanks for the review, I've now pushed this to master. Richard.