I'm working on this bug.

Created attachment 327313 [details] [review]
chatView: functionality added for Home/End keys

In commit 755549HomeAndEndButtonsIgnored, we added functionality for the
Home and End keys, enabling navigation in the chat view.

Problem presented itself when no change occured when using the Home/End
keys.

Since it wasnt apparent that an existing functionality for Home/End was
available, an if/else control structure was inserted to accomodate the
functionality.

Review of attachment 327313 [details] [review]:

In addition to the code issues pointed out below, please follow http://chris.beams.io/posts/git-commit/ for commit messages. In particular:
 - follow the style guidelines like using imperative mood
 - don't describe the code - if the patch adds an if/else
   structure, then it can be seen in the patch (git log -p);
   instead, briefly outline *why* the change is done

::: src/chatView.js
@@ +594,3 @@
+            keyval != Gdk.KEY_KP_Page_Up &&
+            keyval != Gdk.KEY_Home &&
+            keyval != Gdk.KP_Key_Home &&

Gdk.KP_Key_Home is undefined, you want Gdk.KEY_KP_Home (but you also need to handle it!)

@@ +598,3 @@
             return Gdk.EVENT_PROPAGATE;
 
+        if (keyval === Gdk.KEY_Home) {

Style nit: no braces for single-line blocks

@@ +599,3 @@
 
+        if (keyval === Gdk.KEY_Home) {
+            this.vadjustment.value = 0;

Better (because animated):
this._view.emit('move-cursor', Gtk.MovementStep.BUFFER_ENDS, -1);

@@ +602,3 @@
+        }
+        else if (keyval === Gdk.KEY_End) {
+            this.vadjustment.value = this.vadjustment.get_upper();

this._view.emit('move-cursor', Gtk.MovementStep.BUFFER_ENDS, 1);

@@ +607,1 @@
         return this._fetchBacklog();

This is run for *all* keys not excluded above. You can make a point that Home should fetch the backlog when we're already scrolled to the top, but in general Home and End should not load more messages.

Given that you are already special-casing Home and End, I think it would be simpler to write as:

if (keyval == Gdk.KEY_Home ||
    keyval == Gdk.KEY_KP_Home) {
    this._view.emit(...);
    return Gdk.EVENT_STOP;
} else if (keyval == Gdk.KEY_End ||
           keyval == Gdk.KEY_KP_End) {
    this._view.emit(...);
    return Gdk.EVENT_STOP;
}

if (keyval != Gdk.KEY_Up &&
    ...

Created attachment 328904 [details] [review]
chatView: add functionality for Home and End buttons

In commit 755549a, we will add functionality to handle
input from Home and End buttons.

The problem is reproducible when we use either the Home
or End button when in a room.

To fix this, we will add specific functionality to
handle those specific inputs. When we run polari
with this patch, hitting Home and End will result
in expected behaviour, and hitting Home will not
load additional text after consecutive inputs.

Review of attachment 328904 [details] [review]:

Code looks good now with the style nits fixed.

The commit message is still problematic - not least, commit 755549a does not exist (I suspect it is this commit on your local branch, but you won't know the final hash until the patch is pushed upstream). I suggest the following:

chatView: Add shortcuts to jump to top/bottom
    
GtkTextView's built-in bindings - <ctrl>Home and <ctrl>End - don't work,
as they are used by the first-room/last-room application shortcuts. As
we hide the cursor and don't have horizontal scrolling in the chat log,
the built-in bindings for jumping to the beginning/end of the current
line aren't useful, so use them to move to the top/bottom instead.

::: src/chatView.js
@@ +600,3 @@
+            keyval === Gdk.KEY_KP_Home) {
+            this._view.emit('move-cursor', Gtk.MovementStep.BUFFER_ENDS,
+            		    -1, false);

Only use spaces for indentation, no tabs (and most certainly don't mix them like you do here)

@@ +602,3 @@
+            		    -1, false);
+            return Gdk.EVENT_STOP;
+            } else if (keyval === Gdk.KEY_End ||

Indentation

@@ +608,3 @@
+            	       		       1, false);
+            	       return Gdk.EVENT_STOP;
+            	       }

Dto.

Created attachment 329028 [details] [review]
chatView: Add shortcuts to jump to top/bottom

GtkTextView's built-in bindings, <ctrl>Home and <ctrl>End, don't work,
as they are used by the first-room/last-room application shortcuts. As
we hide the cursor and don't have horizontal scrolling in the chat log,
the built-in bindings for jumping to the beginning/end of the current
line aren't useful, so use them to move to the top/bottom of the
chatlog instead.

That *should* sort the indentation. 

I'm not entirely sure what you mean by first-room/last-room application?

Review of attachment 329028 [details] [review]:

Mostly good, but:
 - should be squashed with the previous patch
 - there's on indentation issue left:

::: jhbuild/checkout/polari/src/chatView.js
@@ +604,3 @@
+        } else if (keyval === Gdk.KEY_End ||
+                   keyval === Gdk.KEY_KP_End) {
+                   this._view.emit('move-cursor',

I know this is a bit confusing because of the aligned subconditions, but the else block is misaligned:

 if (foo)
     foo();
  else if (bar)
         bar();

(In reply to Danny Mølgaard from comment #7)
> I'm not entirely sure what you mean by first-room/last-room application?

Not application, application *shortcuts*[0]. The usual keybindings for jumping to the top/bottom of the buffer are <ctrl>Home/<ctrl>End (try for instance the "TextView" -> "Multiple Views" example in gtk3-demo), but those are shadowed by the above shortcuts that take precedence over widget keybindings.

Another fix would be to simply use a different key combo for our shortcuts, but:
 (1) <ctrl>Home/<ctrl>End are a pretty logical addition to
     <ctrl>PgUp/<ctrl>PgDown
 (2) Home/End without modifiers are unused as explained in
     the commit message

[0] https://git.gnome.org/browse/polari/tree/src/application.js#n107

Oh right, it's double indented. For the love of me I couldn't see that. Which patch do you want me to squash the changes on?

(In reply to Danny Mølgaard from comment #10)
> Oh right, it's double indented. For the love of me I couldn't see that.
> Which patch do you want me to squash the changes on?

By this I mean, since I didn't squash the previous one, which of the two current ones would you like me to admit the changes to?

(In reply to Danny Mølgaard from comment #11)
> By this I mean, since I didn't squash the previous one, which of the two
> current ones would you like me to admit the changes to?

I mean there should be a single patch that applies to master. So unless you have worked on other stuff as well, running "git rebase -i origin/master" and changing all "pick" from the second to last line to "squash" should do the job ...

Created attachment 329039 [details] [review]
chatView: Add shortcuts to jump to top/bottom

GtkTextView's built-in bindings, <ctrl>Home and <ctrl>End, don't work,
as they are used by the first-room/last-room application shortcuts. As
we hide the cursor and don't have horizontal scrolling in the chat log,
the built-in bindings for jumping to the beginning/end of the current
line aren't useful, so use them to move to the top/bottom of the
chatlog instead.

How's this? using "git rebase -i origin/master" resulted in two patches where I squased the second one. Now there's only one, which I just attached.

I've no clue if it did the trick, so I apologize in advance if it made no difference.

Comment on attachment 328904 [details] [review]
chatView: add functionality for Home and End buttons

(In reply to Danny Mølgaard from comment #14)
> I've no clue if it did the trick

It did, thanks. The indentation is still slightly off (2nd block uses 6 spaces instead of 4), but I can just fix that up when pushing.

Congratulations on your first contribution :-)

Attachment 329039 [details] pushed as 3bb9e3c - chatView: Add shortcuts to jump to top/bottom