Created attachment 154055 [details] [review]
st_widget_get_theme_node: break the widget's CSS style if called too early

It's wrong to do anything that requires looking up a widget's style
before you add the widget to the stage, since its final style may
depend on properties inherited from its parents.
st_widget_get_theme_node() emits a warning in this case, but many
would-be contributors apparently don't notice. Help them out.

Review of attachment 154055 [details] [review]:

I'd rather turn it into g_error.  Even better have some sort of test suite that catches this kind of stuff but that's a harder patch of course =)

I don't see why this is any more of a problem than any other g_warning g_critical that is being spewed by patches and not caught. If that's a problem, we need to either:

 A) Make them fatal
 B) Take error messages off of the controlling terminal and do something like log to a file and make the bad ones blink on the screen

And in most of the uses of st_widget_get_theme_node() that we are doing, I don't think your approach helps any - presumably we are looking up some custom property, so it won't be in the "obviously broken" CSS style.

I don't think returning a GError here makes sense - GError is only supposed to be used when it's something that the program can reasonably handle and do something about - usually in cases where an error should be displayed to the user. It's not supposed to be used for cases that represent buggy programs. Because there is no way that a program can reasonably handle its own bugs.

It would be nice if we had some way of throwing informative exceptions from C code back into JS, but we don't. It's either g_warning, g_critical(), or g_error().

Created attachment 154079 [details] [review]
st_widget_get_theme_node: g_error if called on an unparented widget

the g_error version

I wasn't sure if this was too harsh... in particular, if a patch introduced
such a bug, but only in a slightly-obscure case (say, dragging a window from
one workspace to another in the overview), such a bug might make it through
patch review and into git.

(In reply to comment #3)
>  B) Take error messages off of the controlling terminal and do something like
> log to a file and make the bad ones blink on the screen

that was my other thought (the "blink on the screen" bit). maybe we could use the message tray :-)

(well, unless the bug is in the message tray code...)

> And in most of the uses of st_widget_get_theme_node() that we are doing, I
> don't think your approach helps any - presumably we are looking up some custom
> property, so it won't be in the "obviously broken" CSS style.

right, but once you hit that code path, the widget gets permanently stuck to the broken CSS, so once you do put it on the stage somewhere, it will have a magenta background and a cyan border, etc.

> I don't think returning a GError here makes sense

agreed, i was assuming walters meant g_error() as opposed to g_warning()

(oh, also, most of the cases of this are people trying to get the height or width of an actor before putting it on the stage, and the st_widget_get_theme_node() call happens inside the guts of st.)

Created attachment 154121 [details] [review]
Fix invalid call to st_widget_get_theme_node() in WindowOverlay

