Created attachment 209795 [details] [review]
[PATCH 1/1] libpanel-applet: Fix random grey box on logout



See patch commit message for how and why this patch works

Review of attachment 209795 [details] [review]:

Thanks for tracking this down!

The one major issue I can see is that everything we do in unrealize should be to match something done in realize. I agree that, in this specific case, it shouldn't matter much, but still.

So there are several ways to deal with it:
 - connect to the signal in realize
 - block the signal in init (and when we unrealize), and unblock it on realize
 - check that we're realized in panel_applet_container_plug_removed() before emitting "applet-broken"

I don't really like any of those solutions, but the last one feels the less broken to me. Maybe it's just a matter of taste.

Also, did you check everything still works fine by killing an applet while the panel is running?

::: gnome-panel/libpanel-applet-private/panel-applet-container.c
@@ +164,3 @@
+	 *  from being generated. This prevents the "random grey box on
+	 *  logout"
+	 */

"* " instead of "*  " to have proper alignment in the comment :-)

I'm fine with any solution that works.  There's actually a bug in the existing patch (forgot to forward the unrealize -- whoops), so I'm attaching that patch as v2.

I'm also attaching a much simpler v3 patch based on option 3 above (ie, check the realized state). However, checking the realized state of the PanelAppletContainer doesn't work because gtk_widget_unrealize doesn't set the unrealized state until *after the children are unrealized* -- so the patch checks the unrealized state of the socket instead. Ok?

Created attachment 209831 [details] [review]
[PATCH v2 1/1] libpanel-applet: Fix random grey box on logout

Created attachment 209832 [details] [review]
[PATCH v3 1/1] libpanel-applet: Fix random grey box on logout

Review of attachment 209832 [details] [review]:

Thanks, I think that's the approach I like best. A few comments, though:

Did you check it worked fine for the non-logout cases? (You can simulate an applet crashing by killing its process)

::: gnome-panel/libpanel-applet-private/panel-applet-container.c
@@ +230,3 @@
 static gboolean
 panel_applet_container_plug_removed (PanelAppletContainer *container)
 {

We need a comment explaining why we check the realized state of the socket. Something like:
"If the socket is not realized, then it means the container is getting unrealized and that's why the plug is removed."

@@ +232,3 @@
 {
+	if (!gtk_widget_get_realized (container->priv->socket))
+		return FALSE;

Actually, I wouldn't return FALSE: I'd just move the "g_signal_emit" in this if:

if (!gtk_widget_get_realized (container->priv->socket))
  g_signal_emit (container, signals[APPLET_BROKEN], 0);

I think we want to keep all the other code in this function when this occurs.

I just forgot to mention that earlier: I tested both reload and don't reload code paths, and they function as expected.

Sure, I'll add a comment.

I just exited here because all the functionality for cleaning up after the container is in panel_applet_container_dispose. The functionality in panel_applet_container_plug_removed is present only for when the applet is being restarted so the relevant container state needs to be reset. I could add that to the comment as well.

That said, if you'd still prefer to execute this cleanup anyway, I'll just conditionalize the APPLET_BROKEN signal as you suggest.

(In reply to comment #7)
> I just forgot to mention that earlier: I tested both reload and don't reload
> code paths, and they function as expected.

Good to know, thanks!

> I just exited here because all the functionality for cleaning up after the
> container is in panel_applet_container_dispose. The functionality in
> panel_applet_container_plug_removed is present only for when the applet is
> being restarted so the relevant container state needs to be reset. I could add
> that to the comment as well.
> 
> That said, if you'd still prefer to execute this cleanup anyway, I'll just
> conditionalize the APPLET_BROKEN signal as you suggest.

The reason I prefer to execute that cleanup is to avoid any kind of assumption like "we're getting unrealized, so we'll be disposed too very soon". It's likely a true statement, but not something that can be taken for granted.

Created attachment 210087 [details] [review]
[PATCH v4 1/1] libpanel-applet: Fix random grey box on logout

Made requested changes; re-tested ok.

Comment on attachment 210087 [details] [review]
[PATCH v4 1/1] libpanel-applet: Fix random grey box on logout

Thanks, pushed!