Created attachment 153309 [details] [review]
Implement display:none in StWidget

Occasionally it is convenient to be able to hide components using CSS.
Implement the display property, which can be set to "none" to achieve
this. The values "block" and "inline" are parsed as well, but neither
one have any effect.
The implementation in StWidget is extremely naive, but appears to
work surprisingly well in most cases.

Created attachment 153310 [details] [review]
[StScrollbar] Respect display:none when allocating

Independant from any actual size or display settings, the forward/backward
steppers will always have a fixed size.
Take display:none into account, so that the steppers may be hidden
separately.

Created attachment 153311 [details] [review]
[Overview] Make background color stylable

Created attachment 153312 [details] [review]
[InfoBar] Make spacing between message and button stylable

Created attachment 153313 [details] [review]
[CSS] Update overview style to match latest mockup

Created attachment 153336 [details] [review]
[Overview] Make background color stylable

Set the background color, so that the patch does not change appearance.

Created attachment 153337 [details] [review]
[InfoBar] Make spacing between message and button stylable

Update CSS so that there is no change in appearance.

Created attachment 153338 [details] [review]
[CSS] Update overview style to match latest mockup

Rebased patch.

Review of attachment 153309 [details] [review]:

I'd drop the block and inline, I don't think they'll ever make sense for St.  

Two other concerns:

* I think not chaining up in allocate is potentially problematic, among other things we'll never reset the needs_allocation flag
* This patch won't handle dynamic changes I believe since we won't queue a reallocate

Created attachment 153347 [details] [review]
[StScrollbar] Allocate steppers according to size requests

Change the scrollbar patch to use the steppers' size requests instead of
allocating a fixed size depending on the display property.

A side effect is that the patch now is independent of the other patches in this bug (hooray!), although the CSS patch looks crappy without ...

Created attachment 153348 [details] [review]
[CSS] Update overview style to match latest mockup

Updated CSS to not use display:none to hide the scroll bar steppers.

(In reply to comment #9)
> Review of attachment 153309 [details] [review]:
> 
> I'd drop the block and inline, I don't think they'll ever make sense for St.

Yeah, I don't think they'll ever be meaningful either - basically I included them to specify not-none.

It's probably better to make display_none a boolean property in StThemeNode which is set to TRUE iff display:none is specified in the CSS.

That is, if we don't drop the patch altogether ;)

The scroll bar patch got updated to respect size requests, so this patch is no longer required by anything.

We could therefore either:

 - trash it

 - rewrite it as a shortcut for "width: 0px; height: 0px;" in StThemeNode; as
   this would not affect StWidget at all, it would both address your valid
   concerns regarding the current version and not interfere with your ongoing
   work on the widget painting

Opinions?

Review of attachment 153347 [details] [review]:

So looking at this the StScrollBar looks even weirder to me in that it appears to be a St.Bin with no child (but 3 actors who have it set as their parent).  It overrides _allocate but not _get_preferred_[width,height]. So I think it would request an overall width/height of 0, right?

Ohh I see, in st-scroll-view.c there's:

st_theme_node_get_length (theme_node, "scrollbar-width", FALSE, &result);

Which is kind of lame.

Anyways let's not block on draining this swamp right now, this patch looks fine to me if it solves your immediate problem.

Review of attachment 153336 [details] [review]:

::: js/ui/overview.js
@@ +225,3 @@
         // Background color for the Overview
 
+        this._backOver = new St.Label();

Why St.Label instead of St.Widget?  And actually why do we still have _backOver at all if this._group is now painting the background?

Review of attachment 153337 [details] [review]:

I don't quite understand the bug this is fixing but the patch seems OK to me.

Review of attachment 153348 [details] [review]:

One comment you don't have to address in this patch.

::: data/theme/gnome-shell.css
@@ +226,2 @@
 }
+#SwitchScroll #hhandle {

This isn't in the scope of this patch but I think that "hhandle" should probably be a style class instead of a  name.  Though we aren't really trying to restrict uniqueness of names now.

Hm, maybe we could get away with using "#SwitchScroll StButton".

