i can't reproduce this... can you clarify more specifically exactly what you're doing?

The described problem happens when you have an existing conversation that has a scrollbar. You allow the notification to pop down, and then receive a new message for the conversation and expand the notification again. The scrollbar doesn't move to the bottom to show the latest message. I'm attaching a screenshot that demonstrates that.

I'm not sure why this.scrollTo(St.Side.BOTTOM); in Notification::_append() in telepathyClient.js doesn't do the trick - perhaps because it is called while the notification is in the banner mode, but I don't know what exactly happens with the allocations in that case. Calling this.scrollTo(St.Side.BOTTOM); in Notification::popOut() or in MessageTray::_expandNotification() fixes this problem, but I don't know if either one is the best solution or more of a band-aid.

Created attachment 158776 [details]
The scrollbar doesn't move to the bottom to show the latest message, which is 'hello'

After the fix, in current master I am experiencing the opposite problem: if I am currently reading past messages and a new one arrives, I am immediately scrolled to the bottom, so I need to go up to catch up, then I get a new message again, I scroll up again, etc.

Various designs are possible, I believe:

1) Always scroll.
The current solution, leads to buggy situation.

2) Never scroll.
Requires scrolling manually, but ensures that you are not automatically scrolled away from what you were reading.

3) Scroll when the notification is popped out.
Still requires manual scrolling if you receive multiple messages (and possibly answer to them), without closing the notification.

4) Scroll when sending a message.
Does not resolve the problem of incoming messages.

5) Keep an "allow-scroll" flag that is set on notification pop out and outgoing message, and cleared when scrolling manually.
Solves most of use cases, but does not allow peeking some old messages and then going back to the most recent ones without closing and reopening.

6) Scroll if and only if the scrollarea is at the end when appending the new message.
The most complete solution, matches behaviour of empathy (and other applications using similar concepts, like gnome-terminal), but requires knowing the scrolling position.

(In reply to comment #4)
> 6) Scroll if and only if the scrollarea is at the end when appending the new
> message.

Yup

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

Created attachment 178878 [details] [review]
Scroll down when notifying for new messages

The scrolling action in the appendMessage function scrolls to the bottom of the notification as it was last time it was shown.
Scrolling down after showing it solved this problem

Oh, I think I totally misunderstood the "always scrolling" problem ...
But, correct me if I'm wrong :
In current master, when the notification is visible (ie not reduced), the scroll is automatic too, isn't it ?
Given that, is it better to get a fully scrolled down popup that you might have to scroll up to read past message if you recieved a lot of them (like with the patch), or to always have to scroll down by yourself ?

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

See my analysis in https://bugzilla.gnome.org/show_bug.cgi?id=640337#c5 for my best understanding of the problem.

We should also call scrollTo() in Notification::_appendTimestamp() and Notification::apprendPresence() in telepathyClient.js or find some way to unify these calls.

I first thought we should just add scrollTo() in Notification::addActor() in messageTray.js, but that would not be right for other types of notifications. We want the scrollbar to stay on top if the notification body is too long and results in a scrollbar. (The very long single chat message that takes up the whole scroll area and ends up with a scrollbar at the bottom is a corner case we can ignore.)

