Created attachment 146947 [details] [review]
[StThemeNode] Don't give negative width/height for content boxes

If the space we're allocated is too small for our border + padding
constraints, don't give negative allocations to callers.  Squash
to zero.

It isn't really useful for callers to get negative content sizes,
and certainly breaks most allocation code.

Created attachment 146948 [details] [review]
[gnome-shell.css] Remove blue background from scrollbars

This makes it slightly less ugly.

Created attachment 146949 [details] [review]
switch to scrolling GenericDisplay, remove menus from apps

Temporarily removed search-by-menu from apps as well, will readd
later as a ShellApp feature.

Comment on attachment 146949 [details] [review]
switch to scrolling GenericDisplay, remove menus from apps

This one is in-progress.

Comment on attachment 146947 [details] [review]
[StThemeNode] Don't give negative width/height for content boxes

>+  content_box->x2 = (int)(0.5 + content_width);
>+  content_box->y2 = (int)(0.5 + content_height);

You need to add content_box->x1 and ->y1 to those (or noncontent_left and _top).

Otherwise good

(In reply to comment #5)
> (From update of attachment 146947 [details] [review])
> >+  content_box->x2 = (int)(0.5 + content_width);
> >+  content_box->y2 = (int)(0.5 + content_height);
> 
> You need to add content_box->x1 and ->y1 to those (or noncontent_left and
> _top).

Oops, good catch thank you!

Comment on attachment 146947 [details] [review]
[StThemeNode] Don't give negative width/height for content boxes

Attachment 146947 [details] pushed as dcd7762 - [StThemeNode] Don't give negative width/height for content boxes

Comment on attachment 146948 [details] [review]
[gnome-shell.css] Remove blue background from scrollbars

Attachment 146948 [details] pushed as fcac7ac - [gnome-shell.css] Remove blue background from scrollbars

Created attachment 147136 [details] [review]
switch to scrolling GenericDisplay, remove menus from apps

Temporarily removed search-by-menu from apps as well, will readd
later as a ShellApp feature.

Created attachment 147137 [details] [review]
[genericDisplay] remove information button

Presently doesn't do much except for larger images, but for those
we want to get people into the image viewing app.

Created attachment 147138 [details] [review]
[dash] Port pane to CSS

Created attachment 147139 [details] [review]
[dash CSS] Move closer to shellChrome

Created attachment 147140 [details] [review]
[genericDisplay] Port to CSS, style cleanups

Mostly a straightforward porting of style code to CSS.

Add some padding at the top between the close button and the items.

Center the text and description.

Created attachment 147141 [details] [review]
Add missing Lang.bind() in genericDisplay

Created attachment 147142 [details] [review]
[overview] Constrain popup panes, dim workspaces when active

Intermediary step for future design.

Created attachment 147143 [details] [review]
[dash] Port section container to CSS, lower spacing

Newer mockup has smaller spacing.

Created attachment 147144 [details] [review]
[places] Port more to CSS, shrink spacing

Shrink the spacing to correspond with newer mockup.

Created attachment 147145 [details] [review]
[dash] Swap More for triangle

In newer mockup.

Review of attachment 146947 [details] [review]:

::: src/st/st-theme-node.c
@@ +2238,3 @@
+
+  content_box->x2 = (int)(0.5 + content_width);
+  content_box->y2 = (int)(0.5 + content_height);

Where's content_box->x1 in this?

Suggest instead of this simply adding to the existing code:

 context_box->x2 = MAX (context->box->x1, context->box->x2);
 context_box->y2 = MAX (context->box->y1, context->box->y2);

Review of attachment 147136 [details] [review]:

genericDisplay.js changes look fine except for details. dash.js not sure about - without trying them, looks like the behavior would be weird. Removal of menus from apps more - not going to review that in detail. If we want to make that intermediate step, I'm sure you've figured out how to rip the code out correctly :-)

::: js/ui/dash.js
@@ -796,3 @@
             this._searchResultsSection.content.append(section.header.actor, Big.BoxPackFlags.NONE);
             section.resultArea = new ResultArea(section.type, false);
-            section.resultArea.controlBox.hide();

Won't the effect here be that the first page of search results gets a scrollbar per area, which is going to be awkward (small scrolling areas areas are always awkward) and ugly. Do we want scrolling before we dive into a category?

