Created attachment 254789 [details] [review]
appDisplay: Change pages with page down/up keys

Add key bindings to app picker to allow change pages using
the page up/down keys.

Created attachment 254790 [details] [review]
appDisplay: Change pages with page down/up keys

Add key bindings to app picker to allow change pages using
the page up/down keys.

Review of attachment 254790 [details] [review]:

::: js/ui/appDisplay.js
@@ +340,3 @@
+
+        this.actor.connect('notify::mapped', Lang.bind(this,
+                           function() {

The above code indents the anonymous function by 4 spaces instead of aligning with the other parameters - we should at least be consistent within a method :-)

@@ +342,3 @@
+                           function() {
+                                if (this.actor.mapped)
+                                    this._keyPressEventId = 

Trailing whitespace

@@ +345,3 @@
+                                        global.stage.connect('key-press-event',
+                                                             Lang.bind(this, this._onKeyPressEvent));
+                                    else

Wrong indentation

@@ +346,3 @@
+                                                             Lang.bind(this, this._onKeyPressEvent));
+                                    else
+                                        global.stage.disconnect(this._keyPressEventId);

Usually this should be something along the lines of

  if (this._keyPressEventId)
      global.stage.disconnect(this._keyPressEventId);
  this._keyPressEventId = 0;

It should be safe in this case to assume that you are only notified when ClutterActor:mapped actually changes, still ...

@@ +463,3 @@
+                this.goToPage(this._currentPage + 1);
+        }
+        return true;

You should only return 'true' if the event was handled (read: on PageUp/PageDown) and 'false' otherwise. For instance another handler that is listening to key events on the stage at that point is ViewSelector (for search-as-you-type); it happens to still work because signal handlers are processed in the order they are attached, and ViewSelector connects its handler before this one, but that's a bunch of implementation details you should not rely on.

Created attachment 254798 [details] [review]
appDisplay: Change pages with page down/up keys

Add key bindings to app picker to allow change pages using
the page up/down keys.

Review of attachment 254798 [details] [review]:

::: js/ui/appDisplay.js
@@ +347,3 @@
+                } else {
+                    global.stage.disconnect(this._keyPressEventId);
+                    this._keyPressEventId = 0;

The point of resetting the handler id is that 0 is not a valid id (e.g. gobject.connect('signal') will never return 0), so you can use it to ensure you are not trying to disconnect the same handler twice (which will result in a warning):

  if (this._keyPressEventId)
      global.stage.disconnect(this._keyPressEventId);

In this case clutter is well-behaving in that it will only notify on 'mapped' if the value actually changed, so it will still work (without warnings) without resetting the id, but either do it properly or don't do it at all :-)

@@ +459,3 @@
+    _onKeyPressEvent: function(actor, event) {
+        if (event.get_key_symbol() == Clutter.Page_Up) {
+            if (this._currentPage > 0)

Mmmh, would it make sense to move those checks into goToPage()?

Created attachment 254875 [details] [review]
appDisplay: Move boundary page assertions

Since the function that manages the changes between pages is
goToPage, just move the assertions of page >= 0 and page < nPages
to that function

Created attachment 254876 [details] [review]
appDisplay: Change pages with page down/up keys

Add key bindings to app picker to allow change pages using
the page up/down keys.

(In reply to comment #6)
> Created an attachment (id=254875) [details] [review]
> appDisplay: Move boundary page assertions
> 
> Since the function that manages the changes between pages is
> goToPage, just move the assertions of page >= 0 and page < nPages
> to that function

I sneaked a new line because it looks better, since I'm touching that function, is it ok?

(In reply to comment #5)
> Review of attachment 254798 [details] [review]:
> 
> ::: js/ui/appDisplay.js
> @@ +347,3 @@
> +                } else {
> +                    global.stage.disconnect(this._keyPressEventId);
> +                    this._keyPressEventId = 0;
> 
> The point of resetting the handler id is that 0 is not a valid id (e.g.
> gobject.connect('signal') will never return 0), so you can use it to ensure you
> are not trying to disconnect the same handler twice (which will result in a
> warning):
> 
>   if (this._keyPressEventId)
>       global.stage.disconnect(this._keyPressEventId);
> 
> In this case clutter is well-behaving in that it will only notify on 'mapped'
> if the value actually changed, so it will still work (without warnings) without
> resetting the id, but either do it properly or don't do it at all :-)
oh, I though disconnect from value 0 is ok, I didn't know we use that like a boolean. Understood
> 
> @@ +459,3 @@
> +    _onKeyPressEvent: function(actor, event) {
> +        if (event.get_key_symbol() == Clutter.Page_Up) {
> +            if (this._currentPage > 0)
> 
> Mmmh, would it make sense to move those checks into goToPage()?
yes

btw, although I answered to your comments after pushing my new patches, all is already applied. Just to be sure I don't confuse you.

Review of attachment 254875 [details] [review]:

OK

Review of attachment 254876 [details] [review]:

LGTM, one behavioral question:

::: js/ui/appDisplay.js
@@ +455,3 @@
     },
 
+    _onKeyPressEvent: function(actor, event) {

Mmh, should we return early when _displayingPopup is true? It's what we do for scrolling ...

(In reply to comment #12)
> Review of attachment 254876 [details] [review]:
> 
> LGTM, one behavioral question:
> 
> ::: js/ui/appDisplay.js
> @@ +455,3 @@
>      },
> 
> +    _onKeyPressEvent: function(actor, event) {
> 
> Mmh, should we return early when _displayingPopup is true? It's what we do for
> scrolling ...

oh, I thougth about that before, but I though "it's comfortable to change pages with the popup open if using keyboard" but I forgot that we can scroll with page up/down in the scrollView of collection.

So yes, good catch.

Attachment 254875 [details] pushed as f38091d - appDisplay: Move boundary page assertions
Attachment 254876 [details] pushed as 7b7c456 - appDisplay: Change pages with page down/up keys