(In reply to comment #10)
> See my analysis in https://bugzilla.gnome.org/show_bug.cgi?id=640337#c5 for my
> best understanding of the problem.

I looked into this and it seems to be more complicated than that. The pick (or a clutter_get_allocation_box call) does not propagate down to the StBoxLayout and thus the adjustment does not get updated when it (the notification) isn't mapped (visible).

Not sure what the correct solution is but we could either connect to allocation-changed or adjustment::changed and call scrollTo in response to that and disconnect the signal immediately afterwards, this way we can be sure that the message has been added and that the allocation happened. (similar to the style-changed hack I did in the other bug).

But maybe there is a better fix that can be done inside scrollTo and not having the caller hackaround it. (Can't really think of anything though that would work without assuming behaviour / policy inside scrollTo).

(In reply to comment #12)
> (In reply to comment #10)
> > See my analysis in https://bugzilla.gnome.org/show_bug.cgi?id=640337#c5 for my
> > best understanding of the problem.
> 
> I looked into this and it seems to be more complicated than that. The pick (or
> a clutter_get_allocation_box call) does not propagate down to the StBoxLayout
> and thus the adjustment does not get updated when it (the notification) isn't
> mapped (visible).
> 
> Not sure what the correct solution is but we could either connect to
> allocation-changed or adjustment::changed and call scrollTo in response to that
> and disconnect the signal immediately afterwards, this way we can be sure that
> the message has been added and that the allocation happened. (similar to the
> style-changed hack I did in the other bug).

This sounds very complicated/hacky. Maybe we can just remember that we need to scroll the bottom and do it when we show the notification? I guess the notification might not be immediately shown at the point of the scrollTo call if there is another notification being shown then with higher priority.

Created attachment 182952 [details] [review]
notification: Remove scrollTo hack

Remove the hack from Notification.scrollTo because it is unreliable,
the caller should make sure to call scrollTo when it will actually
have the desired effect.

Created attachment 182953 [details] [review]
telepathyClient: Pin the scrollbar to the bottom

When new messages come in we want to scroll down so that the user
sees the incoming messages. The current implementation does not work
because it relyies on a synchron allocation hack which does not for
unmapped notifications.

Fix that by connecting to adjustment::changed and scroll whenever the
adjustment changes which equals
"new messages/new timestamp/presense change".

Comment on attachment 182952 [details] [review]
notification: Remove scrollTo hack

ok to commit once the other patch is committed

Comment on attachment 182953 [details] [review]
telepathyClient: Pin the scrollbar to the bottom

>because it relyies on a synchron allocation hack which does not for
>unmapped notifications.

relies, synchronous, "does not work"

>"new messages/new timestamp/presense change".

presence

>+    addActor: function(actor, style) {
>+        MessageTray.Notification.prototype.addActor.call(this, actor, style);
>+        if (this._onScrollAdjustmentChangedId == 0) {
>+            this._onScrollAdjustmentChangedId = this._scrollArea.vscroll.adjustment.connect('changed',
>+                Lang.bind(this, function() {
>+                        this.scrollTo(St.Side.BOTTOM);
>+                }));
>+        }
>     },

I think it would make more sense to split the _scrollArea creation out of addActor into a separate (private) method, and then Telepathy.Notification can just call that method from its _init(). Then you can connect to the signal from there, rather than having to wait until after the first child is added.

It might also be nice to make it only autoscroll if it was already at the bottom (and not if the user has scrolled back to see something else). You could do that by recording the max value each time and only re-scrolling if the current value is the old max.

Created attachment 183011 [details] [review]
telepathyClient: Pin the scrollbar to the bottom

When new messages come in we want to scroll down so that the user
sees the incoming messages. The current implementation does not work
because it relies on a synchronous allocation hack which does not for
work unmapped notifications.

Fix that by connecting to adjustment::changed and scroll whenever the
adjustment changes which equals "new messages", "new timestamp" or
"presense change", but don't interference with the user's scroll actions
i.e when the user scrolls back to read something don't scroll to the bottom.

Created attachment 183013 [details] [review]
telepathyClient: Pin the scrollbar to the bottom

When new messages come in we want to scroll down so that the user
sees the incoming messages. The current implementation does not work
because it relies on a synchronous allocation hack which does not work
for unmapped notifications.

Fix that by connecting to adjustment::changed and scroll whenever the
adjustment changes which equals "new messages", "new timestamp" or
"presense change", but don't interference with the user's scroll actions
i.e when the user scrolls back to read something don't scroll to the bottom.

---

Fix yet another typo in the commit message ...

Comment on attachment 183013 [details] [review]
telepathyClient: Pin the scrollbar to the bottom

looks good

Attachment 182952 [details] pushed as dc2cae0 - notification: Remove scrollTo hack
Attachment 183013 [details] pushed as a0a8342 - telepathyClient: Pin the scrollbar to the bottom