::: js/ui/genericDisplay.js
@@ -399,3 +387,3 @@
     // around to the bottom.
     selectUp: function() {
-        let count = this._list.displayedCount;
+        let count = this._list.get_children().length;

This is sort of horrifyingly inefficient. Can you use instead this.getMatchedItemsCount()? [three or so more examples below]

@@ -636,3 @@
      * Updates the displayed items, applying the search string if one exists.
      * @flags: Flags controlling redisplay behavior as follows:
-     *  RESET_CONTROLS - indicates if the page selection should be reset when displaying the matching results.

You don't seem to have removed the actual enum value

@@ +619,3 @@
      *  SUBSEARCH - Indicates that the current _search is a superstring of the previous
      *  one, which implies we only need to re-search through previous results.
      *  FULL - Indicates that we need recreate all displayed items; implies RESET_CONTROLS as well

You didn't remove this referece

Review of attachment 147137 [details] [review]:

I don't really like the thought of leaving a bunch of unconnected code in place with the idea that we'll add it back in some other form later. Is there a reason we need to remove the (i) button right now before we start working on the code for some better sort of "more information"?

Review of attachment 147139 [details] [review]:

Presumably OK, needs to be squashed with preceding commit.

Review of attachment 147139 [details] [review]:

Presumably OK, needs to be squashed with preceding commit.

Review of attachment 147138 [details] [review]:

Looks fine, make sure that the close.svg move is actually in your commit. (Hard to tell from the patch)

Review of attachment 147141 [details] [review]:

Looks fine, remember to squash

Review of attachment 147140 [details] [review]:

Only see a few minor things, mostly looks good to me.

::: data/theme/gnome-shell.css
@@ +133,3 @@
+.dash-recent-docs-item {
+    font-size: 14px;
+    color: #ffffff;

It's not a coincidence that the white color and font size

@@ +146,3 @@
+    border-radius: 4px;
+    color: #ffffff;
+    font-size: 14px;

Is shared with this

@@ +163,3 @@
+.generic-display-item-description {
+    font-size: 12px;
+    color: rgba(255,255,255,0.73)

[ Would be more understandable as gray rather than a partially transparent white. ]

@@ +170,3 @@
+.app-icon-label {
+    font-size: 12px;
+    color: #ffffff;

And the white color is shared with this

@@ +177,3 @@
+.places-item {
+    font-size: 14px;
+    color: #ffffff;

And the white color and font size is again shared here; so I think really that should be represented in the CSS instead of duplicated. This might mean using multiple style classes for some actors.

::: js/ui/genericDisplay.js
@@ +109,3 @@
     markSelected: function(isSelected) {
+        let themeNode = this.actor.get_theme_node();
+        this.actor.set_style_pseudo_class(isSelected ? "selected" : null);

You don't use the themeNode

Review of attachment 147142 [details] [review]:

::: js/ui/overview.js
@@ +200,2 @@
         // Dynamic width
+        this._paneContainer.height = this._workspacesHeight;

Can't make heads or tails of this

Review of attachment 147143 [details] [review]:

Mostly looks good

::: js/ui/dash.js
@@ -578,3 @@
 
-        this.sectionArea = new Big.Box({ orientation: Big.BoxOrientation.VERTICAL,
-                                         spacing: DASH_SECTION_SPACING });

Shouldn't you remove the constant then?

Review of attachment 147144 [details] [review]:

::: js/ui/places.js
@@ +92,3 @@
         // Subdivide left into actions and devices
+        this._actionsBox = new St.BoxLayout({ vertical: true,
+                                               style_class: "places-actions" });

Seems you forgot to add gnome-shell.css changes to the patch?

Review of attachment 147145 [details] [review]:

Looks straightforaward. section-more.svg needs to be added to the Makefile.am. Are section-more.svg and back.svg consistent or slightly different?

::: data/theme/gnome-shell.css
@@ +187,3 @@
 
+.places-actions {
+    spacing: 4px;

Should have been in the other patch

Created attachment 147423 [details] [review]
[StBoxLayout] Add st_box_layout_remove_all, st_box_layout_destroy_all

In a variety of places we're using boxes as data-modeling displays,
and in doing so we often want to either remove the children or
explictly destroy them.

Now ideally Gjs would support callbacks, and this would make using
the for_each functions possible, but even then these functions
are more efficient and shorter to type, at least.

Created attachment 147424 [details] [review]
[StBoxLayout] Add st_box_layout_get_n_children

Created attachment 147425 [details] [review]
switch to scrolling GenericDisplay, remove menus from apps

Temporarily removed search-by-menu from apps as well, will readd
later as a ShellApp feature.

Created attachment 147426 [details] [review]
[genericDisplay] remove information button

Presently doesn't do much except for larger images, but for those
we want to get people into the image viewing app.

Created attachment 147427 [details] [review]
[overview] Port pane to CSS

Created attachment 147428 [details] [review]
[genericDisplay+other] Port to CSS, style cleanups

Mostly a straightforward porting of style code to CSS, except
that various bits of other code referenced a few GenericDisplay
constants, so those needed to be ported as well.

Add some padding at the top between the close button and the items.

Center the text and description.

Created attachment 147429 [details] [review]
[overview] Constrain popup panes, dim workspaces when active

Intermediary step for future design.

Created attachment 147430 [details] [review]
[dash] Port section container to CSS, lower spacing

Newer mockup has smaller spacing.

Created attachment 147431 [details] [review]
[placeDisplay] Port more to CSS, lower spacing

Newer mockups have smaller spacing here.

Created attachment 147432 [details] [review]
[dash] Swap More for triangle

In newer mockup.

(In reply to comment #27)
> Review of attachment 147142 [details] [review]:
> 
> ::: js/ui/overview.js
> @@ +200,2 @@
>          // Dynamic width
> +        this._paneContainer.height = this._workspacesHeight;
> 
> Can't make heads or tails of this

Created attachment 147505 [details] [review]
[StBoxLayout] Add st_box_layout_remove_all, st_box_layout_destroy_all

In a variety of places we're using boxes as data-modeling displays,
and in doing so we often want to either remove the children or
explictly destroy them.

Now ideally Gjs would support callbacks, and this would make using
the for_each functions possible, but even then these functions
are more efficient and shorter to type, at least.

Created attachment 147506 [details] [review]
[StBoxLayout] Add st_box_layout_get_n_children

Created attachment 147507 [details] [review]
[StBoxLayout] Add missing _queue_relayout in _add_actor

Created attachment 147508 [details] [review]
[StTooltip] Fix _get_preferred_width

It was ignoring the label width.

Created attachment 147509 [details] [review]
[StTooltip CSS] Add some style

Not final, but stands out and doesn't look completely wrong, at least.

Created attachment 147510 [details] [review]
switch to scrolling GenericDisplay, remove menus from apps

Temporarily removed search-by-menu from apps as well, will readd
later as a ShellApp feature.

Created attachment 147511 [details] [review]
[overview] Port pane to CSS

Created attachment 147512 [details] [review]
[genericDisplay+other] Port to CSS, style cleanups

Mostly a straightforward porting of style code to CSS, except
that various bits of other code referenced a few GenericDisplay
constants, so those needed to be ported as well.

Add some padding at the top between the close button and the items.

Center the text and description.

Created attachment 147513 [details] [review]
[overview] Constrain popup panes, dim workspaces when active

Intermediary step for future design.

Created attachment 147514 [details] [review]
[dash] Port section container to CSS, lower spacing

Newer mockup has smaller spacing.

Created attachment 147515 [details] [review]
[dash] Port search section headers to CSS

Experimented with moving the (see all) into a tooltip, but
given that we're not emphasizing the drilldown, removed for now.

Created attachment 147516 [details] [review]
[placeDisplay] Port more to CSS, lower spacing

Newer mockups have smaller spacing here.

Created attachment 147517 [details] [review]
[dash] Swap More for triangle

In newer mockup.

Review of attachment 147505 [details] [review]:

Looks good with one naming suggestion.

::: src/st/st-box-layout.c
@@ +1382,3 @@
+ */
+void
+st_box_layout_destroy_all (StBoxLayout *self)

Think this would be clearer as destroy_children() - destroy_all() doesn't make immediate sense to me reading it... by comparison., gtk_widget_show_all() shows the *widget* and all children. I think the inconsistency with remove_all() is fine. [The remove_all() name probably should be kept for consistency with clutter_group_remove_all()]

Review of attachment 147506 [details] [review]:

Looks fine - I guess you could argue that 

 if (box.get_children().length > 0)

would be better replaced with a O(1):

 if (box.has_children())

but I don't think we need the API clutter - counting the elements of a list is vastly faster than constructing a JS array for the list.

::: src/st/st-box-layout.c
@@ +1389,3 @@
+/**
+ * st_box_layout_get_n_children:
+ * @self:

Should be @self: a #StBoxLayout

[ Yeah, doesn't add anything, but that's the style and the empty description draws the eye ]

Review of attachment 147507 [details] [review]:

Clutter group does the ordering as:

  /* queue a relayout, to get the correct positioning inside
   * the ::actor-added signal handlers
   */
  clutter_actor_queue_relayout (CLUTTER_ACTOR (group));

  g_signal_emit_by_name (container, "actor-added", actor);

Probable makes sense to do the same here, generally a good idea to emit signals as late as possible. Otherwise looks good.


Seems reasonable to do the same here - always emit signals as late as possible.

Review of attachment 147508 [details] [review]:

This isn't right - the arrow width doesn't contribute to the width of the label since it's stacked with the label vertically. I think the right fix is more like:

      clutter_actor_get_preferred_width (priv->label,
                                         label_height,
-                                         &min_label_w,
-                                         &natural_label_w);
+                                         min_width_p,
+                                         natural_width_p);

(And same in the other branch, and remove the unused variables.)

Review of attachment 147509 [details] [review]:

Not sure if it's worth quibbling here, but I'm pretty sure 0.66 is way too transparent - 0.66 doesn't preserve legibility - we use 0.9 for the calendar popup and IIRC for GTK+ tooltips.

Review of attachment 147511 [details] [review]:

::: data/theme/gnome-shell.css
@@ +127,3 @@
 
+.dash-pane {
+    background-color: rgba(00,00,00,0.95);

Didn't comment on this last time, but I object to the 00 being used here - that makes the implication that these are hex numbers like a #000000ff, but they are *decimal* numbers.

Review of attachment 147514 [details] [review]:

Same as previous version except for one bug-fix

Review of attachment 147517 [details] [review]:

Same as previous patch except for Makefile.am addition

::: data/Makefile.am
@@ +32,3 @@
 	theme/scroll-button-up-hover.png	\
+	theme/scroll-vhandle.png        \
+	theme/section-more.svg

Please keep the \ lined up

Review of attachment 147516 [details] [review]:

New version looks good

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

Review of attachment 147515 [details] [review]:

Looks good, except for problem noted below

::: data/theme/gnome-shell.css
@@ +101,3 @@
+}
+
+.dash-search-section-title, dash-search-section-count-text {

The latter doesn't match what you use in the code - doesn't have -text

Review of attachment 147512 [details] [review]:

Looks reasonable, few details about the CSS

::: data/theme/gnome-shell.css
@@ +150,3 @@
 
+.dash-recent-docs-item {
+}

Our CSS lookup performance is up O(n) in the number of rules, so I think we should avoid empty rules being there for future convenience.

@@ +175,3 @@
+
+.generic-display-item-name {
+}

Empty

@@ +179,3 @@
+.generic-display-item-description {
+    font-size: 12px;
+    color: rgba(255,255,255,0.73)

Really, should be gray not transparent white

@@ +200,3 @@
+
+.places-item {
+}

Empty

::: js/ui/dash.js
@@ -541,3 @@
 Section.prototype = {
     _init: function(titleString, suppressBrowse) {
-        this.actor = new Big.Box({ spacing: SECTION_INNER_SPACING });

There were no other users of this constant than the two here

@@ +545,2 @@
         this.header = new SectionHeader(titleString, suppressBrowse);
+        this.actor.add(this.header.actor, Big.BoxPackFlags.NONE);

Big.BoxPackFlags leftover.

William Jon McCann wrote on 2009-11-06 19:39:41 UTC:
>The places area spacing is inconsistent with the recent items area.  It should
>be the same.  In particular, the vertical spacing between rows.

Having different vertical spacings is beautiful also - it looks like many small items in the recent documents section, then less items but slightly bigger (due to larger vertical spacing) in the places section, and then on the top of that few big items in an application well. It seems like blossoming flower. :)

But, as I understand, we've lost that in a trunk version ? :(

Review of attachment 147513 [details] [review]:

The commit message for this patch still is badly insufficient. You are clearly making some sort of change to the sizes of the panes, but that is not described by "Constrain popup panes", and I'm not going to try and reverse engineer what was meant.

From IRC:

<walters>       owen: hm weird, basically i just said that workspacesHeight is really "dash content height" = "area for new more popup"

So, maybe this part of the patch has something to do with making the panes only extend vertically to the bottom of the workspaces? Commit message needs to say that.

Review of attachment 147510 [details] [review]:

Commit message isn't good enough; other than that, some small comments.

::: js/ui/dash.js
@@ +840,3 @@
         }
 
+        this._searchResultsSection.content.queue_relayout();

This absolutely needs some sort of "darned if I know what's going on (maybe it has something to do with ...?) but if I don't do this then .... happens" comment. it sounded like you didn't figure out the root cause.

::: js/ui/genericDisplay.js
@@ +334,3 @@
 GenericDisplay.prototype = {
+    _init : function(flags) {
+        let disableVScrolling = (flags & GenericDisplayFlags.DISABLE_VSCROLLING) > 0;

Why '> 0' ?

@@ -335,3 +340,3 @@
-        this._maxItemsPerPage = null;
-        this._list = new Shell.OverflowList({ spacing: 6.0,
-                                              item_height: ITEM_DISPLAY_HEIGHT });
+        if (disableVScrolling) {
+            this.actor = this._list = new Shell.OverflowList({ spacing: 6,
+                                                                 item_height: 50 });

You seem to leave the constant in the file but stop using it

@@ +471,3 @@
         this._filterReset();
         this._openDetailIndex = -1;
+        this.actor.get_vscroll_bar().get_adjustment().value = 0;

What happens is this is called in the ShellOverFlowList case?

@@ +618,3 @@
             this._setMatches(this._getSearchMatchedItems(false));
         }
+        this._list.get_children().forEach(Lang.bind(this, function (child) { this._list.remove_actor(child); }));

Can't you use 'this._list.remove_all()' in both cases?

(In reply to comment #69)

> ::: js/ui/dash.js
> @@ +840,3 @@
>          }
> 
> +        this._searchResultsSection.content.queue_relayout();
> 
> This absolutely needs some sort of "darned if I know what's going on (maybe it
> has something to do with ...?) but if I don't do this then .... happens"
> comment. it sounded like you didn't figure out the root cause.

No =/  I probably should, but I'd really like to spend the time soon redoing the search to avoid ShellOverflowList entirely.

Added a comment.

> ::: js/ui/genericDisplay.js
> @@ +334,3 @@
>  GenericDisplay.prototype = {
> +    _init : function(flags) {
> +        let disableVScrolling = (flags &
> GenericDisplayFlags.DISABLE_VSCROLLING) > 0;
> 
> Why '> 0' ?

I like to avoid using random integers as booleans basically.

Attachment 147506 [details] pushed as 057f0ef - [StBoxLayout] Add st_box_layout_get_n_children
Attachment 147507 [details] pushed as 8040ad6 - [StBoxLayout] Add missing _queue_relayout in _add_actor
Attachment 147508 [details] pushed as 263d738 - [StTooltip] Fix _get_preferred_width
Attachment 147509 [details] pushed as 1626e8c - [StTooltip CSS] Add some style
Attachment 147510 [details] pushed as d51384f - switch to scrolling GenericDisplay, remove menus from apps
Attachment 147511 [details] pushed as bc255a5 - [overview] Port pane to CSS
Attachment 147512 [details] pushed as 66cab3b - [genericDisplay+other] Port to CSS, style cleanups
Attachment 147513 [details] pushed as ce90dda - [overview] Constrain popup panes, dim workspaces when active
Attachment 147514 [details] pushed as bf68f9f - [dash] Port section container to CSS, lower spacing
Attachment 147515 [details] pushed as 426d7bc - [dash] Port search section headers to CSS
Attachment 147516 [details] pushed as 06cf6c5 - [placeDisplay] Port more to CSS, lower spacing
Attachment 147517 [details] pushed as 4014313 - [dash] Swap More for triangle