Created attachment 251753 [details] [review]
appDisplay: Fix from rebase

Created attachment 251754 [details] [review]
appDisplay, IconGrid: Added pagination

AppDisplay: Add max pages

AppDisplay: doesn't superpose dash and pagination indicator to allview

FrequentView: not overlap dash

AppDisplay: typos

AppfolderPopup: expand on x

AppDisplay: Add tweener for change pages

Created attachment 251755 [details] [review]
Added pages indicators

AllView: pagination indicator reworked

pagesindicator css: change spacing

PaginationIconIndicator: change icon name

AllView, indicator layout, paginationScrollView: fixed allocation cycle hidding actors of indicators

IndicatorLayout: fixed allocation

No more hacking to Indicator

before implementing with StBin

Indicator with Shell.GenericContainer

testing allocation issues with visual glitchs

yeah....Shell.GenericContainer works fine for skipping painting
actors...

fixed frequent apps css spacing calculation, since the box passed didn't
take into account the grid spacing in updateDisplaySize, which is the
widget which has the right padding

Makefile.am : Added indicators svg

Added good page indicators

Forgot the indicators

Fixed indicator layout growing

New pagination indicators

Created attachment 251756 [details] [review]
PaginationScrollView: Added pan action

PaginationScrollView: take into account velocity on paning

PagionationScrollView, allview: take into account velocity when panning

PAginationScrollView: time when changin more than 1 page is the same as 1

appDisplay:..We need the pan action commit before.. Fix click action outside folder pop up

Created attachment 251757 [details] [review]
IconGrid, AppDisplay: New implementation to allow pagination and collections work fine

IconGrid, AppDisplay: New implementation of pagination to make spacing adapting screen size and solve some bugs

Created attachment 251758 [details] [review]
AppDisplay: Start always at page 0

AppDisplay: Always start at page 0

AllView: start at page 0 on hidden overview!

Created attachment 251759 [details] [review]
AppDisplay: Make space on grid to fit collection when opening

Created attachment 251760 [details] [review]
Collections contained inside the parent view

Created attachment 251761 [details] [review]
Icons style changed

Created attachment 251762 [details] [review]
Adapt spacing and icons size to available screen size. That is a good start to responsive design

Created attachment 251763 [details] [review]
Calculation of responsive grid outside allocation

Created attachment 251812 [details] [review]
appDisplay, IconGrid: Added pagination

Created attachment 251813 [details] [review]
appDisplay, IconGrid: Added pagination

Review of attachment 251812 [details] [review]:

Only some quick high-level comments for now:

 - adding a new feature (pagination in iconGrid) and starting to make use of it (paginated app display) looks like a fairly obvious split
 - there are a lot of debugging left-overs in iconGrid (see comment below)

::: js/ui/appDisplay.js
@@ +35,3 @@
 const FOLDER_SUBICON_FRACTION = .4;
 
+const MAX_APPS_PAGES = 20;

Unused for now => belongs in another commit

@@ +39,3 @@
+//fraction of page height the finger or mouse must reach before
+//change page
+const PAGE_SWITCH_TRESHOLD = 0.2;

Dto.

::: js/ui/iconGrid.js
@@ +376,2 @@
     childrenInRow: function(rowWidth) {
+        return this._computeLayout(rowWidth)[0]

Looks like this function doesn't exist anymore? (It should though, instead of all the computeLayoutOld, computeLayoutOldOld, computeLayoutNew mess ...)

Review of attachment 251813 [details] [review]:

Ha! You already fixed the main point from the previous review by yourself :-)

Created attachment 251814 [details] [review]
appDisplay, IconGrid: Added pagination

ei! I'm jut testing =) ! (althoguth the last patch is the good for me)

oh yeah, the two previous comments also apply now, just a moment

Created attachment 251815 [details] [review]
appDisplay, IconGrid: Added pagination

Created attachment 251828 [details] [review]
appDisplay, IconGrid: Added pagination

Created attachment 251844 [details] [review]
Added pages indicators

Created attachment 251868 [details] [review]
Added pages indicators

Created attachment 251926 [details] [review]
PaginationScrollView: Added pan action

Created attachment 251928 [details] [review]
IconGrid, AppDisplay: New implementation of pagination to make spacing adapting screen size and solve some bugs

Created attachment 251929 [details] [review]
IconGrid, AppDisplay: New implementation of pagination to make spacing adapting screen size and solve some bugs

Created attachment 251933 [details] [review]
AppDisplay: Start always at page 0

AppDisplay: Always start at page 0

AllView: start at page 0 on hidden overview!

Created attachment 251935 [details] [review]
PaginationScrollView: Added pan action

Created attachment 251945 [details] [review]
IconGrid, AppDisplay: New implementation of pagination to make spacing adapting screen size and solve some bugs

Created attachment 251949 [details] [review]
AppDisplay: Start always at page 0

Created attachment 251976 [details] [review]
AppDisplay: Make space on grid to fit collection when opening

Created attachment 251977 [details] [review]
AppDisplay: Make space on grid to fit collection when opening

Created attachment 251981 [details] [review]
AppDisplay: Make space on grid to fit collection when opening

Created attachment 251984 [details] [review]
Collections contained inside the parent view

Created attachment 251988 [details] [review]
Icons style changed

Created attachment 251993 [details] [review]
appDisplay: Fix from rebase

Created attachment 251994 [details] [review]
appDisplay, IconGrid: Added pagination

Created attachment 251995 [details] [review]
Added pages indicators

Created attachment 251996 [details] [review]
PaginationScrollView: Added pan action

Created attachment 251997 [details] [review]
IconGrid, AppDisplay: New implementation of pagination to make spacing adapting screen size and solve some bugs

Created attachment 251998 [details] [review]
AppDisplay: Start always at page 0

Created attachment 251999 [details] [review]
AppDisplay: Make space on grid to fit collection when opening

Created attachment 252000 [details] [review]
Collections contained inside the parent view

Created attachment 252001 [details] [review]
Icons style changed

Created attachment 252002 [details] [review]
Adapt spacing and icons size to available screen size. That is a good start to responsive design

Created attachment 252003 [details] [review]
Calculation of responsive grid outside allocation

Review of attachment 251993 [details] [review]:

No. There is nothing wrong with this code at the point of this patch - this.actor is an St.BoxLayout, which has a method add() to add  children. This change is only needed when this.actor is changed to an St.Widget at a later point, so that's where this change belongs (squashed, not as a separate patch, and in particular not with a generic non-descriptive commit message)

On a general note, *all* commit messages need work - having only a subject outlining the change without a body which explains a particular change in more detail is only acceptable for trivial/obvious patches, which is clearly not the case here.
(Also minor style issue: Subject lines use present tense, e.g. "Add pagination" rather than "Added pagination")

Review of attachment 251994 [details] [review]:

I still think it is a good idea to split this up further, e.g. first add the feature to IconGrid (or add a new PaginatedIconGrid class), then make use of it in AppDisplay. It's also not quite clear why you implement pagination this way, just to change it in a later patch - it's probably not worth to keep this in the history. Another major concern is the odd object hierarchy in AppDisplay, see a lengthy comment inline. Other than that, a lot of style nits (probably more than the ones indicated) ...