(In reply to comment #16)
> Review of attachment 153348 [details] [review]:
> 
> One comment you don't have to address in this patch.
> 
> ::: data/theme/gnome-shell.css
> @@ +226,2 @@
>  }
> +#SwitchScroll #hhandle {
> 
> This isn't in the scope of this patch but I think that "hhandle" should
> probably be a style class instead of a  name.  Though we aren't really trying
> to restrict uniqueness of names now.
> 
> Hm, maybe we could get away with using "#SwitchScroll StButton".

Broken record: selectors that add with actor names are expensive. #SwitchScroll StButton means "for every StButton, when computing classes, check to see if any parent has the #SwitchScroll name" - sometimes this could be optimized, especially if we made names actually unique, but it isn't currently and its not trivial.

> Broken record: selectors that add with actor names are expensive. #SwitchScroll
                                ^^^
                                end

(In reply to comment #18)
> > Broken record: selectors that add with actor names are expensive. #SwitchScroll
>                                 ^^^            ^^^^^
>                                 end            |||||
                                                class names

(to be perfectly clear - what would be the tag name in HTML)

(In reply to comment #16)
> Hm, maybe we could get away with using "#SwitchScroll StButton".

Owen's notes on performance aside, the above matches both handle and steppers, so no, in most cases we wouldn't get away with it ...


(In reply to comment #15)
> Review of attachment 153337 [details] [review]:
> 
> I don't quite understand the bug this is fixing but the patch seems OK to me.

The spacing between message and button is done by adding a literal space to the message (text = text + ' '), the patch changes that to apply spacing to the box layout.

The patch also moves the 'info-bar' class name from the second StBin to the box layout, which is not strictly necessary - it changes the CSS to end with a widget class name, which should be more expensive (bad), but it allows the CSS patch to set the spacing with ".info-bar" instead of ".info-bar StBoxLayout" (good).

On a side note, I wonder if class names still are that expensive when
(1) used as .class-name > WidgetClass
(2) the child widget is a WidgetClass, not a subclass


(In reply to comment #14)
> Why St.Label instead of St.Widget?  And actually why do we still have _backOver
> at all if this._group is now painting the background?

StWidget is an abstract type and cannot be instantiated. There is a comment in the code about this._group being positioned on the primary monitor only, while this._backOver is stretched to span the entire screen. Not sure if that's still accurate, but I don't have a multi monitor setup, so I don't dare touching that.

(In reply to comment #20)
> On a side note, I wonder if class names still are that expensive when
> (1) used as .class-name > WidgetClass

This still has every-actor-of-the-class expense, but it's much les than .class-name WidgetClass since only the parent needs to be checked not the entire hierarchy.

> (2) the child widget is a WidgetClass, not a subclass

The rarer 'Foo' is in our stage, the cheaper that:

 .something 'Foo'

is. So:

 .something StScrollView

would be pretty cheap, while 

 .something StWidget

is very expensive.

The underlying logic for both of the above is pretty straightforward - it just comes from the way we find the style rules that apply to a given actor:

 - We scan through all the selector in the CSS file linearly
 - For each selector we check if it matches the the actor
   - We check if the last element of the selector matches the actor
   - If it does, we check the next-to-last against the parent (with the > 
     combinator), or against all parents (with no combinator)
   - And the same for the second-to-last element and so forth

So the cost of:

 .something StWidget

Is basically O(n log(n)) in the number of actors in our stage - we have to test .something against every parent of every StWidget.

(In reply to comment #21)

Thanks for the clarification!

Attachment 153337 [details] pushed as 9a1cb9c - [InfoBar] Make spacing between message and button stylable
Attachment 153347 [details] pushed as 32e2ff7 - [StScrollbar] Allocate steppers according to size requests

Comment on attachment 153309 [details] [review]
Implement display:none in StWidget

Marking attachment 153309 [details] [review] as obsolete to get it off the list.

Review of attachment 153336 [details] [review]:

Resolved on IRC; this._backOver will inherit the background-color from this._group, which it needs to since this._backOver is expanded past the group.

Attachment 153336 [details] pushed as 2a2b597 - [Overview] Make background color stylable
Attachment 153348 [details] pushed as d8af8f7 - [CSS] Update overview style to match latest mockup