(In reply to comment #4)
> I wasn't sure if this was too harsh... in particular, if a patch introduced
> such a bug, but only in a slightly-obscure case (say, dragging a window from
> one workspace to another in the overview), such a bug might make it through
> patch review and into git.

Of couse this absurd example case has been chosen arbitrarily ...

(In reply to comment #0)
> Three times in the last week or so I've had to point out where people's patches
> were causing this warning. (bug 607244 comment 15, bug 609777 comment 14, bug
> 610049 comment 9). We need to make this more noticeable.
> 
> One possibility would be to have st_widget_get_theme_node() crash or (probably
> equivalently) return NULL in this case. Another possibility is to just return
> nonsensical CSS in response to their nonsensical request.

Well the issue is that I tend to not notice warnings unless something does not work as expected. (I stopped keeping a terminal window open while running g-s a while ago because closing it by accident has an ugly side effect).
I.e if something is wrong I go look for warnings to see what might show up there, but in those cases it did what I expected it to do so the warnings went unnoticed.
(I know now that doing it that way is wrong but it wasn't obvious that it is)

Review of attachment 154121 [details] [review]:

::: js/ui/workspace.js
@@ +398,3 @@
         parentActor.add_actor(this.closeButton);
+
+        button.hide();

What does this have to do with the patch?

@@ +536,3 @@
     _onStyleChanged: function() {
+        if (!this.title.get_parent() || !this.closeButton.get_parent())
+            return;

Hmm, a bit ugly, but OK. Needs a comment though above this along the lines of 

 // Wait to run until both have been added to the actor hierarchy

(In reply to comment #9)
> Review of attachment 154121 [details] [review]:
> 
> ::: js/ui/workspace.js
> @@ +398,3 @@
>          parentActor.add_actor(this.closeButton);
> +
> +        button.hide();
> 
> What does this have to do with the patch?

The widget style is not updated until the widget is mapped - so without that change the first invocation of _onStyleChanged() returns immediately because the button has not been parented yet and the second invocation does not occur until the button is shown (read: the pointer hovers over the window clone) ...

Yeah, I'd suggest another comment ...


> @@ +536,3 @@
>      _onStyleChanged: function() {
> +        if (!this.title.get_parent() || !this.closeButton.get_parent())
> +            return;
> 
> Hmm, a bit ugly, but OK. Needs a comment though above this along the lines of 
> 
>  // Wait to run until both have been added to the actor hierarchy


What about something along the lines of:

  let _relayout = false;
  if (this.title.get_parent()) {
     _relayout = true;
     ...
  }
  if (this.closeButton.get_parent()) {
      _relayout = true;
      ...
  }
  if (_relayout)
    this.parentActor.queue_relayout();

Not sure if that's less ugly ... maybe the proper thing to do would be to add a container widget to WindowOverlay and add both widgets there?

(In reply to comment #10)
> (In reply to comment #9)
> > Review of attachment 154121 [details] [review] [details]:
> > 
> > ::: js/ui/workspace.js
> > @@ +398,3 @@
> >          parentActor.add_actor(this.closeButton);
> > +
> > +        button.hide();
> > 
> > What does this have to do with the patch?
> 
> The widget style is not updated until the widget is mapped - so without that
> change the first invocation of _onStyleChanged() returns immediately because
> the button has not been parented yet and the second invocation does not occur
> until the button is shown (read: the pointer hovers over the window clone) ...
> 
> Yeah, I'd suggest another comment ...

OK. then the easy fix here is something I was going to suggest, but decided wasn't prettier than the way you did it.

Move the style-changed connections to the end of the function and do:

 title.connect('style-changed', ...)
 button.connect('style-changed', ...)
 self._onStyleChanged()

Created attachment 154382 [details] [review]
Fix invalid call to st_widget_get_theme_node() in WindowOverlay

(In reply to comment #11)
> Move the style-changed connections to the end of the function and do:
> 
>  title.connect('style-changed', ...)
>  button.connect('style-changed', ...)
>  self._onStyleChanged()

Mmmmh, still a little ugly (the call to _onStyleChanged has to be done conditionally), but nevertheless nicer than the original patch ...

> Mmmmh, still a little ugly

if it helps, note that queue_relayout() is idempotent until the relayout actually happens, so you don't need to be careful about only calling it once as you suggested in comment #10. So you could just split _onStyleChanged into separate _onTitleStyleChanged and _onCloseButtonStyleChanged methods, and call queue_relayout() from both of them; if the styles change at the same time, it will still only cause a single relayout.

(In reply to comment #13)
> if it helps, note that queue_relayout() is idempotent until the relayout
> actually happens, so you don't need to be careful about only calling it once as
> you suggested in comment #10. So you could just split _onStyleChanged into
> separate _onTitleStyleChanged and _onCloseButtonStyleChanged methods, and call
> queue_relayout() from both of them; if the styles change at the same time, it
> will still only cause a single relayout.

I don't think that will work - the button is initially hidden, so it won't receive style-changed signals until shown; at the same time, its height is used when calculating the window clone's position.

I may not particularly like the current patch, but I'm not sure about there being a cleaner (and less intrusive!) solution ...

Review of attachment 154382 [details] [review]:

The question of whether parentActor was in the stage or not was actually why I didnt' suggest the approach originally. I didn't realize that we were being called both with and without parentActor being "anchored" to a stage so I didn't think a conditional would actually be needed, but without the conditional there was some "fragility" of the code that I didn't like - if things got rearranged it might break.

I think it's fine as you have it. Not absolutely beautiful but compact and clear :-)

Comment on attachment 154382 [details] [review]
Fix invalid call to st_widget_get_theme_node() in WindowOverlay

Attachment 154382 [details] pushed as 126d02a - Fix invalid call to st_widget_get_theme_node() in WindowOverlay

Review of attachment 154079 [details] [review]:

I'd like to go with making them fatal for now; let's apply this.

Attachment 154079 [details] pushed as d128cc5 - st_widget_get_theme_node: g_error if called on an unparented widget

I have a slight objection to the "let's just crash the shell" approach. when using a gnome-shell from a package then you're most likely never going to see the g_error() message - nor any other message, ever.

it's also pretty clear that just crashing with a g_error() hasn't helped at all avoiding committing patches that end up calling get_theme_node() on unparented actors, since a month after this particular course of action was chosen and committed I have a shell that crashes every time I get into the chrome.

one option that wouldn't be the equivalent of carpet bombing the user would be to use a g_critical() and have the shell run using a g_log_fatal_mask() of G_LOG_LEVEL_CRITICAL when running from a jhbuild root.

failing the fatal-critical approach, at least the g_error() should print out more debugging information, e.g.:

  - the widget for which the style was asked (name and/or type)
  - the parent widget, if any (name and/or type)

otherwise the usefulness in debugging is severely limited

Created attachment 247670 [details] [review]
st: Be more forgiving when calling get_theme_node() on unstaged widgets

While it is obviously still an error to call get_theme_node() on a
widget that hasn't been added to the stage hierarchy yet, asserting
on it hasn't proven too successful in avoiding those errors - it's
likely the most frequent reason for crash reports. Just accept that
there'll always be code paths where we can hit this case and make
it non-fatal.

Review of attachment 247670 [details] [review]:

OK.

Attachment 247670 [details] pushed as e70c0d3 - st: Be more forgiving when calling get_theme_node() on unstaged widgets

I went ahead and cherry picked this into 3.8 ... we shouldn't let users wait for another six months to finally get rid of this crashes.

*** Bug 682383 has been marked as a duplicate of this bug. ***

Thanks Florian! This "prevents" the crashing of Nautilus if started as first application.

*** Bug 695030 has been marked as a duplicate of this bug. ***