::: data/theme/gnome-shell.css
@@ +903,3 @@
 .frequent-apps > StBoxLayout {
     /* horizontal padding to make sure scrollbars or dash don't overlap content */
+    padding: 8px 88px;

Why the top/bottom padding? Should be a separate patch.

::: js/ui/appDisplay.js
@@ -114,3 @@
 const FolderView = new Lang.Class({
     Name: 'FolderView',
-    Extends: AlphabeticalView,

Why? In particular as you add it back at a later point ...

@@ +193,3 @@
 });
 
+const AppPages = new Lang.Class({

The new hierarchy is a bit confusing:
 - there's AllView, just like now
   (except that it accesses private properties of
    children left and right=
 - there's PaginationScrollView, which mostly intercepts
   the size negotiation mechanism to set the page size before
   the actual child allocation (but then does other stuff as well,
   including more accesses of private properties and odd calls into
   the parent)
 - there's AppPages, which ... uhm?
   All the new stuff either delegates to the child or to the parent

I think a better split is:
 - keep the implementation in AllView
 - add AllViewActor (or PaginationScrollView for what it's worth) to
   replace the St.ScrollView in AllView, which does nothing but
   providing the required hook into size negotiation/allocation

This should get rid of all those nasty little hierarchy violations, where you either access private properties of some child, or delegate to a parent because that's where the implementation happened to end up.

The main issue then is to provide the required hook into alloction - the obvious solution would be to emit a signal before allocating the child. While it is possible to define "proper" signals in GObject subclasses, it might be worth considering adapting Cosimo's gnome-documents trick instead (https://git.gnome.org/browse/gnome-documents/tree/src/utils.js#n115)

@@ +230,3 @@
 
+    addItem: function(item) {
+        this._addItem(item);

Side note: this code change is responsible for the regression noted at Guadec, that the view doesn't follow the focus when keynaving through pages ...

@@ +242,3 @@
+
+    setGridParentSize: function(size) {
+        this._grid._parentSize = size;

No. Reading another object's private properties is a clear violation of hierarchy, writing it is a definitive no-go.

@@ +285,3 @@
+    },
+
+    vfunc_get_preferred_height: function (container, forWidht) {

Typo: forWidth

@@ +301,3 @@
+        let childBox = new Clutter.ActorBox();
+        // Get the boxLayout inside scrollView
+        let child = this.get_children()[2];

Ugh. That's an implementation detail of StScrollView which could change at any time. Use this._box.

@@ +346,3 @@
                 this._updateIconOpacities(isOpen);
                 if (isOpen) {
+                    this._pages._grid.actor.y = popup.parentOffset;

Nope, _grid is private to this._pages.

@@ +349,2 @@
                 } else {
+                    this._pages._grid.actor.y = 0;

Dto.

@@ +395,3 @@
+    },
+
+    _onPan: function(action) {

This function does nothing and isn't called by anything, so why is it there in the first place?

@@ +400,3 @@
+
+    addApp: function(app) {
+       let appIcon = this._paginationView._pages.addItem(app);

Nope, see above.

@@ +404,3 @@
+
+    addFolder: function(dir) {
+        let folderIcon = this._paginationView._pages.addItem(dir);

Dto.

@@ +412,3 @@
+
+    removeAll: function() {
+        this._paginationView._pages.removeAll();

Dto.

@@ +416,3 @@
+
+    loadGrid: function() {
+        this._paginationView._pages.loadGrid();

Dto.

@@ +791,3 @@
                                                        x_fill: true,
                                                        y_fill: true,
+                                                       x_expand: true,

Looks unrelated - should this be a separate patch?

::: js/ui/iconGrid.js
@@ +179,3 @@
                                         fillParent: false,
+                                        xAlign: St.Align.MIDDLE,
+                                        usePagination: false });

I'm still not quite convinced that a parameter is better than a new PaginatedIconGrid subclass - _getPreferredHeight() does completely different things based on the parameter, and to a large extent the same is true for _allocate(); also there are API additions to IconGrid that don't make any sense when not using pagination (nPages(), getPagePosition())

@@ +188,1 @@
+        if(this._usePagination) {

Style nit: space before '(' (throughout the patch)

@@ +258,3 @@
         let height = nRows * this._vItemSize + totalSpacing;
+        if(this._usePagination) {
+            this._spacePerRow = this._vItemSize + spacing;

let spacePerRow = ...

@@ +274,2 @@
     _allocate: function (grid, box, flags) {
+        if(this._fillParent) {

Unrelated (and wrong) whitespace change

@@ +286,2 @@
         let [nColumns, usedWidth, spacing] = this._computeLayout(availWidth);
+        if(this._usePagination) {

Apart from the usual missing space, we also omit braces for one-line blocks, but the comment makes it enough of a gray area to add them anyway here IMO ...

@@ +309,1 @@
+        if(children.length > 0) {

Nit: no braces (the special case here could be avoided by doing something like

  if (columnIndex == 0 && this._usePagination)
      this._firstPagesItems.push(children[i]);

early in the loop; but then having two (columnIndex == 0) blocks in the same loop might be more confusing than the loss of clarity the special case adds)

Maybe the clearest thing to do is to get rid of the property altogether, see comment below ...

@@ +335,3 @@
+                if(this._usePagination) {
+                    if((i + 1) % this._childrenPerPage == 0) {
+                        y+= this._spaceBetweenPages;

Space around operators!

@@ +336,3 @@
+                    if((i + 1) % this._childrenPerPage == 0) {
+                        y+= this._spaceBetweenPages;
+                        if(i < children.length) {

No braces.

@@ +348,3 @@
     },
 
+    _calculateChildrenBox: function(child, x, y) {

Nit: the name should be _calculateChildBox() (or _calculateChildrenBoxes() if that was what it actually did)

Could be in a separate commit (to make it more obvious that it's just moving code around)

@@ +350,3 @@
+    _calculateChildrenBox: function(child, x, y) {
+        let [childMinWidth, childMinHeight, childNaturalWidth, childNaturalHeight]
+        = child.get_preferred_size();

Odd spacing - I'd break the line after the '=' and indent the follow-up.

@@ +373,2 @@
     childrenInRow: function(rowWidth) {
+        return this._computeLayout(rowWidth)[0]

Unintended change.

@@ +379,3 @@
     },
 
+    _computeLayout: function (forWidth, forHeight) {

forHeight looks unused in this function, no callers have been adjusted, and the additional parameter is removed again in a later patch - you know what to do :-)

@@ +388,3 @@
             let emptyArea = forWidth - itemWidth;
             spacing = Math.max(this._spacing, emptyArea / (2 * this._colLimit));
+            // We have to care that new spacing must not change number of rows per page.

The comment is a bit misleading - the spacing does of course affect the number of rows per page, but we don't want it to differ here from the calculations in _getPreferredHeight()

@@ +391,3 @@
+            if(this._usePagination) {
+                let spaceBetweenPages = this._parentSize[1] - (this._rowsPerPage * (this._vItemSize + spacing));
+                if(spaceBetweenPages < 0) {

No braces.

@@ +443,3 @@
+    getPagePosition: function(pageNumber) {
+        if(!this._nPages)
+            return;

You should still return something to avoid a "function foo doesn't always return a value" warning

@@ +449,3 @@
+            throw new Error('Invalid page number ' + pageNumber);
+        }
+        let childBox = this._firstPagesItems[pageNumber].get_allocation_box();

Is there any real benefit in keeping track of first-items-per-page (and recreating it on every allocation change) rather than just using

  let childBox = this._getVisibleChildren()[pageNumber * this._childrenPerPage].get_allocation_box();

here?

how is this looking ?
we're coming down to the wire for 3.9.90.
Ideally, I'd need to have stuff tomorrow

Created attachment 252907 [details] [review]
iconGrid: Split out _calculateChildBox

Split out the calculation of the child box in allocation function
to be reusable by subclasses and let the code more modular

Created attachment 252908 [details] [review]
iconGrid: Move spacing calculation to its own function

Move spacing calculation to a function, which allow
to be reusable and rewritable by subclasses

Created attachment 252909 [details] [review]
iconGrid: Add PaginatedIconGrid and change IconGrid for allow us to use pagination

Add a subclass of IconGrid with proper functions to work with
pagination.
Also, we change the implementation of IconGrid to use the dynamic
spacing calculated before allocation of parents which allow us
to use the PaginationIconGrid, since we need to know before
the PaginatedIconGrid allocation the parent size to know how many
pages we need to allocate all the apps items, and therefore, how many
height we need for the PaginatedIconGrid.

Created attachment 252910 [details] [review]
iconGrid, searchDisplay: Refactor childrenInRow to columnsForWidth

Since the parameter of the function is the width, reflect that in
the function name. Also, since we are counting columns, not only
children for each row, reflect that in the function name also.

Created attachment 252911 [details] [review]
appDisplay: Move FolderView near FolderIcon for better context

Since FolderView are closely related with FolderIcon, we
have more context while working if FolderView is near
FolderIcon

Created attachment 252912 [details] [review]
appDisplay: Animate _updateIconOpacities

Animate the transition between full opacity and partly opacity
to follow overall animations design of gnome-shell

Created attachment 252913 [details] [review]
appDisplay: Use new IconGrid implementation in AllView and FrequentView

Add "usePagination" parameter to Alphabetical view to allow AllView
use the new PaginatedIconGrid.
FrequentView adapts the new IconGrid implementation.
Create a PagesView class to allow us control the allocation of
the IconGrid child and the scroll adjustment.
Hook up the allocation/size comunication between the FrequentView,
AllView and FolderView with AppDisplay with a ViewStackLayout that
updates all views.

Created attachment 252914 [details] [review]
gnome-shell.css: Change app picker bottom padding

Increase the bottom padding between the views and the control buttons
of the AppDisplay to be more eye pleasant

Created attachment 252915 [details] [review]
appDisplay: Add pages indicators

Add indicators to the pagination in AllView, which displays
how many pages of apps we have and allows the user to
navigate through them

Created attachment 252916 [details] [review]
appDisplay: Add and rework pan action response

Add pan action to AllView and rework it to take
into account the velocity the user gives to the action

Created attachment 252917 [details] [review]
iconGrid, appDisplay: Use surrounding spacing on AllView and FrequentView

Add functions in iconGrid and adapt the main views to use surrounding
spacing, which will allows in the next patch makes the folder view,
with its new implementation, be contained inside the main views without
cutting off the boxpointer or the close button.

Created attachment 252918 [details] [review]
boxPointer, appDisplay: Add functions to query popup and boxpointer offsets

It will be necesary in the upcomming patch with the new implementation
of the folder view, to maintain the view contained without cut off
the popup or the close button.

Created attachment 252919 [details] [review]
appDisplay: FolderView and FolderIcon new implementation

The popup of the FolderView is now contained inside
the parent view, solving the overflow of apps with a ScrollView.
Also, solved a lot of bugs in popup/FolderView calculation
of position and size.

Created attachment 252920 [details] [review]
appDisplay: Start always at scroll 0 on FolderView

Reset the scroll adjustment between popups opennings,
following the same design we want to the AllView

Created attachment 252921 [details] [review]
appDisplay: Start always at page 0 in AllView

Reset the AllView scroll adjustment between overview openings,
following design reasons

Created attachment 252922 [details] [review]
appDisplay: Make space on grid to fit collection when opening

Add properly functions in AllView and FolderView to animate
the FolderView popup opening and give enough space on the parent
view to fit the popup of the folder view

Created attachment 252923 [details] [review]
gnome-shell.css: Change icons style to follow new design

Change the background, glow and labels of the Dash and AppDisplay
to follow the new design

Created attachment 252924 [details] [review]
appDisplay, iconGrid, searchDisplay: Adapt spacing and icons size to available screen size.

Add a convenient function in IconGrid to calculate the responsive grid
before any view allocation.
This function use dynamic paddings on the views and scale the icons
if the size given to the IconGrid is too small to fit a minimum of
icons rows/columns

Created attachment 252986 [details] [review]
iconGrid: Split out _calculateChildBox

Split out the calculation of the child box in allocation function
to be reusable by subclasses and let the code more modular

Created attachment 252987 [details] [review]
iconGrid: Move spacing calculation to its own function

Move spacing calculation to a function, which allow
to be reusable and rewritable by subclasses

Created attachment 252988 [details] [review]
iconGrid: Add PaginatedIconGrid and change IconGrid for allow us to use pagination

Add a subclass of IconGrid with proper functions to work with
pagination.
Also, we change the implementation of IconGrid to use the dynamic
spacing calculated before allocation of parents which allow us
to use the PaginationIconGrid, since we need to know before
the PaginatedIconGrid allocation the parent size to know how many
pages we need to allocate all the apps items, and therefore, how many
height we need for the PaginatedIconGrid.

Created attachment 252989 [details] [review]
iconGrid, searchDisplay: Refactor childrenInRow to columnsForWidth

Since the parameter of the function is the width, reflect that in
the function name. Also, since we are counting columns, not only
children for each row, reflect that in the function name also.

Created attachment 252990 [details] [review]
appDisplay: Move FolderView near FolderIcon for better context

Since FolderView are closely related with FolderIcon, we
have more context while working if FolderView is near
FolderIcon

Created attachment 252991 [details] [review]
appDisplay: Animate _updateIconOpacities

Animate the transition between full opacity and partly opacity
to follow overall animations design of gnome-shell

Created attachment 252992 [details] [review]
appDisplay: Use new IconGrid implementation in AllView and FrequentView

Add "usePagination" parameter to Alphabetical view to allow AllView
use the new PaginatedIconGrid.
FrequentView adapts the new IconGrid implementation.
Create a PagesView class to allow us control the allocation of
the IconGrid child and the scroll adjustment.
Hook up the allocation/size comunication between the FrequentView,
AllView and FolderView with AppDisplay with a ViewStackLayout that
updates all views.

Created attachment 252993 [details] [review]
appDisplay: Makes key navigation works with pagination

We have to rewrite the ensureIconVisibility function
to change pages when we reach an item inside another
page than the current

Created attachment 252994 [details] [review]
gnome-shell.css: Change app picker bottom padding

Increase the bottom padding between the views and the control buttons
of the AppDisplay to be more eye pleasant

Created attachment 252995 [details] [review]
appDisplay: Add pages indicators

Add indicators to the pagination in AllView, which displays
how many pages of apps we have and allows the user to
navigate through them

Created attachment 252996 [details] [review]
appDisplay: Add and rework pan action response

Add pan action to AllView and rework it to take
into account the velocity the user gives to the action

Created attachment 252997 [details] [review]
iconGrid, appDisplay: Use surrounding spacing on AllView and FrequentView

Add functions in iconGrid and adapt the main views to use surrounding
spacing, which will allows in the next patch makes the folder view,
with its new implementation, be contained inside the main views without
cutting off the boxpointer or the close button.

Created attachment 252998 [details] [review]
boxPointer, appDisplay: Add functions to query popup and boxpointer offsets

It will be necesary in the upcomming patch with the new implementation
of the folder view, to maintain the view contained without cut off
the popup or the close button.

Created attachment 252999 [details] [review]
appDisplay: FolderView and FolderIcon new implementation

The popup of the FolderView is now contained inside
the parent view, solving the overflow of apps with a ScrollView.
Also, solved a lot of bugs in popup/FolderView calculation
of position and size.

Created attachment 253000 [details] [review]
appDisplay: Start always at scroll 0 on FolderView

Reset the scroll adjustment between popups opennings,
following the same design we want to the AllView

Created attachment 253001 [details] [review]
appDisplay: Start always at page 0 in AllView

Reset the AllView scroll adjustment between overview openings,
following design reasons

Created attachment 253002 [details] [review]
appDisplay: Make space on grid to fit collection when opening

Add properly functions in AllView and FolderView to animate
the FolderView popup opening and give enough space on the parent
view to fit the popup of the folder view

Created attachment 253003 [details] [review]
appDisplay: Add pan action to FolderView

Since we have now a ScrollView in the FolderView,
add support for the pan action

Created attachment 253004 [details] [review]
gnome-shell.css: Change icons style to follow new design

Change the background, glow and labels of the Dash and AppDisplay
to follow the new design

Created attachment 253005 [details] [review]
appDisplay, iconGrid, searchDisplay: Adapt spacing and icons size to available screen size.

Add a convenient function in IconGrid to calculate the responsive grid
before any view allocation.
This function use dynamic paddings on the views and scale the icons
if the size given to the IconGrid is too small to fit a minimum of
icons rows/columns

Review of attachment 252991 [details] [review]:

Since the only param changing is the opacity, I'd just have "opacity" as a local and use that when constructing the params in tweener.

Review of attachment 252995 [details] [review]:

::: data/Makefile.am
@@ +42,3 @@
 	theme/more-results.svg			\
 	theme/noise-texture.png			\
+	theme/page-indicator-active.svg	\

indentation needs to be aligned.

::: js/ui/appDisplay.js
@@ +166,3 @@
+    
+    _init: function(parent, index) {
+

no whitespace

@@ +168,3 @@
+
+        this.actor = new St.Button({ style_class: 'pages-icon-indicator',
+                                     button_mask: St.ButtonMask.ONE || St.ButtonMask.TWO,

this is wrong, needs to be a binary or

(and why do you allow left and middle click, but not right click?)

@@ +173,3 @@
+        this.actor.connect('clicked', Lang.bind(this, this._onClicked));
+        this.actor._delegate = this;
+        this._parent = parent;

use signals instead of methods on the parent.

@@ +174,3 @@
+        this.actor._delegate = this;
+        this._parent = parent;
+        this.actor._index = index;

yuck. Just use this._index.

@@ +192,3 @@
+    _init: function(params) {
+        params['y_expand'] = true;
+        params['x_expand'] = true;

ugh. just pass a constant set of params. you don't seem to construct PaginationIndicator anywhere else.

@@ +225,3 @@
+        let children = this.actor.get_children();
+        for (let i in children)
+            this.actor.set_skip_paint(children[i], true);

please do not use set_skip_paint. this is on the chopping block for removal.

@@ +241,3 @@
+            childBox.y1 = i * heightPerChild;
+            childBox.y2 = childBox.y1 + heightPerChild;
+            // We currently threat the overflow not painting more indicators

"threat"?

@@ +271,3 @@
+                                                              y_align: Clutter.ActorAlign.CENTER });
+        this.actor.add_actor(this._paginationIndicator.actor);
+        for (let i = 0; i < MAX_APPS_PAGES; i++) {

hm. i'm not a fan of adding the maximum number of indicators and then only painting some of them. i'd actually do this more as one actor which draws a bunch of circles on a drawing area. should be a lot easier to manage.

that will also get rid of all the icky allocation problems and will help us get pagination indicator animations that match the mockup.

@@ +274,3 @@
+            let indicatorIcon = new PaginationIconIndicator(this, i);
+            if (i == 0)
+                indicatorIcon.setChecked(true);

this should happen when we go to page when we first display the indicator.

@@ +314,2 @@
     _onNPagesChanged: function(iconGrid, nPages) {
+        this._paginationIndicator._nPages = nPages;

you can't do this. figure out something else to do.

@@ +324,3 @@
+
+    indicatorsGoToPage: function(pageNumber) {
+        // Since it can happens after a relayout, we have to ensure that all is unchecked

???

explain this a bit more.

@@ +325,3 @@
+    indicatorsGoToPage: function(pageNumber) {
+        // Since it can happens after a relayout, we have to ensure that all is unchecked
+        let indicators = this._paginationIndicator.actor.get_children();

why are you using get_children / set_checked instead of the public setChecked API you made?

also, put this inside the if statement

@@ +327,3 @@
+        let indicators = this._paginationIndicator.actor.get_children();
+        if (this._grid.nPages() > 1) {
+            for (let index in indicators)

don't use for...in.

I'm reviewing the same patch series locally - should I stop doing this?

Review of attachment 252996 [details] [review]:

i'll have to look this over later

::: js/ui/appDisplay.js
@@ +40,3 @@
 const MAX_APPS_PAGES = 20;
 
+//fraction of page height the finger or mouse must reach before

needs to be capitalized and have proper whitespace.

@@ +300,3 @@
+        let panAction = new Clutter.PanAction({ interpolate: false });
+        panAction.connect('pan', Lang.bind(this, this._onPan));
+        panAction.connect('gesture-cancel', Lang.bind(this, function() {

Lang.bind(this, this._onPanEnd)

@@ +303,3 @@
+            this._onPanEnd(this._panAction);
+        }));
+        panAction.connect('gesture-end', Lang.bind(this, function() {

Lang.bind(this, this._onPanEnd)

@@ +364,3 @@
+        // return smoothly to the current page using the default velocity
+        if (this._currentPage != pageNumber) {
+            let min_velocity = totalHeight / (PAGE_SWITCH_TIME * 1000);

minVelocity

@@ +378,3 @@
+                           time: time,
+                           transition: 'easeOutQuad' };
+            Tweener.addTween(this._verticalAdjustment, params);

just put the params inline here

Created attachment 253154 [details] [review]
appDisplay: Animate _updateIconOpacities

Animate the transition between full opacity and partly opacity
to follow overall animations design of gnome-shell

Review of attachment 253000 [details] [review]:

ok.

Review of attachment 253001 [details] [review]:

i think the behavior would be better if this was when showing the app grid (so connect to notify::mapped), but i'm fine with this.

Review of attachment 252998 [details] [review]:

squash these with the next patch then. minimal apis like these should be added when they're first used.

::: js/ui/appDisplay.js
@@ +1047,3 @@
+    },
+
+    getOffset: function (element, side) {

i'd prefer different apis rather than a string-based one.

@@ +1052,3 @@
+            offset = this.closeButton.get_theme_node().get_length('-shell-close-overlap-y');
+        else
+        if (element == 'arrowHeight')

freaky else if style.

Review of attachment 252997 [details] [review]:

i have no idea what "surrounding spacing" is. please explain more.

::: js/ui/iconGrid.js
@@ +194,3 @@
+        this.bottom_padding = 0;
+        this.right_padding = 0;
+        this.left_padding = 0;

these seem to only be set once and are equivalent to 'spacing'. just use that?

Review of attachment 252992 [details] [review]:

::: data/theme/gnome-shell.css
@@ +938,1 @@
 .frequent-apps > StBoxLayout {

why aren't we changing frequent-apps if this adapts that also

::: js/ui/appDisplay.js
@@ +71,3 @@
+        params = Params.parse(params, { usePagination: false });
+        
+        if(params['usePagination'])

params.usePagination

@@ +132,3 @@
+        params['reactive'] = true;
+        this.parent(params);
+        this._parent = parent;

ugh. again, use signals instead of keeping a parent pointer, especially when we have 'this.parent' and 'this._parent'.

@@ +137,2 @@
+    vfunc_get_preferred_height: function (forWidth) {
+        return [0, 0];

???

@@ +141,2 @@
+    vfunc_get_preferred_width: function(forHeight) {
+        return [0, 0];

???

@@ +145,3 @@
+    vfunc_allocate: function(box, flags) {
+        box = this.get_parent().allocation;
+        box = this.get_theme_node().get_content_box(box);

uh? why do you use the parent's allocation instead of the box you are given?

this seems super broken.

@@ +177,3 @@
+        this._box = new St.BoxLayout({ vertical: true });
+        this._verticalAdjustment = new St.Adjustment();
+        this._box.set_adjustments(new St.Adjustment() /* unused */, this._verticalAdjustment);

i think you can just pass null.

@@ +182,3 @@
         this._stack.add_actor(this._grid.actor);
         this._eventBlocker = new St.Widget({ x_expand: true, y_expand: true });
+        this._stack.add_actor(this._eventBlocker, {x_align:St.Align.MIDDLE});

bad whitespace.

@@ +202,3 @@
+        // When the number of pages change (i.e. when changing screen resolution)
+        // we have to tell pagination that the adjustment is not correct (since the allocated size of pagination changed)
+        // For that problem we return to the first page of pagination.

that seems like a bad solution. you should try to keep the page you are on.

@@ +351,3 @@
+        box.y2 = height;
+        box = this.actor.get_theme_node().get_content_box(box);
+        box = this._grid.actor.get_theme_node().get_content_box(box);

do you really need both of these lines?

it seems that this could all be solved by connecting to allocation-changed on the grid actor. which you could do entirely from inside icon grid.

@@ +444,3 @@
+        this._viewStack = new St.Widget({ x_expand: true, y_expand: true });
+        this._viewStackLayout = new ViewStackLayout();
+        this._viewStack.set_layout_manager(this._viewStackLayout);

just use layout_manager: new ViewStackLayout()

@@ +445,3 @@
+        this._viewStackLayout = new ViewStackLayout();
+        this._viewStack.set_layout_manager(this._viewStackLayout);
+        this._viewStackLayout.connect('allocated-size-changed', Lang.bind(this, this._onAllocateSizeChanged));

connect to allocation-changed on the actor. layout manager signals have given us trouble.

@@ +564,3 @@
+        let availHeight = box.y2 - box.y1;
+        for (let i = 0; i < this._views.length; i++) {
+            this._views[i].view.adaptToSize(availWidth, availHeight);

the overview has had bad experiences with "push-style" allocations before, so i'd prefer if we could integrate with clutter's natural sizing system as much as possible.

let's hope this doesn't screw us up.

::: js/ui/iconGrid.js
@@ -354,3 @@
-            spacing = Math.round(spacing);
-        }
-        this.setSpacing(spacing);

now why was this removed?

Review of attachment 253154 [details] [review]:

style nit, otherwise fine.

::: js/ui/appDisplay.js
@@ +259,3 @@
+                       time: INACTIVE_GRID_OPACITY_ANIMATION_TIME,
+                       transition: 'easeOutQuad' };
+            Tweener.addTween(this._items[id].actor, params);

just put these inline in the tweener call here.

Created attachment 253158 [details] [review]
appDisplay: Add and rework pan action response

Add pan action to AllView and rework it to take
into account the velocity the user gives to the action

Review of attachment 253158 [details] [review]:

Look good, except for some minor comments

::: js/ui/appDisplay.js
@@ +443,3 @@
+        if (diffCurrentPage > this._pagesView.height * PAGE_SWITCH_TRESHOLD) {
+            if (action.get_velocity(0)[2] > 0 && this._currentPage > 0)
+                this.goToPage(this._currentPage - 1, action);

The action parameter is a left-over from a previous iteration.

@@ +445,3 @@
+                this.goToPage(this._currentPage - 1, action);
+            else if (this._currentPage < this._grid.nPages() - 1)
+                     this.goToPage(this._currentPage + 1, action);

Dto.

@@ +447,3 @@
+                     this.goToPage(this._currentPage + 1, action);
+        } else {
+            this.goToPage(this._currentPage, action);

Dto.

@@ +450,3 @@
+        }
+        this._panning = false;
+        return false;

Neither 'gesture-end' nor 'gesture-cancel' have a return value.

Review of attachment 252986 [details] [review]:

In the commit message: The last past misses a verb ("be more modular") - not sure it's needed at all though.

Review of attachment 252987 [details] [review]:

::: js/ui/iconGrid.js
@@ +393,3 @@
+    },
+
+    _getSpacing: function() {

My idea of this was to have this function return the appropriate spacing for the class (e.g. including all the existing "if (this._colLimit) { ... }" stuff); if we go with the setSpacing()/_getSpacing() approach instead, I want something more explicit like the "setSizeManually" property in BaseIcon instead of an odd "use CSS until calling setSpacing() for the first time" API.

@@ +401,3 @@
+      to know how much spacing can we have at the grid
+     */
+    updateSpacingForSize: function(availWidth, availHeight) {

This belongs in a later patch?

Review of attachment 252988 [details] [review]:

The commit message still lies (IconGrid is not changed to use dynamic spacing yet) :-)

(Also overly long subject line, just "iconGrid: Add PaginatedIconGrid" is fine ...

::: js/ui/iconGrid.js
@@ +179,3 @@
                                         columnLimit: null,
+                                        minRows: 1,
+                                        minColumns: 1,

It is odd to have properties that are only used in a subclass. Also, as mentioned in an earlier review, it would be good to document what they do, as the name is a bit ambiguous. I'd still prefer this to be in a separate patch (not least because you then have a commit messages explaining them properly instead of sneaking them in here)

@@ +502,3 @@
+    },
+    /**
+    * Compute the pagination values. This functions is intended to be called

"is intended" or "must"?

@@ +526,3 @@
+        // Take into account when the number of pages changed (then the height of the entire grid changed for sure)
+        // and also when the spacing is changed, sure the hegiht per page changed and the entire grid height changes, althougt
+        // maybe the number of pages doesn't change

Some obvious typos apart, the comment is pretty unclear. It *tries* to explain why the 'n-pages-changed' signal is used for something not at all related to the number of pages. Either rename the signal, or better, use it according to the name (e.g. a way to update how many indicators are drawn, together with an active-page-changed signal to determine which indicator should be checked).
The hacky bit is that pages have to be computed before the actual allocation, which is why you need this function to be called from the parent. So the parent already tracks size changes to call this function, why abuse this signal to make the parent respond to size changes it's already tracking anyway?

@@ +540,3 @@
+    },
+
+    getPageYPosition: function(pageNumber) {

"getYPosition" sounds like "getWidthSize" - I'd go with just "getPageY"

Review of attachment 252989 [details] [review]:

"rename" sounds more correct than "refactor" to me. (Also, if you only prefix "iconGrid", the subject will not exceed the character limit)

Review of attachment 252990 [details] [review]:

s/are closely related with/is closely related to/

Not sure I consider the reasoning good enough to make git-blame harder to use, but no strong opinion.

Review of attachment 252992 [details] [review]:

::: js/ui/appDisplay.js
@@ +125,3 @@
 });
 
+const PagesView = new Lang.Class({

A short comment explaining why this actor is need would be rad.

@@ +131,3 @@
+    _init: function(parent, params) {
+        params['reactive'] = true;
+        this.parent(params);

Still not convinced that splitting params between caller and callee makes sense ...

@@ +132,3 @@
+        params['reactive'] = true;
+        this.parent(params);
+        this._parent = parent;

As mentioned earlier, 'this.parent' vs 'this._parent' vs 'this.get_parent()' all meaning different things is not acceptable, sorry.

@@ +146,3 @@
+        box = this.get_parent().allocation;
+        box = this.get_theme_node().get_content_box(box);
+        this.set_allocation(box, flags); 

This is wrong - the content_box adjustment should be made for content only (e.g. children), the actor itself should take the full allocation. Regarding the hack with taking the parent allocation: Can't you set the actor to expand+fill, so the passed-in box is already what you want? In fact, I suspect that you can make it work without overwriting allocate() at all ...

@@ +156,3 @@
+        let child = this.get_child();
+        child.allocate(childBox, flags);
+        this._parent.updateAdjustment(availHeight);

I'm pretty sure I complained about this in an earlier review. Doesn't

  this._box.connect('notify::allocation, Lang.bind(this, this._updateAdjustment));

work in AllView?

Otherwise a signal would be a better API (either a GObject signal, or using Cosimo's emitJS trick from documents)

@@ +168,3 @@
+        this._pagesView = new PagesView(this, { style_class: 'all-apps',
+                                                x_align: St.Align.MIDDLE,
+                                                y_align: St.Align.MIDDLE });

Is there any point in setting align values if the child will be allocated all available space anyway?

@@ +207,3 @@
 
+    _onNPagesChanged: function(iconGrid, nPages) {
+        this._paginationInvalidated = true;

This is still rather fiddly: We rely on 'n-pages-changed' to be emitted before the allocation is updated, and that the allocation actually changes when the number of pages changes (is this true when a page is added because a new application has been installed?)

Can't we just do something like

   // Switch to the first page, to make sure that our position is still valid
    this._verticalAdjustment.value = 0;

@@ +214,3 @@
+        let params = { value: this._grid.getPageYPosition(this._currentPage),
+                       time: PAGE_SWITCH_TIME,
+                       transition: 'easeOutQuad' };

The extra params variable doesn't really add anything ...

@@ +224,3 @@
+            if (this._currentPage > 0) {
+                nextPage = this._currentPage - 1;
+                this.goToPage(nextPage);

Using nextPage for the previous page is a bit odd - I'd just write
  this.goToPage(this._currentPage - 1);
directly (same below) ...

@@ +227,3 @@
+            }
+        }
+        if (direction == Clutter.ScrollDirection.DOWN) {

else if - the direction can't be both UP and DOWN, so no need to evaluate this condition when we already handled UP.

@@ +323,3 @@
 const FrequentView = new Lang.Class({
     Name: 'FrequentView',
+    Extends: AlphabeticalView,

Oh, I'm sorry, but - can you do a cleanup patch first that renames AlphabeticalView? The name implies that it sorts items alphabetically (and indeed it used to enforce pre-3.8 by inserting items at the right position; when landing the folder stuff, we decided this was unnecessarily costly compared to just sorting items upfront before insertion). So it is confusing to use 'AlphabeticalView' in a view that uses a different sort order ...
(Something like BaseView, BaseAppView, ...)

@@ +395,3 @@
+        let availWidth = box.x2 - box.x1;
+        let availHeight = box.y2 - box.y1;
+        // Prepare children of all views for the upcomming allocation, calculate all

*upcoming

@@ +445,3 @@
+        this._viewStackLayout = new ViewStackLayout();
+        this._viewStack.set_layout_manager(this._viewStackLayout);
+        this._viewStackLayout.connect('allocated-size-changed', Lang.bind(this, this._onAllocateSizeChanged));

*_onAllocatedSizeChanged

Review of attachment 252993 [details] [review]:

Is it possible to squash this with the patch that breaks the functionality in the first place? (Not a big deal if not ...)

::: js/ui/iconGrid.js
@@ +547,3 @@
+    getItemPage: function(item) {
+        let found = false;
+        let children = this._getVisibleChildren();

Do we actually need the loop, or can we do:

let index = children.indexOf(item);
if (index == -1) {
    throw new Error('Item not found.');
    return 0;
}
return Math.floor(index / this._childrenPerPage);

Review of attachment 252994 [details] [review]:

The canonical prefix for changes to gnome-shell.css is 'theme:', otherwise good.

Review of attachment 252995 [details] [review]:

Has this patch actually changed since my last review?

Review of attachment 253004 [details] [review]:

This is fairly noisy in the stylesheet - maybe use an additional class, so that you don't have to add overview-icon-with-label all over the place?

Something like:

let styleClass = 'overview-icon';
if (params.showLabel)
    styleClass += ' overview-icon-with-label';
this.actor = new St.Bin({ style_class: styleClass });

in BaseIcon, then just use

FooBar > .overview-icon.overview-icon-with-label {
    padding: 10px 8px 5px 8px;
}

in the CSS.

::: data/theme/gnome-shell.css
@@ +998,2 @@
     border-radius: 4px;
+    /* visually, since the label control its own espacing inside,

Nice Spanglish! *spacing

@@ +998,3 @@
     border-radius: 4px;
+    /* visually, since the label control its own espacing inside,
+       seems more visual consistent to have diferent padding for

*different

@@ +999,3 @@
+    /* visually, since the label control its own espacing inside,
+       seems more visual consistent to have diferent padding for
+       top and bottom */

The comment refers to the label, but the style here is used by items with and without labels; I'd probably reverse this, e.g. use the simple padding here and add an extra style for the -with-label variants (with the comment obviously).

@@ +1010,3 @@
+.show-apps > .overview-icon,
+.grid-search-result .overview-icon {
+	border-radius: 4px;

Indent! (Also: why set the border-radius at all here? It's already set above, no?)

::: js/ui/iconGrid.js
@@ +19,3 @@
                                         setSizeManually: false,
                                         showLabel: true });
+        let binParams = { style_class: 'overview-icon',

I'd probably just write this as

let styleClass = params.showLabel ? 'overview-icon-with-label'
                                  : 'overview-icon';
this.actor = new St.Bin({ style_class: styleClass, ... });

Review of attachment 253000 [details] [review]:

Some errors in the commit message (openings, for the AllView), otherwise fine.

Review of attachment 253001 [details] [review]:

I'd probably say "Always open with the first page" instead of "start at page 0", but sure ...

Review of attachment 253003 [details] [review]:

Untested on my side, but looks good to me.

Review of attachment 252997 [details] [review]:

::: js/ui/appDisplay.js
@@ +74,3 @@
                                                 minColumns: MIN_COLUMNS,
+                                                fillParent: false,
+                                                useSurroundingSpacing: false });

Mmmh, why use a default of 'true' in IconGrid, overwrite it with 'false' here, and then use 'true' again for all subclasses?

::: js/ui/iconGrid.js
@@ +182,3 @@
                                         fillParent: false,
+                                        xAlign: St.Align.MIDDLE,
+                                        useSurroundingSpacing: true });

Doesn't this change SearchDisplay?

@@ +189,3 @@
         this._xAlign = params.xAlign;
         this._fillParent = params.fillParent;
+        this._useSurroundingSpacing = params.useSurroundingSpacing;

Not sure if this is a naming problem or just needs a comment, but it's not immediately obvious what this property does; the name suggests that it will make use of extra space, but it's actually distributing the area available for spacing differently. useDynamicPadding maybe?

@@ +191,3 @@
+        this._useSurroundingSpacing = params.useSurroundingSpacing;
+
+        this.top_padding = 0;

camelCase for JS properties.

Review of attachment 252998 [details] [review]:

::: js/ui/appDisplay.js
@@ +1047,3 @@
+    },
+
+    getOffset: function (element, side) {

Odd API, better as separate functions IMHO. Or: Do you actually need the individual elements? Or can this be something like
  getOffset: function(side) {
    let offset = this._boxPointer.getPadding(side);
    if (side == St.Side.TOP)
      offset -= this.closeButton.get_theme_node().get_length('-shell-close-overlap-y');
    if (side == this._arrowSide)
      offset += this._boxPointer.getArrowHeight();
    return offset;
  }

Created attachment 253289 [details] [review]
appDisplay: Add and rework pan action response

Add pan action to AllView and rework it to take
into account the velocity the user gives to the action

Created attachment 253290 [details] [review]
iconGrid: Split out _calculateChildBox

Split out the calculation of the child box in allocation function
to be reusable by subclasses and let the code be more modular

Created attachment 253294 [details] [review]
iconGrid: Move spacing calculation to its own function

Move spacing calculation to a function, which allow
to be reusable and rewritable by subclasses

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

Created attachment 253367 [details] [review]
iconGrid: Add PaginatedIconGrid

The new PaginatedIconGrid class acts as a container for pages.
So the new class provides  the container behaviour and some
useful functions like positions of pages, number of pages, etc.
But, it doesn't add indicators of the pages and doesn't manage
the scroll of the pages, neither any management of the pages
like in which page currently it is, etc.

Created attachment 253373 [details] [review]
iconGrid: Rename childrenInRow to columnsForWidth

Since the parameter of the function is the width, reflect that in
the function name. Also, since we are counting columns, not only
children for each row, reflect that in the function name also.

Created attachment 253406 [details] [review]
iconGrid: Use minimum rows/columns properties to adjust spacing

Use properties to allow iconGrid satisfy the rows/columns
minima properties reducing spacing acordingly.

Created attachment 253407 [details] [review]
appDisplay: Move FolderView near FolderIcon for better context

Since FolderView is closely related with FolderIcon, we
have more context while working if FolderView is near
FolderIcon

Review of attachment 253290 [details] [review]:

ok

Review of attachment 253294 [details] [review]:

Minor nit in the commit message:
"allow to be reusable" is wrong - "which makes it reusable and overwritable by subclasses" maybe?

::: js/ui/iconGrid.js
@@ +392,3 @@
+
+    /**
+     * This function is intended to use it before iconGrid allocation,

s/use it before/to be used before/

(Not sure the comment makes too much sense at this point, given that it's not how it's used right now ...)

Review of attachment 253367 [details] [review]:

::: js/ui/iconGrid.js
@@ +424,3 @@
+        if(!this._nPages) {
+            alloc.min_size = 0;
+            alloc.natural_size = 0;

Mmmh, why this special case? The calculation below will get you exactly this result for nPages == 0.

@@ +482,3 @@
+    /**
+    * Compute the pagination values. This function must be called
+    * before allocation of the grid.

Just an idea:
Instead of this comment, we could add something like:

  if (this._childrenPerPage == 0)
    log('computePages() must be called before allocate(); pagination will not work.');

in _allocate()?

@@ +516,3 @@
+            return 0;
+        if(pageNumber < 0  || pageNumber > this._nPages) {
+            throw new Error('Invalid page number ' + pageNumber + ' requested, ' + this._nPages + ' computed pages');

Mmh, can this be a problem if we don't catch it (hint: we don't)?

Review of attachment 253373 [details] [review]:

ok

Review of attachment 253406 [details] [review]:

I'm still not quite convinced by the property names (as they can be read as: always allocate space to fit at least that many rows/columns), but can't really come up with anything better ...

The commit message would be a could place to explain a bit better what the properties actually do :-)

::: js/ui/iconGrid.js
@@ +420,3 @@
+        // Limit spacing to the item size
+        maxSpacing = Math.min(maxSpacing, Math.min(this._vItemSize, this._hItemSize));
+        // The minimum spacing, regardless it doesn't achieve the row/columng minima,

"regardless of whether it satisfies"; also: column, not columng

Review of attachment 253407 [details] [review]:

ok

Review of attachment 253289 [details] [review]:

::: js/ui/appDisplay.js
@@ +389,3 @@
+            this._currentPage = pageNumber;
+            Tweener.addTween(this._verticalAdjustment,
+                             { value: this._grid.getPageYPosition(this._currentPage),

This function has been renamed in an earlier patch.

@@ +431,3 @@
+                this.goToPage(this._currentPage - 1);
+            else if (this._currentPage < this._grid.nPages() - 1)
+                     this.goToPage(this._currentPage + 1);

Wrong indentation.

Created attachment 253424 [details] [review]
appDisplay: Rename AlphabeticalView to BaseAppView

Since the items are not ordered in an alphabetical
way, just rename the class to be consistent.

Review of attachment 253424 [details] [review]:

lgtm

Created attachment 253429 [details] [review]
iconGrid: Move spacing calculation to its own function

Move spacing calculation to a function, which makes
it reusable and overwritable by subclasses

Created attachment 253431 [details] [review]
iconGrid: Add PaginatedIconGrid

The new PaginatedIconGrid class acts as a container for pages.
So the new class provides  the container behaviour and some
useful functions like positions of pages, number of pages, etc.
But, it doesn't add indicators of the pages and doesn't manage
the scroll of the pages, neither any management of the pages
like in which page currently it is, etc.

Created attachment 253432 [details] [review]
iconGrid: Use minimum rows/columns properties to adjust spacing

Use properties to allow iconGrid satisfy the rows/columns
minima properties reducing spacing acordingly.

Created attachment 253433 [details] [review]
iconGrid: Use minimum rows/columns properties to adjust spacing

Use properties to allow iconGrid satisfy the rows/columns
minima properties reducing spacing acordingly.
The properties are named minRow and minColumns,
so if you set the minRow to, for example 4,
the spacing of iconGrid will be reduced to fit
4 rows in the available space.

Created attachment 253435 [details] [review]
appDisplay: Add and rework pan action response

Add pan action to AllView and rework it to take
into account the velocity the user gives to the action

Created attachment 253437 [details] [review]
appDisplay: Change keyboard focus from FolderPopup to FolderView

Since now FolderView has its own widget, the focus has to
be moved to the widget inside the FolderView when the popup
opens. Currently the focus was moved to the FolderPopup
widget, which is wrong with the new implementation.

Created attachment 253449 [details] [review]
appDisplay: Use new IconGrid implementation in AllView and FrequentView

Add "usePagination" parameter to Alphabetical view to allow AllView
use the new PaginatedIconGrid.
FrequentView adapts the new IconGrid implementation. Create a
PagesView class to allow us control the allocation of the IconGrid
child and the scroll adjustment. Hook up the allocation/size comunication
between the FrequentView, AllView and FolderView with AppDisplay with
a ViewStackLayout that updates all views.

Created attachment 253451 [details] [review]
appDisplay: Use new IconGrid implementation in AllView and FrequentView

Add "usePagination" parameter to Alphabetical view to allow AllView
use the new PaginatedIconGrid.
FrequentView adapts the new IconGrid implementation. Create a
PagesView class to allow us control the allocation of the IconGrid
child and the scroll adjustment. Hook up the allocation/size comunication
between the FrequentView, AllView and FolderView with AppDisplay with
a ViewStackLayout that updates all views.

Review of attachment 252999 [details] [review]:

The commit message is a bit misleading - "new implementation" suggests a rewrite of the same functionality. How about something like: "appDisplay: Align collection grid with parent view"?

::: js/ui/appDisplay.js
@@ +441,3 @@
     _createItemIcon: function(item) {
         if (item instanceof Shell.App)
             return new AppIcon(item);

Missing braces.

@@ +446,3 @@
+            this._folderIcons.push(folderIcon);
+            return folderIcon;
+        } else

Dto.

Alternatively, you could leave the function alone and push the folder in addFolder() ...

@@ +540,3 @@
+        // Update folder views
+        for (let id in this._folderIcons) {
+            this._folderIcons[id].adaptToSize(availWidth, availHeight);

for (let i = 0; i < this._folderIcons; i++)
    this._folderIcons[i].adaptToSize();

@@ +862,3 @@
+        this.actor.set_policy(Gtk.PolicyType.NEVER, Gtk.PolicyType.AUTOMATIC);
+        this._box = new St.BoxLayout({ vertical: true, reactive: true });
+        this._widget = new St.Widget({ layout_manager: new Clutter.BinLayout() });

Could use a more descriptive name.

@@ +908,3 @@
+        this._parentAvailableWidth = width;
+        this._parentAvailableHeight = height;
+        // Update grid dinamyc spacing based on display width

*dynamic, but I don't think the comment really adds any value

@@ +916,3 @@
+                                    this._boxPointerOffsets['paddingTop'] +
+                                    this._boxPointerOffsets['paddingBottom'] +
+                                    this._boxPointerOffsets['closeButtonOverlap'];

Assuming this API is going to change, so not commenting further on this ...

@@ +922,3 @@
+        this._grid.bottom_padding -= offsetForEachSide;
+        this._grid.left_padding -= offsetForEachSide;
+        this._grid.right_padding -= offsetForEachSide;

Mmmh, this is awkward. You are relying on updateSpacingForSize() setting the padding properties to modify them afterwards here. We need to come up with something clearer here (it reads as if you were decreasing the padding more and more with each call to this function)

@@ +931,3 @@
+        pageBox.x2 = this._parentAvailableWidth;
+        pageBox.y2 = this._parentAvailableHeight;
+        return this.actor.get_theme_node().get_content_box(pageBox);

You could make this function

_getAvailablePageSize() {
    let box = new Clutter.ActorBox();
    box.x1 = box.y1 = 0;
    ...
    let contentBox = this.actor.get_theme_node().get_content_box(box);
    return [(contentBox.x2 - contentBox.x1) - 2 * this._offsetForEachSide.
            (contentBox.y2 - contentBox.y1) - 2 * this._offsetForEachSide];
}

@@ +941,3 @@
+        // to make the calculation of used width right
+        availWidthPerPage -= 2 * this._offsetForEachSide;
+        let maxUsedWidth = this._grid.usedWidth(availWidthPerPage);

Where does the max come from?

(I'd just do
  return this._grid.usedWidth();
which is perfectly understandable code - if the function is kept, see comment below)

@@ +946,3 @@
+
+    usedHeight: function() {
+        // Then calculate the real maxUsedHeight

// Then ...

is an odd way to start a comment when it's at the beginning of a function :-)

@@ +954,3 @@
+        let availHeightPerPage = box.y2 - box.y1;
+        let availWidthPerPage = box.x2 - box.x1;
+        // Since it is inside a boxpointer, take the rigth available size

Typo: right (same below)

@@ +958,3 @@
+        availHeightPerPage -= 2 * this._offsetForEachSide;
+
+        let maxRowsDisplayedAtOnce = this.maxRowsDisplayedAtOnce();

I don't think it's worth splitting this function out - if you discard everything you already do in here, you end up with two lines of code (one if you write it as
   let maxRows = this._grid.rowsForHeight(availHeightPerPage) - 1;
)

@@ +961,3 @@
+        let usedRows = this._grid.nUsedRows(availWidthPerPage);
+        usedRows = usedRows <= maxRowsDisplayedAtOnce ? usedRows : maxRowsDisplayedAtOnce;
+        return usedRows;

return Math.min(this._grid.nUsedRows(availWidthPerPage), maxRowsDisplayedAtOnce); ?

@@ +979,3 @@
+        // doesn't go outside boundary, so we have to take into account the
+        // arrow size, the padding of the boxpointer and the close button displacement
+        this._boxPointerOffsets = boxPointerOffsets;

You are assuming that the view is inside a popup, that popups have arrows, ...

Can we make this just setPaddingOffset() and pass the value for _offsetForEachSide directly?

@@ +1030,3 @@
+    },
+
+    _popUpGridWidth: function() {

Should be popup instead of popUp for consistency (if for whatever reason we keep this function, see comment below)

@@ +1050,3 @@
+        // Be careful, we don't take into account the top panel height etc,
+        // So maybe we put an arrow side "wrong", but anyway, the expanding of the folder view will
+        // do the required space and all will go fine, so not a big problem

I guess we could do better if we used

let spaceTop = this.actor.y - this._parentView.getCurrentPageY();
let spaceBottom = this._parentView.actor.height - (spaceTop + this.actor.height);

(as a bonus, get_tranformed_position() is not the cheapest function to call, so not having to call it has some benefit)

@@ +1055,3 @@
+    },
+
+    _updatePopUpSize: function() {

Popup

@@ +1083,3 @@
+         * Solution: ensure style of the grid just after we add it to the parent
+         * and before the calculation of the position.
+         */

This is not a gtk-doc style comment, so don't use /** */

The entire comment boils down to
  // StWidget delays style calculation until needed, make sure we use the correct values
which isn't that interesting either ...

@@ +1095,3 @@
+        this.view.adaptToSize(this._parentAvailableWidth, this._parentAvailableHeight);
+        /*
+         * Always make the grid (and therefore the boxpointer) to be the max

Comment should use //, but I don't see how it makes sense at this place (if at one point the function is modified, this comment will almost certainly not be updated)

@@ +1100,3 @@
+         * following a design decision.
+         */
+        this.view.actor.set_width(this._popUpGridWidth());

Wait - you are setting the width of the view to a value gathered from a function of view? Why can't this just be handled by the view internally then? It looks like you could just move this (and usedWidth etc.) into adaptToSize() ...
(Same applies to height obviously)

@@ +1105,3 @@
+         * view, so calculate the maximum rows the parent can have, and then substract one,
+         * then calculate the maxUsedHeigth and the current used height, if the needed height
+         * is more, strech to the maxUsedHeight

Same as above.

@@ +1115,3 @@
+            if (this._boxPointerArrowside == St.Side.BOTTOM) {
+                let closeButtonOffset = -this._popup.closeButton.translation_y;
+                // We have to use this function, since this._popup.actor.height not always return a logical value.

Anything less vague about why actor.height may not be the actual height? The rest of the comment is fairly uninteresting.

@@ +1123,3 @@
+                this._popup.parentOffset = yWithButton < 0 ? -yWithButton : 0;
+                this._popup.actor.y = Math.max(y, closeButtonOffset);
+                this._popup.actor.y = y

???

@@ +1125,3 @@
+                this._popup.actor.y = y
+            } else
+                this._popup.actor.y = this.actor.y + this.actor.height;

Missing braces

@@ +1253,3 @@
+        this._arrowSide = side;
+        this._boxPointer._arrowSide = side;
+        this._boxPointer._border.queue_repaint();

No. Either add API to BoxPointer to change the arrowSide parameter after construction, or recreate the popup when the arrow changes (is this something we expect to happen frequently? I can only think of app (de-)installation and display size changes ...)

::: js/ui/iconGrid.js
@@ +379,3 @@
     },
 
+    nUsedRows: function(forWidth) {

Why not just nRows?

@@ +391,3 @@
+            nRows = Math.ceil(children.length / nColumns);
+        else
+            nRows = 0;

Could be written more concise as

let nRows = (nColumns > 0) ? Math.ceil(...) : 0;

@@ +399,3 @@
+    rowsForHeight: function(forHeight) {
+        forHeight -= this.top_padding + this.bottom_padding;
+        let rowsPerPage = Math.floor((forHeight + this._getSpacing()) / this.rowHeight());

spacing is space between rows, not really belonging to either the row before or after it; this makes rowHeight() a bit awkward, in particular as a public function (I think I mentioned this before somewhere?)

@@ +400,3 @@
+        forHeight -= this.top_padding + this.bottom_padding;
+        let rowsPerPage = Math.floor((forHeight + this._getSpacing()) / this.rowHeight());
+        return rowsPerPage;

Odd variable name - the function returns the number of rows for a given height, which may or may not be a page (it's also an IconGrid method, which doesn't have any notion of pages)
Though the only purpose of this function seems to be to get a FolderView's parent view's rowsPerPage property, no?

Created attachment 253474 [details] [review]
theme: Change app picker bottom padding

Increase the bottom padding between the views and the control buttons
of the AppDisplay to be more eye pleasant

Created attachment 253506 [details] [review]
theme: Change icons style to follow new design

Change the background, glow and labels of the Dash and AppDisplay
to follow the new design

Created attachment 253532 [details] [review]
boxPointer, appDisplay: Add functions to query popup and boxpointer offsets

It will be necesary in the upcomming patch with the new implementation
of the folder view, to maintain the view contained without cut off
the popup or the close button.

Review of attachment 253002 [details] [review]:

OK, so here's another lengthy review:
High-level, I'd very much prefer if the bulk of the work could be done in PaginatedIconGrid, to not have to expose too much "random" API to AllView ("getHeightOfRowWithSpacingBelowOrAbove()") ...

Other than that, there's a bunch of suggestions of cleanups, none of which really affect the overall structure - so I hope it's not too bad.

::: js/ui/appDisplay.js
@@ +36,2 @@
 const INACTIVE_GRID_OPACITY = 77;
+const INACTIVE_GRID_OPACITY_ANIMATION_TIME = 0.40;

Might make sense to squash this with the previous patch that introduces this.

@@ +329,3 @@
         // For that problem we return to the first page of pagination.
         this._paginationInvalidated = false;
+        this._popupExpansionNeeded = true;

Odd default value (e.g. I'd only ever expect this to be true if there's an request to open a popup)

@@ +330,3 @@
         this._paginationInvalidated = false;
+        this._popupExpansionNeeded = true;
+        this.displayingPopup = false;

This could be private, no? Also the name sounds like "this._currentPopup && this._currentPopup.isOpen" - maybe this._expandedForPopup?

@@ +344,3 @@
+            this._currentPopup.popdown();
+        else if(this.displayingPopup && this._currentPopup)
+                return;

Wrong indentation in the else part; however I don't really like the mix of early return with normal control flow - can you write this as:

if (this._currentPage == pageNumber && this.displayingPopup && this._currentPopup)
    return;

if (this.displayingPopup && this._currentPopup)
    this._currentPopup.popdown();

@@ +404,3 @@
+     * @param folderNVisibleRowsAtOnce this parameter tell how many rows the folder view has, but,
+     * it is already constrained to be at maximum of main grid rows least one, to ensure we have
+     * enough space to show the folder view popup.

This should use gtk-doc syntax:

/**
 * makeSpaceForPopUp:
 * @iconActor:
 * ...
 * Pan view with items to make space for the folder view
 */

(if needed at all - I'm not sure the details about folderNVisibleRowsAtOnce belong here; as far as the function is concerned, it'll just make space for the passed number of rows ...)

@@ +406,3 @@
+     * enough space to show the folder view popup.
+     */
+    makeSpaceForPopUp: function(iconActor, side, folderNVisibleRowsAtOnce) {

I'd prefer something slightly more generic, as
  makeSpaceForPopup: function(fromY, side, nRows)

(see suggestion below for not calling a method on iconActor, at which point all you need is the position)

@@ +412,3 @@
+        let mainIconRowReached = false;
+        let isMainIconRow = false;
+        let rows = this._grid.pageRows(this._currentPage);

Here's my first high-level concern: The API on IconGrid is getting more and more arbitrary. We now have a public function to get a two-dimensional array of children for a particular page, but the much more generic getVisibleChildren() is still private :-(
(I can think of a way to use something like getPageChildren(this._currentPage) instead (e.g. no multidimensional arrays), but that's not much less specific)
This (and more so the use of rowHeight() below) makes me wish to have the bulk of code in PaginatedIconGrid instead (e.g. something like: this._grid.openSpaceForExtraRows(sourceActor, side, nRows, callback)).
But then, PaginatedIconGrid doesn't have a complete notion of pages, so not sure that's possible ...

@@ +413,3 @@
+        let isMainIconRow = false;
+        let rows = this._grid.pageRows(this._currentPage);
+        this._translatedRows = rows;

???

Can't you do
  this._translatedRows = rowsUp.concat(rowsDown);
at the end, to not include non-translated rows?

(or maybe
  this._translatedChildren = rowsUp.concat(rowsDown).reduce(function(prev, cur) { return prev.concat(cur); });
to get a flat array, so you can iterate it in a simple loop)

As an added bonus, this would make the _popupExpansionNeeded property unnecessary.

@@ +414,3 @@
+        let rows = this._grid.pageRows(this._currentPage);
+        this._translatedRows = rows;
+        for (let rowIndex in rows) {

let ... in doesn't make any guarantees about ordering; use 'for (let rowIndex = 0; rowIndex < rows.length; rowIndex++)' instead.

@@ +419,3 @@
+                mainIconRowReached = true;
+            if (!mainIconRowReached) {
+                rowsUp.push(rows[rowIndex]);

All this can be written much more concise:

let rowsUp = [];
let rowsDown = [];
let rowsArray;
for (let i = 0; i < rows.length; i++) {
    if (mainIconYPosition == rows[i][0].y)
        rowsArray = (side == St.Side.BOTTOM) ? rowsDown : rowsUp;
    else
        rowsArray = (mainIconYPosition < rows[i][0].y) ? rowsDown : rowsUp;
    rowsArray.push(rows[i]);
}

@@ +438,3 @@
+            // There's not need to pan view down
+            if (rowsUp.length >= folderNVisibleRowsAtOnce) {
+                panViewUpNRows = folderNVisibleRowsAtOnce;

Maybe it's just me, but I find this a bit hard to read. What you are doing is pretty simple: you want to make space for n rows (folderNVisibleRowsAtOnce - odd variable name IMHO), and you need to split it between rows that move up and rows that move down.
I think this would be better reflected by:

let nRowsUp;
let nRowsDown;
if (side == St.Side.BOTTOM) {
    nRowsUp = Math.min(rowsAbove.length, nRows);
    nRowsDown = nRows - nRowsUp;
} else {
    nRowsDown = Math.min(rowsBelow.length + emptyRows, nRows);
    nRowsUp = nRows - nRowsDown;
}

(note that I took the freedom to suggest slightly different names - panViewDownNRows/panViewUpNRows are not good names, as the important part (up vs. down) is hidden somewhere in the middle of the name)

@@ +453,3 @@
+        }
+        this._updateIconOpacities(true);
+        // Especial case: last page and no rows below the icon of the folder,  and

¡Olé!

@@ +454,3 @@
+        this._updateIconOpacities(true);
+        // Especial case: last page and no rows below the icon of the folder,  and
+        // there's not need to moave any rows down neither rows up.

Special case: On the last page with no rows below the icon, there's
no need to move any rows either up or down

@@ +456,3 @@
+        // there's not need to moave any rows down neither rows up.
+        // Call directly the popup without animating
+        if (panViewDownNRows > 0 && rowsDown.length == 0 && rowsUp.length == 0) {

This looks wrong - there's at least one row on the page (the one that contains the folder icon), and depending on where we pop up the folder it's either above (rowsUp) or below (rowsDown) the popup. So (rowsDown.length == 0 && rowsUp.length == 0) never evaluates to true, no?

(Does (rowsBelow.length == 0 && nRowsUp == 0) sound right?)

@@ +459,3 @@
+            this.displayingPopup = true;
+            this._popupExpansionNeeded = false;
+            iconActor.onCompleteMakeSpaceForPopUp();

Not a fan - I'd prefer some 'space-ready' (preferably with a better name) signal here and handle it in FolderIcon (e.g. make onCompleteMakeSpaceForPopUp private and a handler to the signal).

@@ +472,3 @@
+            return;
+        }
+        if (this._translatedRows) {

You can save one indentation level by doing

if (!this._translatedRows || !this._translatedRows.length)
    return;

@@ +481,3 @@
+                                              transition: 'easeInOutQuad',
+                                              onComplete: Lang.bind(this, function(){ this.displayingPopup = false; }) };
+                        Tweener.addTween(this._translatedRows[rowId][childrenId], tweenerParams);

No need for a separate tweenerParams variable here ...
(also note some missing spaces here and there)

@@ +488,3 @@
+    },
+
+    _panViewForFolderView: function(rowsUp, rowsDown, panViewUpNRows, panViewDownNRows, iconActor) {

This function doesn't really save much work; IMHO it would make some more sense to do:

  _translateRows: function(rows, translateY)

or

  _translateChildren: function(children, translateY) (e.g. a flat array to save one indentation level)

That way you actually use the function to not repeat code, rather than just making an arbitrary split from makeSpaceForPopup ...

@@ +489,3 @@
+
+    _panViewForFolderView: function(rowsUp, rowsDown, panViewUpNRows, panViewDownNRows, iconActor) {
+        let rowHeight = this._grid.rowHeight();

Is there any possibility of getting rid of this? "rowHeight" is a very simple and understandable name, which an unfortunately surprising return value ...

@@ +501,3 @@
+                                          transition: 'easeInOutQuad' };
+                    if ((rowId == rowsUp.length - 1) && (childrenId == rowsUp[rowId].length - 1))
+                            tweenerParams['onComplete'] = Lang.bind(iconActor, iconActor.onCompleteMakeSpaceForPopUp);

This should be perfectly safe to call unconditionally (in fact if you move some rows up and some rows down, you already call it twice) ...
(in which case you can just pass the params directly to addTween)

(Update: However if we manage to move this into PaginatedIconGrid, we should probably not rely on callback() being able to be called multiple times and keep this code)

@@ +1164,3 @@
             }));
+        this.actor.connect('notify::allocation', Lang.bind(this, function() {
+            Meta.later_add(Meta.LaterType.BEFORE_REDRAW, Lang.bind(this, this._updatePopupPosition));

Is this really related to this patch? Should be separate otherwise.

(Update: uhm - this is to update the position when the icon moves due to makeSpaceForPopup? But then we only open the popup once that's finished (and after updating the position))

@@ +1187,3 @@
     },
 
+    makeSpaceForPopUp: function() {

Why is this public?

@@ +1285,3 @@
     _ensurePopup: function() {
+        if (this._popup && !this._popupInvalidated) {
+            this.makeSpaceForPopUp();

This is odd. "ensure popup" == "make sure the popup is initialized and ready to be used". So don't call makeSpace() here, do it after the call to _ensurePopup() (where the open() call used to be)

@@ +1297,2 @@
                         this.actor.checked = false;
+                        this.returnSpaceToOriginalPosition();

I'd prefer this in the parent view - it already knows about the popup (addFolderPopup), so it could just connect to the open-state-changed signal itself (well, it actually does so already) ...

@@ +1302,3 @@
             this._popup.updateBoxPointer(this._boxPointerArrowside);
         }
         this._updatePopUpSize();

(unrelated to this patch, but: existing function names generally use Popup, not PopUp - would be good to be consistent there)

@@ +1304,3 @@
         this._updatePopUpSize();
         this._updatePopupPosition();
+        this._popupInvalidated = false;

Please fix the whitespace error in the corresponding patch.

::: js/ui/iconGrid.js
@@ +341,3 @@
+        if(child._translateY) {
+            childBox.y1 += child._translateY;
+            childBox.y2 += child._translateY;

This needs to be public if written from outside the class. (But then, I'd love to keep it private if we can)

@@ +590,3 @@
+        let rows = [];
+        let currentItem = this._getVisibleChildren()[pageNumber * this._childrenPerPage];
+        let children = this._grid.get_children();

Mmmh, I would have expected something like:

  let children = this._grid.getVisibleChildren();
  let currentItem = children[pageNumber * this._childrenPerPage];

here ...

@@ +592,3 @@
+        let children = this._grid.get_children();
+        let index = pageNumber * this._childrenPerPage;
+        for (let rowIndex = 0; rowIndex < this._rowsPerPage && index < children.length; rowIndex++) {

Probably easier as:

let rows = [];
let pageOffset = pageNumber * this._childrenPerPage;
let childrenPerRow = this._childrenPerPage / this._rowsPerPage;
for (let i = 0; i < this._rowsPerPage; i++) {
    rows[i] = children.slice(pageOffset + i * this._rowsPerPage, pageOffset + (i + 1) * this._rowsPerPage);
    if (rows[i].length < this._rowsPerPage)
        break;
}
return rows;

Review of attachment 253005 [details] [review]:

Quite a couple of comments (as expected, given that this is the first thorough review of this patch), but nothing really intrusive.

Some nits regarding the commit message:
 - real change is in iconGrid, so that should be the prefix
 - we already adapt the spacing in earlier patches, so subject should focus on the icons
 (something like: "iconGrid: Also adapt icon size to available space")
Body could be something like:
Similar to adapting the spacing dynamically to the available space we already do, scale down icon sizes if the grid is too small to fit the requested minimum number of rows/columns.

::: js/ui/appDisplay.js
@@ +1186,3 @@
+
+    setIconSize: function(size) {
+        this.icon.setIconSize(size);

Both functions are a bit pointless - icon is already a public property, so it's fine to access it from IconGrid (e.g. the assumptions you make change from "item has getIconSize()/setIconSize() functions" to "item has an icon property that has getIconSize()/setIconSize() functions")

::: js/ui/iconGrid.js
@@ +10,3 @@
 const Params = imports.misc.params;
 
+const ICON_SIZE = 96;

I suspect you of cheating here (making the ICON_SIZE constant match the current values in CSS) ... see comment for updateChildrenScale() below.

@@ +419,3 @@
+        let usedWidth = this.columnsForWidth(forWidth) * (this.getHItemSize() + this._getSpacing());
+        usedWidth -= this._getSpacing();
+        return usedWidth + this.left_padding + this.right_padding;

This function could be

usedWidth: function(forWidth) {
    return this.usedWidthForColumns(this.columnsForWidth(forWidth));
}

now.

@@ +433,3 @@
     },
 
+    addItem: function(item, index) {

I'm not opposed to this change (IconGrid was always meant to be used with BaseIcons), but then this should be a separate patch; it would also make sense to actually check the assumptions we make (instead of just documenting them in a comment in updateChildrenScale()), something like:

   (item.icon && item.icon instanceof BaseIcon)

This would require some more adjustments in SearchDisplay though ...


Alternatively, we could make use of the _delegate pattern, and add something like

let children = this._getVisibleChildren();
if (children.length == 0)
    return;
if (!children[0]._delegate || !children[0]._delegate.icon || !children[0]._delegate.icon instanceof BaseIcon) {
    log('Cannot adjust item size without access to a BaseIcon icon property');
    return;
}

to calculateResponsiveGrid()/updateChildrenScale()

@@ +457,3 @@
     },
 
+    getHItemSize: function() {

Should be private afaics

@@ +461,3 @@
+    },
+
+    getVItemSize: function() {

Dto.

@@ +469,3 @@
       to know how much spacing can we have at the grid
      */
     updateSpacingForSize: function(availWidth, availHeight) {

This doesn't need to be public anymore, does it?

@@ +502,3 @@
+    },
+
+    calculateResponsiveGrid: function(availWidth, availHeight) {

As mentioned in some chat conversations, I'm not a big fan of this name - "responsive" means "responding to user input while doing stuff (I/O, expensive calculations etc.)" in the context of desktop applications, so using it with its web design meaning is confusing. Maybe just use adaptToSize() as in AppDisplay? (maybe even do this throughout the patch set, e.g. use this name for updateSpacingForSize() (and include computePages() as you do now) in previous patches, then rename the function to _updateSpacingForSize() and make calculateResponsiveGrid() the new adaptToSize() in this patch)

@@ +508,3 @@
+        let spacing = this._getSpacing();
+        if (this._useSurroundingSpacing)
+            this.top_padding = this.bottom_padding = this.right_padding = this.left_padding = spacing;

updateSpacingForSize() already does this, no?

@@ +510,3 @@
+            this.top_padding = this.bottom_padding = this.right_padding = this.left_padding = spacing;
+
+        let count = 0;

Unused.

@@ +516,3 @@
+                neededWidth = this.usedWidthForNColumns(this._minColumns) - availWidth ;
+            else
+                neededWidth = this.usedWidthForNColumns(this._minColumns) - availWidth ;

No idea what that condition is supposed to look like - it's funny though!

@@ +521,3 @@
+                neededHeight = this.usedHeightForNRows(this._minRows) - availHeight;
+            else
+                neededHeight = this.usedHeightForNRows(this._minRows) - availHeight ;

Dto.

Assuming that the if/else blocks will be different, I think I'd prefer

  if (this._useSurroundingSpacing) {
      neededWidth = ...
      neededHeight = ...
  } else {
  }

@@ +526,3 @@
+                let neededSpaceForEachItem = Math.ceil(neededWidth / this._minColumns);
+                this._fixedHItemSize = this._hItemSize - neededSpaceForEachItem;
+                this._fixedVItemSize = this._vItemSize - neededSpaceForEachItem;

Could be:

let neededSpacePerItem = (neededWidth > neededHeight) ? Math.ceil(neededWidth / this._minColumns)
                                                      : Math.ceil(neededHeight / this._minRows);
this._fixedHItemSize = Math.max(this._hItemSize - neededSpacePerItem, MIN_ICON_SIZE);
this._fixedVItemSize = Math.max(this._vItemSize - neededSpacePerItem, MIN_ICON_SIZE);

@@ +540,3 @@
+            this.updateSpacingForSize(availWidth, availHeight);
+            if (this._useSurroundingSpacing)
+                this.top_padding = this.bottom_padding = this.right_padding = this.left_padding = spacing;

Mmmh, is this right? 'spacing' is the spacing as computed by the first run of updateSpacingForSize ...
(if this is not what we want, then the comment from above applies, e.g. updateSpacingForSize() already updates the padding)

@@ +547,3 @@
+
+    /**
+     * We are supossing that the this._items contain some item that we can set its size. 

"suppose", but "assume" is the better word here; as mentioned above, I'd consider documenting those assumptions in a more formal way by adding checks to addItem().

@@ +554,3 @@
+     * works fine at least.
+     */
+    updateChildrenScale: function(scale) {

Should be either private (the comment even mentions that it is not intended to be called from outside the class), or just be merged with calculateResponsiveGrid (or at least move the later_add() part there, e.g. Meta.later_add(Meta.LaterType.BEFORE_REDRAW, Lang.bind(this, this._updateChildrenScale, scale))) ...

@@ +556,3 @@
+    updateChildrenScale: function(scale) {
+        Meta.later_add(Meta.LaterType.BEFORE_REDRAW, Lang.bind(this, function() {
+            for (let i in this._items) {

let i = 0; i < this._items.length; i++

@@ +557,3 @@
+        Meta.later_add(Meta.LaterType.BEFORE_REDRAW, Lang.bind(this, function() {
+            for (let i in this._items) {
+                let newIconSize = Math.floor(ICON_SIZE * scale);

This looks wrong - you compute scale based on vItemSize/hItemSize, but now you apply it to ICON_SIZE instead? In fact, calculating a scale factor from a target size and then using that to calculate a new size is unnecessarily complicated, just do

  this._items[i].setIconSize(Math.min(this._fixedHItemSize, this._fixedVItemSize));

Or am I overlooking something?

Review of attachment 253506 [details] [review]:

Minor comments, good to go with those fixed.

::: data/theme/gnome-shell.css
@@ +992,3 @@
+    /* visually, since the label control its own espacing inside,
+       seems more visual consistent to have diferent padding for
+       top and bottom */

Some typos (spacing, different) and odd language. Maybe:

 /* since the label controls its own spacing, it is visually more
    consistent to use different padding values for top and bottom */

@@ +1654,2 @@
 .chat-notification-scrollview{
+    max-height: 22em;

Unrelated whitespace change

::: js/ui/iconGrid.js
@@ +26,2 @@
                                   x_fill: true,
+                                  y_fill: true  });

Not sure what changed here - did you sneak in an additional space before '}'?

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

Review of attachment 253429 [details] [review]:

ok

Review of attachment 253435 [details] [review]:

::: js/ui/appDisplay.js
@@ +47,3 @@
 const INDICATOR_MOVE_OFFSET = 60;
+// Fraction of page height the finger or mouse must reach before
+// change page

"before change page" sounds wrong - "to change page"?

@@ +416,3 @@
+        let [dist, dx, dy] = action.get_motion_delta(0);
+        let adjustment = this._verticalAdjustment;
+        adjustment.value -= (dy / this._pagesView.height) * adjustment.page_size;

Ah sorry, the previous comment was about 'gesture-end'/'gesture-canceled' (e.g. _onPanEnd) - the 'pan' signal does have a return value, so the previous version of _onPan was fine.

@@ +423,3 @@
+        if (diffCurrentPage > this._pagesView.height * PAGE_SWITCH_TRESHOLD) {
+            if (action.get_velocity(0)[2] > 0 && this._currentPage > 0)
+                this.goToPage(this._currentPage - 1, true, true);

Too many parameters (bad rebase? Also: http://www.codinghorror.com/blog/2005/10/avoiding-booleans.html)

@@ +425,3 @@
+                this.goToPage(this._currentPage - 1, true, true);
+            else if (this._currentPage < this._grid.nPages() - 1)
+                        this.goToPage(this._currentPage + 1, true , true);

Bad indentation

Review of attachment 253532 [details] [review]:

I agree with Jasper that this patch can be squashed with the patch that starts making use of this API (appDisplay: FolderView and FolderIcon new implementation)

::: js/ui/appDisplay.js
@@ +1060,3 @@
+    },
+
+    getOffset: function (side) {

You will always use this as "getOffset(St.Side.TOP) + getOffset(St.Side.BOTTOM)", no?
That's not wrong of course, but I don't think you have to be overly generic here (it's not a class that I'd expect to be used outside the app picker any time soon), so making this something like getVerticalOffset (or maybe even call it padding?) would be fine.

Review of attachment 253437 [details] [review]:

The whole patch doesn't make much sense to me - we are definitively not moving the focus to the FolderView's widget (as it doesn't have can_focus set), we are moving it to the first actor in its hierarchy which sets can_focus (which happens to work out right). So the actual problem doesn't appear to be the call to navigate_focus in the popup, but the time it's called - just moving it below the _boxPointer.show() call seems to work in a quick test ...

::: js/ui/appDisplay.js
@@ +1059,3 @@
     },
 
+    makeSpaceForPopUp: function() {

Rebase gone wrong?

@@ +1063,3 @@
+    },
+
+    returnSpaceToOriginalPosition: function() {

Dto.

Review of attachment 253431 [details] [review]:

Just some minor nits at this point ...

::: js/ui/iconGrid.js
@@ +364,3 @@
     },
 
+    rowHeight: function() {

Still want to get rid of this :-)

@@ +486,3 @@
+            nRows = Math.min(nRows, this._rowLimit);
+        let oldHeightUsedPerPage = this._availableHeightPerPageForItems();
+        let oldNPages = this._nPages;

Both variables are no longer used.

Review of attachment 253451 [details] [review]:

::: data/theme/gnome-shell.css
@@ +936,2 @@
 .search-display > StBoxLayout,
+.all-apps,

Mmmh, is this change really necessary? For the other elements where we use a ScrollView, the padding *has* to be on the child (BoxLayout) rather than the view for the scroll bars, which is no longer the case here. Still, I don't see why it'd hurt to leave the padding on the box (just for the symmetry with the other selectors)

::: js/ui/appDisplay.js
@@ +121,3 @@
 });
 
+/**

We use /** */ to mark gtk-doc style comments. Normal comments like this one should use //

@@ +125,3 @@
+* we have to create a custom class that returns 0 to its
+* get_preferred_height/width. In this manner the parent
+* will not take into account child preferred height/width

The comment is a bit chatty - how about
// Ignore child size requests to use the available size from the parent

@@ +128,3 @@
+*/
+const PagesView = new Lang.Class({
+    Name: 'PagesView',

There's not really anything 'view-ey' left here (which is good!), so maybe PagesBin is a better name?

@@ +146,3 @@
     _init: function() {
+        this.parent({ usePagination: true }, null);
+        this._pagesView = new PagesView({ style_class: 'all-apps',

You don't appear to be using this outside this function, so could be made local

@@ +159,2 @@
+        this._stack = new St.Widget({ layout_manager: new Clutter.BinLayout() });
+        this._box = new St.BoxLayout({ vertical: true });

Dto.

@@ +160,3 @@
+        this._box = new St.BoxLayout({ vertical: true });
+        this._verticalAdjustment = new St.Adjustment();
+        this._box.set_adjustments(new St.Adjustment() /* unused */, this._verticalAdjustment);

So my annotation patch didn't work?

@@ +165,3 @@
         this._stack.add_actor(this._grid.actor);
         this._eventBlocker = new St.Widget({ x_expand: true, y_expand: true });
+        this._stack.add_actor(this._eventBlocker, { x_align:St.Align.MIDDLE });

Uhm - the widget is set to expand, so the alignment is meaningless. Not to mention that add_actor() takes a single ClutterActor parameter, so whatever parameters you add after that are ignored anyway :-)
(if you needed the alignment, you would have to set it as { x_align: Clutter.ActorAlign.CENTER } on the eventBlocker)

@@ +188,3 @@
     },
 
+    goToPage: function(pageNumber, animated) {

Can we get rid of this parameter? A single 'animate' boolean isn't that bad, but it looks like it's only needed in a single place (when called during allocation in adaptToSize()), so maybe have some _recomputingSpacing property that we can check here instead?
(like "let time = this._recomputingSpacing ? 0 : PAGE_SWITCH_TIME")
Or just not call goToPage() there, see comment below ...

@@ +264,3 @@
     _ensureIconVisible: function(icon) {
+        let itemPage = this._grid.getItemPage(icon);
+        this.goToPage(itemPage, true, true);

too many parameters

@@ +267,3 @@
+    },
+
+    updateAdjustment: function(availHeight) {

You can just move the code into adaptToSize(). Otherwise the method should be private.

@@ +300,3 @@
+        let oldNPages = this._grid.nPages();
+
+        // Update the adjustments based on display height

Pointless comment (this is exactly what the code itself suggests)

@@ +302,3 @@
+        // Update the adjustments based on display height
+        this.updateAdjustment(availHeight);
+        // Update grid dinamyc spacing based on display width

"dynamic", and s/width/size/ (you are passing the height as well). But again, the code itself is clear enough by itself.

@@ +304,3 @@
+        // Update grid dinamyc spacing based on display width
+        this._grid.updateSpacingForSize(availWidth, availHeight);
+        // Calculate pagination values

Again the comment doesn't add any value

@@ +307,3 @@
+        this._grid.computePages(availWidth, availHeight);
+        
+        if (this._availWidth != availWidth || this._availHeight != availHeight || oldNPages != this._grid.nPages()) {

No braces, and here a small comment would help

@@ +308,3 @@
+        
+        if (this._availWidth != availWidth || this._availHeight != availHeight || oldNPages != this._grid.nPages()) {
+            this.goToPage(0, false);

This looks like it could be a bit smarter, e.g.
   this.goToPage(this._currentPage <= this._grid.nPages() ? this._currentPage : 0);

Or just set adjustment.value directly to avoid going through Tweener when you don't want animation, and get rid of the 'animate' parameter :-)

@@ +342,3 @@
+    adaptToSize: function(width, height) {
+        let box = new Clutter.ActorBox();
+        box.x1 = 0;

Could use
  box.x1 = box.y1 = 0;
to save a line of code

@@ +436,3 @@
+        this.actor = new St.Widget({ style_class: 'app-display',
+                                     x_expand: true, y_expand: true });
+        this.actor.set_layout_manager(new Clutter.BoxLayout({ vertical: true }));

You could just add { layout_manager: new Clutter.BoxLayout({ orientation: Clutter.Orientation.VERTICAL }) } the the property list. Also: why is this change needed at all? Nowadays this is actually exactly what St.BoxLayout does anyway ...

@@ +437,3 @@
+                                     x_expand: true, y_expand: true });
+        this.actor.set_layout_manager(new Clutter.BoxLayout({ vertical: true }));
+        this._viewStackLayout = new ViewStackLayout();

this._viewStack will take a reference, so you don't need to make this a property.

@@ +444,1 @@
+        this.actor.add_actor(this._viewStack, { expand: true });

Odd line move.

@@ +551,3 @@
+    _onAllocatedSizeChanged: function(actor, width, height) {
+        let box = new Clutter.ActorBox();
+        box.x1 = 0;

Again, could use
  box.x1 = box.y1 = 0;
to save a line of code

@@ +560,3 @@
+        for (let i = 0; i < this._views.length; i++) {
+            this._views[i].view.adaptToSize(availWidth, availHeight);
+        }

No braces.

::: js/ui/iconGrid.js
@@ +394,3 @@
     },
 
+    /**

Wrong comment style, use //

Review of attachment 253433 [details] [review]:

The commit message still needs some work (typos apart); the main problem as far as I can see is that the properties don't make that much sense before we start scaling items down to satisfy the minima. So how about

iconGrid: Add minRows/minColumns properties

When we adapt the grid to different display sizes, we don't want the number of
displayed items to get too small. In the future we will scale down icons to
make sure that the grid fits add least minRows x minColumns items, but for now
we only take the properties into account when calculating the dynamic spacing.

::: js/ui/iconGrid.js
@@ +406,3 @@
+        let maxEmptyHArea = availWidth - this._minColumns * this._hItemSize;
+        let maxHSpacing, maxVSpacing;
+        if (this._minRows == 1)

<=, or you'll end up with negative spacing if minRows <=0

Created attachment 253631 [details] [review]
appDisplay: FolderView and FolderIcon new implementation

The popup of the FolderView is now contained inside
the parent view, solving the overflow of apps with a ScrollView.
Also, solved a lot of bugs in popup/FolderView calculation
of position and size.

Created attachment 253635 [details] [review]
iconGrid: Add padWithSpacing property

Add a property to also add the calculated spacing
around the grid.
This will allow FolderView to be aligned with the
main grid without cutting off any of the surrounding
boxPointer decorations or close button

Review of attachment 253635 [details] [review]:

::: js/ui/appDisplay.js
@@ +80,3 @@
                                                 minColumns: MIN_COLUMNS,
+                                                fillParent: false,
+                                                usePadWithSpacing: false });

Same comment as before: Why set it to false here, then use true for all users?

::: js/ui/iconGrid.js
@@ +181,3 @@
                                         fillParent: false,
+                                        xAlign: St.Align.MIDDLE,
+                                        usePadWithSpacing: false });

Oh sorry, I meant 'pad' the verb, not the ... uhm ... lady's product. So 'padWithSpacing' (or something better for 'also add spacing around the grid', if someone wants to chime in)

@@ +420,3 @@
+            maxHSpacing = Math.floor(maxEmptyHArea / (this._minColumns +1));
+        } else {
+            if (this._minRows == 1)

As mentioned for a previous patch, should be <= IMHO

@@ +528,3 @@
         this._rowsPerPage = Math.floor((availHeightPerPage + spacing) / (this._vItemSize + spacing));
         this._nPages = Math.ceil(nRows / this._rowsPerPage);
+        this._spaceBetweenPages = availHeightPerPage - (this._availableHeightPerPageForItems() - this.topPadding - this.bottomPadding);

This looks fishy, see comment below

@@ +534,2 @@
     _availableHeightPerPageForItems: function() {
+        return this._rowsPerPage * this.rowHeight() - this._getSpacing() + this.topPadding + this.bottomPadding;

Is this right? Padding is not supposed to be available for items, is it?

Created attachment 253637 [details] [review]
border-radius for overview-icons to match with rounded edges of running-indicator.svg

Don't know if it's relevant for this bug, but 691578 still exists (at least in wip/paging-release2).

Two solutions for this:

1) Modify the edge radius of data/running-indicator.svg and change the border-radius in the dash
Possible issue:
- border-radius in the dash could be too less, make it look too edged

2) Change the border-radius of overview-icons in the app picker and the search results.
Possible issue:
- too much border radius?

A patch for solution #2 is attached.

Florian, we have to modify the glow (I knew it, just not enough time to modify it), will do it after landing pagination. Thanks!

Review of attachment 253631 [details] [review]:

Couple of comments still, nothing too serious (except for the messing-with-boxpointer-internals of course) ...

::: js/ui/appDisplay.js
@@ +875,3 @@
+        let scrollableContainer = new St.BoxLayout({ vertical: true, reactive: true });
+        let gridContainer = new St.Widget({ layout_manager: new Clutter.BinLayout() });
+        gridContainer.add_child(this._grid.actor);

Why do you need this? The box is needed because it's the only StWidget you can put in an StScrollView, but why not add the grid directly to the box?

@@ +923,3 @@
+        // Take into account padding, arrow and close button to maintain
+        // the icons inside folder view aligned with the main view and
+        // avoid to cut off the popup or the close button

// Set extra padding to avoid popup or close button being cut off?

@@ +936,3 @@
+        let pageBox = new Clutter.ActorBox();
+        pageBox.x1 = 0;
+        pageBox.y1 = 0;

I'd write this on one line (pageBox.x1 = pageBox.y1 = 0;), but up to you.

@@ +943,3 @@
+        // We only can show icons inside the collection view boxPointer
+        // so we have to substract the required padding etc of the boxpointer
+        // to make the calculation of used width right

the last line reads odd, I'd just leave it out (the comment makes perfect sense without it)

@@ +984,3 @@
+        // we have to tell collection view that the calculated values of boxpointer arrow side, position, etc 
+        // are not correct (since the allocated size of pagination changed)
+        // For that problem we calculate everything again and apply it maintaining the current popup.

Some typos in the comment, some errors (we don't "tell collection view" anything), and a bit chatty ...

How about

// whether we need to update arrow side, position etc.

Not sure it's really needed though, it doesn't read odd ...

@@ +1033,3 @@
+                                 this._popup.getCloseButtonOverlap()) / 2;
+        // We have to ensure the folder view boxpointer and the close button
+        // doesn't go outside boundary, so we have to take into account the

don't

@@ +1034,3 @@
+        // We have to ensure the folder view boxpointer and the close button
+        // doesn't go outside boundary, so we have to take into account the
+        // arrow size, the padding of the boxpointer and the close button displacement

I'd just say
// Add extra padding to prevent boxpointer decorations and close button being cut off

@@ +1040,3 @@
+
+    _updatePopupPosition: function() {
+        if (this._popup) {

A function with a single big if() block screams early return ...

@@ +1041,3 @@
+    _updatePopupPosition: function() {
+        if (this._popup) {
+            // Position the popup above or below the source icon

Unnecessary comment

@@ +1045,3 @@
+                let closeButtonOffset = -this._popup.closeButton.translation_y;
+                // We use this function since this.actor.height doesn't return a corrrect value,
+                // due to the way the AppFolderPopup manages it

This is still vague :-)
Not a big issue though IMHO ...

@@ +1190,3 @@
+        this._arrowSide = side;
+        this._boxPointer._arrowSide = side;
+        this._boxPointer._border.queue_repaint();

As mentioned previously: don't do this.

::: js/ui/iconGrid.js
@@ +398,3 @@
+    rowsForHeight: function(forHeight) {
+        forHeight -= this.topPadding + this.bottomPadding;
+        let rowsPerPage = Math.floor((forHeight + this._getSpacing()) / this.rowHeight());

Confusing variable name (forHeight may or may not be a full page height) - either kill it (e.g. return Math.floor(...) directly), or come up with a better name.

Also:
the _rowsPerPage calculation could now use this function, no?

@@ +403,3 @@
+
+    usedHeightForNRows: function(nRows) {
+        return this.rowHeight() * nRows - this._getSpacing() + this.topPadding + this.bottomPadding;

_availableHeightPerPageForItems() could now use this function.

Created attachment 253645 [details] [review]
iconGrid: Add padWithSpacing property

Add a property to also add the calculated spacing
around the grid.
This will allow FolderView to be aligned with the
main grid without cutting off any of the surrounding
boxPointer decorations or close button

Created attachment 253648 [details] [review]
boxPointer: Allow to update the arrow side

Add a function to allow to update the arrow side

Created attachment 253649 [details] [review]
appDisplay: Align and contain collection grid with parent view

The popup of the FolderView is now contained inside
the parent view, solving the overflow of apps with a ScrollView.
Also, solved a lot of bugs in popup/FolderView calculation
of position and size.

Review of attachment 253645 [details] [review]:

In the commit message: "... or the close button" reads better to me. But yeah, this is good to go!

::: js/ui/iconGrid.js
@@ +454,3 @@
     _getPreferredHeight: function (grid, forWidth, alloc) {
+        alloc.min_size = (this._availableHeightPerPageForItems() + this.bottomPadding + this.topPadding) * this._nPages + this._spaceBetweenPages * this._nPages;
+        alloc.natural_size = (this._availableHeightPerPageForItems() + this.bottomPadding + this.topPadding) * this._nPages + this._spaceBetweenPages * this._nPages;

The error was there before, but 'spaceBetweenPages * nPages' should be 'spaceBetweenPages * (nPages - 1)', no?
(Fixed belongs in the previous patch obviously).

@@ +503,3 @@
                 y += this._vItemSize + spacing;
                 if((i + 1) % this._childrenPerPage == 0)
+                    y+= - spacing + this._spaceBetweenPages + this.bottomPadding + this.topPadding ;

Stray space before ;

Starting the calculation with a negative value reads odd, any particular reason you changed the order there?

Review of attachment 253648 [details] [review]:

::: js/ui/boxpointer.js
@@ +644,3 @@
+    updateArrowSide: function(side) {
+        this._arrowSide = side;
+        this._border.queue_repaint();

Are you sure that's all that's needed? What if the function is called why the boxpointer is visible?
(If this is indeed enough, then we can squash it back with the follow-up patch - sorry for the noise)

Created attachment 253651 [details] [review]
iconGrid: Add padWithSpacing property

Add a property to also add the calculated spacing
around the grid.
This will allow FolderView to be aligned with the
main grid without cutting off any of the surrounding
boxPointer decorations or close button

Created attachment 253652 [details] [review]
iconGrid: Add padWithSpacing property

Add a property to also add the calculated spacing
around the grid.
This will allow FolderView to be aligned with the
main grid without cutting off any of the surrounding
boxPointer decorations or the close button

Created attachment 253653 [details] [review]
iconGrid: Add padWithSpacing property

Add a property to also add the calculated spacing
around the grid.
This will allow FolderView to be aligned with the
main grid without cutting off any of the surrounding
boxPointer decorations or the close button

Review of attachment 253653 [details] [review]:

::: js/ui/iconGrid.js
@@ +418,3 @@
+            // to divide the empty space
+            maxVSpacing = Math.floor(maxEmptyVArea / (this._minRows +1));
+            maxHSpacing = Math.floor(maxEmptyHArea / (this._minColumns +1));

Space after '+' on both lines

Created attachment 253659 [details] [review]
appDisplay: Align and contain collection grid with parent view

The popup of the FolderView is now contained inside
the parent view, solving the overflow of apps with a ScrollView.
Also, solved a lot of bugs in popup/FolderView calculation
of position and size.

Created attachment 253661 [details] [review]
iconGrid: Add padWithSpacing property

Add a property to also add the calculated spacing
around the grid.
This will allow FolderView to be aligned with the
main grid without cutting off any of the surrounding
boxPointer decorations or the close button

Review of attachment 253659 [details] [review]:

Yup.

Created attachment 253683 [details] [review]
appDisplay: Make space on grid to fit collection when opening

Add properly functions in AllView and FolderView to animate
the FolderView popup opening and give enough space on the parent
view to fit the popup of the folder view

Created attachment 253685 [details] [review]
appDisplay: Add page indicators

Add indicators to the pagination in AllView, which displays
how many pages of apps we have and allows the user to
navigate through them.

Review of attachment 253683 [details] [review]:

Sigh, if you look long enough you always find stuff to complain about ...

This is actually starting to look quite good, but marking needs-work because you missed some occurrences of displayingPopup when renaming to _displayingPopup.

::: js/ui/appDisplay.js
@@ +290,3 @@
         this._eventBlocker.add_action(this._clickAction);
 
+        this._displayingPopup = false;

This looks right to me ...

@@ +307,2 @@
     goToPage: function(pageNumber) {
+        if (this._currentPage == pageNumber && this.displayingPopup && this._currentPopup)

... this doesn't (you are now using a wild mix of _displayingPopup vs. displayingPopup)

Unrelated, I do wonder if using

  if (this._displayingPopup)
      return;

would make sense here? Page switching via scrolling and swiping is already disabled when a folder is open, it might make sense to do the same for indicators?
(Not a strong opinion on my part, just an observation - so feel free to ignore etc.)

@@ +355,3 @@
+    * makeSpaceForPopUp:
+    * @y: The base position to move rows
+    * @side: The movement direction

This is wrong - items may use in both directions, this parameter is about whether the empty space should be created above or below @y. I didn't mention it in the previous review, but having St.Side.BOTTOM mean "create empty space *above* @y" is fairly odd. Not sure the code to reverse the arrow direction for this parameter is worth it though ...

@@ +360,3 @@
+    */
+    makeSpaceForPopup: function(y, side, nRows) {
+        let mainIconYPosition = y;

Why? The additional variable isn't needed at all (maybe 'y' could have a more expressive name like sourceRowY or so, though I think just 'y' should be fine)

@@ +415,3 @@
+        for (let i = 0; i < children.length; i++) {
+            children[i].translation_y = 0;
+            let params = { translation_y: translationY,

Yesssssss!

@@ +417,3 @@
+            let params = { translation_y: translationY,
+                           time: POPUP_FOLDER_VIEW_ANIMATION,
+                           onUpdate: function() { this.queue_relayout(); },

This shouldn't be necessary anymore (another yesssss!)

@@ +426,3 @@
+    },
+
+    returnSpaceToOriginalPosition: function() {

This can be private now, no?

@@ +1095,2 @@
         if (this._boxPointerArrowside == St.Side.BOTTOM) {
             let closeButtonOffset = -this._popup.closeButton.translation_y;

Doesn't belong in this patch, but at some point this variable ended up unused ...

@@ +1095,3 @@
         if (this._boxPointerArrowside == St.Side.BOTTOM) {
             let closeButtonOffset = -this._popup.closeButton.translation_y;
+            this._popup.actor.y = this.actor.allocation.y1 + this.actor.translation_y - this._popupHeight();

Odd to use actor.allocation here, no idea why actor.y doesn't work for you. I'll take a look later if I have some time, but let's not block on this ...
(I consider it also slightly inelegant to include translation_y here, as it relies on an implementation detail of how makeSpaceForPopup is implemented, but I don't really have a good suggestion how to avoid it off-hand (adding a getItemPosition() on AllView that returns this isn't that much nicer ... let's not block on this)

@@ -975,3 @@
             let closeButtonOffset = -this._popup.closeButton.translation_y;
-            // We use this function since this.actor.height doesn't return a corrrect value,
-            // due to the way the AppFolderPopup manages it

Why add this comment earlier and remove it now? I'd say either don't add it in the first place, or leave it ..

Review of attachment 253685 [details] [review]:

Much better. Still, I found a small bug that needs fixing ...

::: js/ui/appDisplay.js
@@ +186,3 @@
+    setCurrentPage: function(currentPage) {
+        if (this._currentPage == currentPage)
+            return;

I've found a corner case where this breaks:
If you click the active page, the button toggles its state (checked == false), emits 'page-activated' which results in a call to setCurrentPage() ... but then we return early, so the button is not toggled on again - we end up with *no* indicator being selected.

I see several options for a fix:
(1) remove the above code (it means we will unnecessarily iterate of the children
    sometimes, but it's hardly an expensive operation for any number of indicators
    not completely shit-bat crazy - the actual property/style change in StButton is
    more expensive, but will only happen when there is an actual change)

(2) don't use toggle_mode and add/remove the :checked style yourself (this sounds like
    more work than it is - it's basically

    if (pageIndex == this._currentPage)
        indicator.add_style_pseudo_class('checked');

    in _init() and

    if (i == this._currentPage)
        children[i].add_style_pseudo_class('checked');
    else
        children[i].remove_style_pseudo_class('checked');

    here ...

(3) update the checked property in the 'clicked' handler before emitting the signal - not a
    big fan of this, as on first glance the code would look like it made assumption on whether
    an actual page switch will occur

Created attachment 253702 [details] [review]
appDisplay: Use new IconGrid implementation in AllView and FrequentView

Add "usePagination" parameter to Alphabetical view to allow AllView
use the new PaginatedIconGrid.
FrequentView adapts the new IconGrid implementation. Create a
PagesView class to allow us control the allocation of the IconGrid
child and the scroll adjustment. Hook up the allocation/size comunication
between the FrequentView, AllView and FolderView with AppDisplay with
a ViewStackLayout that updates all views.

Created attachment 253704 [details] [review]
iconGrid: Also adapt icon size to available space

Similar to adapting the spacing dynamically to the available
space we already do, scale down icon sizes if the grid is too
small to fit the requested minimum number of rows/columns.

Review of attachment 253702 [details] [review]:

Looks good to me now

::: js/ui/appDisplay.js
@@ +124,1 @@
+// Ignore child size requests to use the available size from the paren

typo: parent

Review of attachment 253704 [details] [review]:

::: js/ui/appDisplay.js
@@ +579,3 @@
         let oldNPages = this._grid.nPages();
 
+        // Update the adjustments based on display height

Odd comment (e.g. it's commenting a perfectly understandable line that was still there before, but not considered comment-worthy)

@@ +582,3 @@
         this._updateAdjustment(availHeight);
+
+        // Update grid dinamyc spacing based on display width

*dynamic (that's the 3rd time I point this out for this patch?)

::: js/ui/iconGrid.js
@@ +396,2 @@
     rowsForHeight: function(forHeight) {
+        return Math.floor((forHeight -(this.topPadding + this.bottomPadding) + this._getSpacing()) / (this._getVItemSize() + this._getSpacing()));

Already added in a previous patch: missing space after '-'

@@ +456,2 @@
     /**
      * This function must to be called before iconGrid allocation,

This comment should now move to adaptToSize() (which is the new public function which calls this)

@@ +517,3 @@
+    },
+
+    /**

This is not a gtk-doc style comment!

@@ +523,3 @@
+     * we are inside the allocation of the AppDisplay, and modifinyg icon size can cause allocation cycles
+     * So this functions is not intentded to be called outside this class, lets think a little about that. Now reescaling icons
+     * works fine at least.

This comment doesn't make any sense now - you are no longer assuming that an item in _items has a setIconSize() method, you are relying on item.icon being a BaseIcon (apparently that is based on my suggested addItem() patch, you should attach this here as well)
The only thing still comment-worthy in my opinion is the ICON_SIZE (as used anywhere else in IconGrid) and ICON_SIZE (as used here and in BaseIcon) issue ...

@@ +621,3 @@
+    adaptToSize: function(availWidth, availHeight) {
+        this.parent(availWidth, availHeight);
+        this.computePages(availWidth, availHeight);

I know I suggested doing this (for the whole patch set though, not just in the last patch), but I'm no longer certain - both adaptToSize() (for dynamic spacing) and computePages() (mostly to compute the empty space between pages to not have partially visible rows) must be called before allocation, but is it really a good idea to tie pagination to dynamic spacing? Or should we allow paginated grids with fixed spacing, e.g. keep the functions separate?
(If we leave the code like this, computePages() can become private btw)

Created attachment 253709 [details] [review]
iconGrid: Also adapt icon size to available space

Similar to adapting the spacing dynamically to the available
space we already do, scale down icon sizes if the grid is too
small to fit the requested minimum number of rows/columns.

Created attachment 253710 [details] [review]
appDisplay: Use new IconGrid implementation in AllView and FrequentView

Add "usePagination" parameter to Alphabetical view to allow AllView
use the new PaginatedIconGrid.
FrequentView adapts the new IconGrid implementation. Create a
PagesView class to allow us control the allocation of the IconGrid
child and the scroll adjustment. Hook up the allocation/size comunication
between the FrequentView, AllView and FolderView with AppDisplay with
a ViewStackLayout that updates all views.

Review of attachment 253709 [details] [review]:

OK.

Review of attachment 253710 [details] [review]:

Yup.

Created attachment 253714 [details] [review]
appDisplay: Make space on grid to fit collection when opening

Add properly functions in AllView and FolderView to animate
the FolderView popup opening and give enough space on the parent
view to fit the popup of the folder view

https://bugzilla.gnome.org/show_bug.cgi?id=706081

iconGrid: Fix pageRows() corner case

Created attachment 253715 [details] [review]
appDisplay: Add page indicators

Add indicators to the pagination in AllView, which displays
how many pages of apps we have and allows the user to
navigate through them.

Review of attachment 253714 [details] [review]:

OK. I'd still like to see how the code would look with moving the functionality to IconGrid, but let's not block on this ...

Created attachment 253716 [details] [review]
iconGrid: Use minimum rows/columns properties to adjust spacing

Use properties to allow iconGrid satisfy the rows/columns
minima properties reducing spacing acordingly.
The properties are named minRow and minColumns,
so if you set the minRow to, for example 4,
the spacing of iconGrid will be reduced to fit
4 rows in the available space.

Review of attachment 253715 [details] [review]:

LGTM

Review of attachment 253716 [details] [review]:

OK

Created attachment 253717 [details] [review]
iconGrid: Add PaginatedIconGrid

The new PaginatedIconGrid class acts as a container for pages.
So the new class provides  the container behaviour and some
useful functions like positions of pages, number of pages, etc.
But, it doesn't add indicators of the pages and doesn't manage
the scroll of the pages, neither any management of the pages
like in which page currently it is, etc.

Created attachment 253718 [details] [review]
appDisplay: Add and rework pan action response

Add pan action to AllView and rework it to take
into account the velocity the user gives to the action

Review of attachment 253717 [details] [review]:

OK.

Review of attachment 253718 [details] [review]:

::: js/ui/appDisplay.js
@@ +353,3 @@
+            else 
+                if (this._currentPage < this._grid.nPages() - 1)
+                    this.goToPage(this._currentPage + 1);

Ah sorry - "else if" belongs on one line as in the previous patch. The comment was about the "this.goToPage()" line, which should align with the "this.goToPage()" line in the if block.
(No need to reattach, just fix it in the branch please)

Created attachment 253726 [details] [review]
appDisplay: Animate indicators

Add a translation animation for indicators, following
design reasons

(In reply to comment #196)
> Created an attachment (id=253726) [details] [review]

(reviewed in chat, so the status is fine)

Comment on attachment 253637 [details] [review]
border-radius for overview-icons to match with rounded edges of running-indicator.svg

I'll mark this patch as obsolete - it doesn't apply anymore to the current branch, and kinda got lost in the whole review-rereview cycle. I've filed a rebased version of it as bug 707295, let's take a look once we land the patches in this bug!

We worked in some commits message

Created attachment 253876 [details] [review]
iconGrid: Split out _calculateChildBox

Split out the calculation of the child box in allocation function
to be reusable by subclasses and let the code be more modular

Created attachment 253877 [details] [review]
iconGrid: Move spacing calculation to its own function

Move spacing calculation to a function, which makes
it reusable and overwritable by subclasses

Created attachment 253878 [details] [review]
iconGrid: Add PaginatedIconGrid

The new PaginatedIconGrid class acts as a container for pages.
So the new class provides  the container behaviour and some
useful functions like positions of pages, number of pages, etc.
But, it doesn't add indicators of the pages and doesn't manage
the scroll of the pages, neither any management of the pages
like in which page currently it is, etc.

Created attachment 253879 [details] [review]
iconGrid: Rename childrenInRow to columnsForWidth

Since the parameter of the function is the width, reflect that in
the function name. Also, since we are counting columns, not only
children for each row, reflect that in the function name also.

Created attachment 253880 [details] [review]
appDisplay: Move FolderView near FolderIcon for better context

Since FolderView is closely related with FolderIcon, we
have more context while working if FolderView is near
FolderIcon

Created attachment 253881 [details] [review]
appDisplay: Rename AlphabeticalView to BaseAppView

Since the items are not ordered in an alphabetical
way, just rename the class to be consistent.

Created attachment 253882 [details] [review]
appDisplay: Animate _updateIconOpacities

Animate the transition between full opacity and partly opacity
to follow overall animations design of gnome-shell

Created attachment 253883 [details] [review]
appDisplay: Paginate AllView

Organize applications in AllView by pages using the new PaginatedIconGrid
added previously. Pagination is generally a better pattern for collections
than scrolling, as it better suits spacial memory.
Hook into AppDisplay's allocation function to communicate the available
size to the different views before child allocations - this is only
required by the paginated view (as pages must be computed before
calling get_preferred_height/get_preferred_width), but doing it for
all views will guarantee that their dynamic spacing calculation is
based on the same values.

Created attachment 253884 [details] [review]
iconGrid: Add minRows/minColumns properties

When we adapt the grid to different display sizes,
we don't want the number of displayed items to get
too small. In the future we will scale down icons to
make sure that the grid fits add least minRows
x minColumns items, but for now we only take the
properties into account when calculating the dynamic spacing.

Created attachment 253885 [details] [review]
theme: Change app picker bottom padding

Increase the bottom padding between the views and the control buttons
of the AppDisplay to be more eye pleasant

Created attachment 253886 [details] [review]
appDisplay: Add page indicators

Add indicators to the pagination in AllView, which displays
how many pages of apps we have and allows the user to
navigate through them.

Created attachment 253887 [details] [review]
appDisplay: Add and rework pan action response

Add pan action to AllView and rework it to take
into account the velocity the user gives to the action

Created attachment 253888 [details] [review]
iconGrid: Add padWithSpacing property

Add a property to also add the calculated spacing
around the grid.
This will allow FolderView to be aligned with the
main grid without cutting off any of the surrounding
boxPointer decorations or the close button

Created attachment 253889 [details] [review]
appDisplay: Align and contain collection grid with parent view

The popup of the FolderView is now contained inside
the parent view, solving the overflow of apps with a ScrollView.
Also, solved a lot of bugs in popup/FolderView calculation
of position and size.

Created attachment 253890 [details] [review]
appDisplay: Start always at scroll 0 on FolderView

Reset the scroll adjustment between popups opennings,
following the same design we want to the AllView

Created attachment 253891 [details] [review]
appDisplay: Start always at page 0 in AllView

Reset the AllView scroll adjustment between overview openings,
following design reasons

Created attachment 253892 [details] [review]
iconGrid: Add openExtraSpace()/closeExtraSpace() methods

Add methods to open/close extra space for n rows. The app picker
will use those to make AppFolder popups appear inline with the
main grid rather than on top of it.

Created attachment 253893 [details] [review]
appDisplay: Make space on grid to fit collection when opening

Move icons out of the way to make place for the FolderView
popup before opening it.

Created attachment 253894 [details] [review]
appDisplay: Add pan action to FolderView

Since we have now a ScrollView in the FolderView,
add support for the pan action

Created attachment 253895 [details] [review]
theme: Change icons style to follow new design

Change the background, glow and labels of the Dash and AppDisplay
to follow the new design

Created attachment 253896 [details] [review]
iconGrid: Change IconGrid.addItem() to take an object instead of an actor

IconGrid has never really been a general purpose container, but has
always been used in conjunction with BaseIcon. IconGrid will soon
gain the ability to adjust the item size dynamically to adapt to the
available space, which will require that we can make some more
assumptions about the items added to the grid (namely: we need
access to BaseIcon's setIconSize() method).
So change addItem() to take an object instead, which should have
an actor and a (BaseIcon) icon property.

Based on a patch by Carlos Soriano.

Created attachment 253897 [details] [review]
iconGrid: Also adapt icon size to available space

Similar to adapting the spacing dynamically to the available
space we already do, scale down icon sizes if the grid is too
small to fit the requested minimum number of rows/columns.

this is done, right ?

Yes, Carlos just forgot to update the bug.

Attachment 253876 [details] pushed as cc51982 - iconGrid: Split out _calculateChildBox
Attachment 253877 [details] pushed as cfb8026 - iconGrid: Move spacing calculation to its own function
Attachment 253878 [details] pushed as 3984c47 - iconGrid: Add PaginatedIconGrid
Attachment 253879 [details] pushed as cc44922 - iconGrid: Rename childrenInRow to columnsForWidth
Attachment 253880 [details] pushed as 961e1b8 - appDisplay: Move FolderView near FolderIcon for better context
Attachment 253881 [details] pushed as fbb4077 - appDisplay: Rename AlphabeticalView to BaseAppView
Attachment 253882 [details] pushed as 804c027 - appDisplay: Animate _updateIconOpacities
Attachment 253883 [details] pushed as 754ca7c - appDisplay: Paginate AllView
Attachment 253884 [details] pushed as ae263bb - iconGrid: Add minRows/minColumns properties
Attachment 253885 [details] pushed as e8b35f4 - theme: Change app picker bottom padding
Attachment 253886 [details] pushed as dcea8be - appDisplay: Add page indicators
Attachment 253887 [details] pushed as 46bd1b9 - appDisplay: Add and rework pan action response
Attachment 253888 [details] pushed as 6d6c400 - iconGrid: Add padWithSpacing property
Attachment 253889 [details] pushed as 1313c1b - appDisplay: Align and contain collection grid with parent view
Attachment 253890 [details] pushed as 6ef7753 - appDisplay: Start always at scroll 0 on FolderView
Attachment 253891 [details] pushed as 74978e8 - appDisplay: Start always at page 0 in AllView
Attachment 253892 [details] pushed as dd9f802 - iconGrid: Add openExtraSpace()/closeExtraSpace() methods
Attachment 253893 [details] pushed as 3f24a87 - appDisplay: Make space on grid to fit collection when opening
Attachment 253894 [details] pushed as 1e02081 - appDisplay: Add pan action to FolderView
Attachment 253895 [details] pushed as 9a8bf3b - theme: Change icons style to follow new design
Attachment 253896 [details] pushed as 792b963 - iconGrid: Change IconGrid.addItem() to take an object instead of an actor
Attachment 253897 [details] pushed as d58f064 - iconGrid: Also adapt icon size to available space