I'm working on this bug.

Created attachment 317650 [details] [review]
Load more history (backlog) through keyboard keys

Should be able to fetch backlog even when keyboard navigation keys (PageUp or Up) are used.

This fix enables backlog fetching through key-press events.

Review of attachment 317650 [details] [review]:

Minor style nits apart, the code looks good to me.

The commit message could be improved though. May I suggest something along the lines of:

chatView: Also load history when scrolling via keyboard                         
                                                                                
When available, we fetch more messages from the history when scrolling          
past the top of the view. However as we currently only consider scroll          
events, this will only work when scrolling with a pointer device; also          
handle appropriate key events to make the functionality available via
keyboard as well.

::: src/chatView.js
@@ +313,3 @@
         this.widget.vadjustment.connect('changed',
                                  Lang.bind(this, this._updateScroll));
+        this._view.connect('key-press-event', Lang.bind(this ,this._onKeyPress));

Style nit:
space between parameters should follow the comma, not precede it.

@@ +459,3 @@
+        let [, keyval] = event.get_keyval();
+        if (keyval != Gdk.KEY_Up && keyval != Gdk.KEY_KP_Up && 
+            keyval != Gdk.KEY_Page_Up && keyval != Gdk.KEY_KP_Page_Up)

This is a checking one variable for any of the values we intend to handle, i.e. the operands are completely commutative (unlike a condition like "if (foo != null && foo.matches('bar'))"). In those cases I consider each condition on a separate line more readable:
  if (keyval != Gdk.KEY_Up &&
      keyval != Gdk.KEY_UP_Up &&
      keyval != ...

Review of attachment 317650 [details] [review]:

::: src/chatView.js
@@ +459,3 @@
+        let [, keyval] = event.get_keyval();
+        if (keyval != Gdk.KEY_Up && keyval != Gdk.KEY_KP_Up && 
+            keyval != Gdk.KEY_Page_Up && keyval != Gdk.KEY_KP_Page_Up)

I do not think this is a good approach. E.g. I immediately can think of another button (Home) that also scrolls up but isn’t handled here. Who knows how many more are there? 

Maybe you should just connect to scroll position change event and see if it is near-top instead? Or rather, exactly why

    this.widget.vadjustment.connect('changed',

doesn’t fire/work for keyboard scroll?

(In reply to Simonas Kazlauskas from comment #4)
> I do not think this is a good approach. E.g. I immediately can think of
> another button (Home) that also scrolls up but isn’t handled here.

We are interested in scrolling *past* the top, not *to* the top (which is what Home does). 


> Who knows how many more are there?

GTK+ is free software, so anyone who takes a good look at the code[0].


> exactly why
> 
>     this.widget.vadjustment.connect('changed',
> 
> doesn’t fire/work for keyboard scroll?

Because scrolling changes the adjustment's value, which is the one property not covered by the GtkAdjustment::changed signal[1]. You are probably thinking of GtkAdjustment::value-changed, which we could indeed use to detect scrolling to the top, but not past that. While that difference isn't too important by itself, we do need to handle the corresponding scroll-events anyway to suppress GTK's overshoot indication while there's more history to load. And while less obvious, there's a similar indication for keyboard scrolling as well - trying to arrow-up past the top ends in a call to gtk_widget_keynav_failed() and gtk_widget_error_bell() ...



[0] https://git.gnome.org/browse/gtk+/tree/gtk/gtktextview.c#n1440
[1] https://developer.gnome.org/gtk3/stable/GtkAdjustment.html#GtkAdjustment-changed

(In reply to Florian Müllner from comment #5)
> (In reply to Simonas Kazlauskas from comment #4)
> > I do not think this is a good approach. E.g. I immediately can think of
> > another button (Home) that also scrolls up but isn’t handled here.
> 
> We are interested in scrolling *past* the top, not *to* the top (which is
> what Home does).

The analog pointer operation is grabbing the scrollbar and dragging it all the way to the top - no history loading either

(In reply to Florian Müllner from comment #5)
> We are interested in scrolling *past* the top, not *to* the top (which is
> what Home does). 

(In reply to Florian Müllner from comment #6)
> The analog pointer operation is grabbing the scrollbar and dragging it all
> the way to the top - no history loading either

I guess all this really demonstrates is that touchscreen/phone interactions don’t really work on desktop. What about repeatedly mashing Home while it is already at top? You could argue this also could count as scrolling “past the top” even if technically it isn’t (just like PgUp and ↑).

(In reply to Simonas Kazlauskas from comment #7)
> (In reply to Florian Müllner from comment #6)
> > The analog pointer operation is grabbing the scrollbar and dragging it all
> > the way to the top - no history loading either
> 
> I guess all this really demonstrates is that touchscreen/phone interactions
> don’t really work on desktop.

Touchscreen/phone interactions like grabbing and dragging a scrollbar?


> What about repeatedly mashing Home while it is already at top?

Doesn't make sense to me. Consider room list navigation: You can use <alt>up/down to switch channels, and you could make a point that hitting <alt>up when at the top of the list should wrap around to the end. However when using <alt>1 to jump to the top room directly, I don't see how any other action would make sense no matter how often the shortcut is repeated.

(In reply to Florian Müllner from comment #8)
> Touchscreen/phone interactions like grabbing and dragging a scrollbar?

No, the scrolling past the top to reload. The kind of thing you see in almost every Android application.

> Doesn't make sense to me. Consider room list navigation: You can use
> <alt>up/down to switch channels, and you could make a point that hitting
> <alt>up when at the top of the list should wrap around to the end. However
> when using <alt>1 to jump to the top room directly, I don't see how any
> other action would make sense no matter how often the shortcut is repeated.

That’s true. However, (hypothetically, since I don’t really want do do that) in this case I could also argue that Home should/could show the very first message in the history available rather than the oldest already loaded message.

(In reply to Simonas Kazlauskas from comment #9)
> That’s true. However, (hypothetically)

Well, the whole Home-key issue is completely hypothetical anyway, given that Home will jump to the beginning of the line and <ctrl>Home (which would jump to the start of the buffer) is overloaded by the shortcut to switch to the first room ...

Created attachment 317739 [details] [review]
chatView: Also load history when scrolling via keyboard

Should be able to fetch backlog even when keyboard navigation keys (PageUp or Up) are used.

When available, we fetch more messages from the history when scrolling past the top of the view. However as we currently only consider scroll events, this will only work when scrolling with a pointer device; This fix adds appropriate key events handling to make the functionality also available via keyboard.

Review of attachment 317739 [details] [review]:

Thanks!

Note that git-log doesn't do automatic line-wrapping, so the commit message should be wrapped manually at around 80 characters. And semicolons don't start a new sentence, so no capitalization there - I don't think it's worth doing another revision there though, so I'll just fix those up and push.

::: src/chatView.js
@@ +458,3 @@
+    _onKeyPress: function(w, event) {
+        let [, keyval] = event.get_keyval();
+        if (keyval != Gdk.KEY_Up && 

Nit: trailing whitespace

Attachment 317739 [details] pushed as 4520171 - chatView: Also load history when scrolling via keyboard