Created attachment 139567 [details] [review]
Possible patch

I'm attaching a patch which shows a fixed amount of links fitting into the available space. What do you think of it? Maybe I should add "<<" and ">>" links to go to the first and last item?

Created attachment 139597 [details] [review]
New version with "<<" and ">>" links

Created attachment 139736 [details] [review]
Updated patch against the latest revision

+        let wordWidth = global.get_max_word_width(this.displayControl,

Eek, don't use that crack function =)  I need to remove it.  Also this code won't work with the non-fixed-width dash/genericDisplay rewrite in bug 590493.

This is a tricky problem, but a dynamic way would be to use a Big.Box.  Then connect to notify::allocation on that.  Inside there, divide up the width, and re-set the width of individual children.  You may have to have some sort of _cachedWidth variable to avoid recursion/looping.

Right now though to correctly do this kind of thing you need to write a C class so you can override _get_preferred_[width,height] and _allocate.

You might consider breaking out the paging control into a new JavaScript prototype if you try the JS route, say PagingControl.

Created attachment 140243 [details] [review]
So far...

I've talked with Owen about this on IRC and he told me that using a GenericContainer would be The Right Thing.

I'm still finishing some details but here's what I have so far.

Created attachment 140290 [details] [review]
New patch

Changed the way _allocate works, so that instead of dividing the available width by the width of the biggest child it runs through all the childs and calculates how many of them it can place into the available space.

Still not finished though :(.

Looks definitely like a good start!

+        this._list.connect('notify::n-pages', Lang.bind(this, function (grid, alloc) {
+            this.displayControl.update()
+        }));

Existing bug with the function parameters, I filed bug 591316

+        this.actor.remove_all();
[...]
+        if (numPages < this._numPages)
+            this._pageControls.slice(numPages, this._numPages - numPages);

This isn't actually good enough, because when we have a Link object, we have a memory reference cycle that Javascript can't figure out:

 Link JS object references Link JS proxy
 Link JS proxy references C object
 C object references signal handler (for button-press-event, say)
 Signal handler references Link JS object

Because some of these references are in the C code in GObject, the Javascript garbage collector can't figure them out, and nothing will be ever freed. The solution to this is simple - you should never just remove an actor from it's paraent - you need to destroy() it. Calling destroy() on an actor automatically removes it from its parent, so it isn't typically necessary to do both.

+    _createActors : function() {

Better as "updateActors" ?

+            if (this.actor.get_children().length > 0)
+                this.actor.remove_actor(this._goLast.actor);
+            this._goLast = this._createActor(numPages - 1, ">>");

Seems easier to have the goLast actor be special with a separate callback and not tied to a particular page number. 

In terms of ordering, I think the simplest thing to do is to do what Colin did for the separator for the WellGrid - just create the goFirst/goLast actors initially, and make them always the first two children. There's no reason that the order of children in the group has to correspond to the order that you lay them out in allocate().

+        alloc.min_size = minWidth * 2 + this._spacing;

Could use a comment. (The 2 is for << and >> ?)

+        alloc.natural_size = naturalWidth * this._numPages + this._spacing * (this._numPages - 1);

This seems out of sync with the new allocation code? the preferred width should be the sum of all the actor widths?

+            let [currentMinHeight, currentNaturalHeight] = children[i].get_preferred_height(forWidth);

The width you should pass in here is the width that you are going to allocate the child at - the natural width from child.get_preferred_width(), it looks like.

+    _allocate: function (grid, box, flags) {

I think it would be nice to have a comment up-front explaining the user-visible behavior you are trying to implement. That gives someone reading the function a headstart in figuring it out.

+        let goFirstWidth = children[0].get_preferred_size().slice(2, 3)[0];
+        let goLastWidth = children[children.length - 1].get_preferred_size().slice(2, 3)[0];

Yikes, that's completely unreadable. Suggest adding a helper

 function _getNaturalWidth(actor) {
    let [minWidth, naturalWidth] = actor.get_preferred_width();
    return naturalWidth;
 }

+        let mode = (page < lastPage) + 0;

ETOOTRICKY. If you want a mode variable do something like:

 const GOING_FORWARD = 0;
 const GOING_BACKWARD = 1;

 let mode = (page < lastPage) ? GOING_FORWARD : GOING_BACKWARDS;

+ while (true) {
 [...]
+            if (i == 1 || i == lastPage) {
+                if (mode != 2) {
+                    let direction = (i == 1) ? 1 : -1;
+                    i += direction * offset;
+                    offset = direction;
+                    mode = 2;
+                } else
+                    break;
+            } else if (mode != 2 && i != start) {
+                mode ^= 1;
+                offset++;
+            }
+            i += ((mode) ? 1 : -1) * offset;
+        }

This is pretty difficult to figure out. I think you might be trying too hard to save duplicated code.

Something like;

 startPage = endPage = currentPage;
 while (/* space available */ && /* unadded pages */) {
    if (/* space available * && startPage > 0) {
        /* add startPage -1, remove from space */
        startPage--;
    }

    if (/* space available */ && endPage < numPages - 1) {
        /* add endPage + 1, remove from space */
        endPage++;
    }
 }

Or whatever the appropriate algorithm is. If there's common code between "add startPage - 1" and "add endPage + 1" you can put that into a function.

+
+        for (let i = 0; i < children.length; i++)
+            children[i].hide();
[...]
+        for (let i in visiblePages) {
+            let [j, width, height] = visiblePages[i];
+            children[j].show();

Would advise against a hide/show for actors that stay visible - that's almost certainly going to put you into a loop of constantly relayout.

Suggestion to keep things simple - always allocate the space for the start and end actors, but only show them when appropriate

So:

     1 2 3
     1 2 3 4 >>
  << 3 4 5 6 >>
  << 6 7 8 9

That will prevent you from having to "back up" at the end if you decide you don't have enough space and I think it will look reasonable - maybe even better than alternatives.

Created attachment 140618 [details] [review]
This fix is doomed :P

Here's an updated patch, but again it isn't ready yet. Outstanding issues:

 - Sometimes the number for the active page won't have the correct colour; couldn't figure out why this happens.

 - When lots of pages are listed, switching to any other than the two first or two last ones I only see three page numbers and the arrows ("<<" and ">>").

Created attachment 140778 [details] [review]
One bug less!

Okay, fixed the second problem and some other stuff (and changed the way I was doing some stuff).

The problem with the incorrect item colouring still remains, but apparently only browsing the application categories.

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

We should do some fix here for the 2.28.0 release

This is good work, but also rather a complex patch that interacts in somewhat scary ways with the clutter allocation system.

Our long term goal is probably to use scrolling rather than paging for vertical lists and to make sure that they don't get so long that scrolling is unmanageable.

NbtkScrollView (on the nbtk-introduction branch) makes scrolling pretty trivial to do. If we land the branch we can go that way.

If we don't we might want to consider just limiting the recent-documents list to say 200 items.

And we might need to do that even with scrolling or creating the scrolled list will be prohibitively slow.

What I'd like to see here is a dynamic scrollbar, where going down results in more content being fetched.

We now have scrolling. It's not dynamic scrolling, just shows all the items at the moment - but this particular problem shouldn't be an issue any more.