Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mumble log scrolls to top instead of bottom on some Qt versions (4.8, 5.2, maybe more?) #2504

Closed
mkrautz opened this issue Aug 8, 2016 · 18 comments · Fixed by #6290
Closed

Comments

@mkrautz
Copy link
Contributor

mkrautz commented Aug 8, 2016

From IRC:

22:27:27 <jemu> hiho, it seems that since some newer snapshot versions of mumble the chat scrolls to the top (as opposed to the bottom) when an image is posted. is this a known problem? is it maybe related to some older qt5 versions?
22:29:03 <@mkrautz> jemu: where are you seeing it?
22:29:57 <jemu> mkrautz: two people have reported this to me, one is one ubuntu trusty + snapshot ppa the other on windows 7 + official snapshot build
22:30:46 <jemu> mkrautz: sorry, i confused something, both are linux (one is ubutnu trusty + snapshot ppa, the other is a debian system)
22:31:03 <jemu> it works perfectly fine on my system (gentoo + manually build)
22:33:01 <jemu> i couldn't find a bug report, only some older ones with the opposite issue (not scrolling at all)
22:33:02 <@mkrautz> is the debian system using debian's own package?
22:33:13 <@mkrautz> if so, that would probably have the bug
22:33:29 <jemu> the debian user has done a custom build from git master
22:33:57 <jemu> the ubuntu user has: 1.3.0 (1.3.0~1326~gf4ca0cf~snapshot-1~ppa1~trusty1)
22:34:40 <@mkrautz> OK
22:34:48 <@mkrautz> seems like it's possibly an issue with older versions of Qt then, hmm...
22:34:58 <@mkrautz> jemu: that's pretty old
22:35:36 <@mkrautz> (not that anything should have changed in that regard, but ... it's still a bit oldold)
22:35:41 <@mkrautz> eh, old* :)
22:35:58 <@mkrautz> OK, trust has Qt 5.2
22:36:05 <@mkrautz> and the Debian user is... maybe on Qt 4?
22:37:14 <jemu> mkrautz: yes, the debian user is still on Qt 4
22:37:41 <@mkrautz> OK, I would expect the Qt 4 to be broken in that regard, actually :(
22:37:50 <@mkrautz> the Qt 4 version*
22:38:04 <@mkrautz> I seem to be missing a lot of words today :)
22:38:07 <jemu> good (or bad, depends on context), what about Qt 5.2?
22:38:27 <@mkrautz> jemu: I have no idea about that :(
22:41:21 <jemu> mkrautz: we have updated to 1508 on the ubuntu system, issue persists
22:41:38 <jemu> so maybe we should update from trusty to xenial?
22:41:54 <@mkrautz> jemu: well, it's a bug I'd like to fix... so :)
22:42:15 <jemu> ok, something we can do to help?
22:42:56 <@mkrautz> jemu: if the debian user could try to build 1.2.16 w/ qt 4 and check if that works, that would help :)
@mkrautz mkrautz added this to the 1.3.0 milestone Aug 8, 2016
@mkrautz
Copy link
Contributor Author

mkrautz commented Aug 8, 2016

For Qt 4, we should probably forward-port

192ba96
1a4ae8d

to master from 1.2.x.

@promi
Copy link

promi commented Aug 9, 2016

The issue is fixed for the trusty user with the version from today

2aec53d -> package version 1.3.01533g2aec53dsnapshot-1ppa1~trusty1

While it was still present yesterday with this version

e7ff17b -> package version 1.3.01508ge7ff17bsnapshot-1ppa1~trusty1

@promi
Copy link

promi commented Aug 9, 2016

That is kind of curious as there don't seem to be any changes to the chat between those commits:

e7ff17b...2aec53d

@mkrautz
Copy link
Contributor Author

mkrautz commented Aug 9, 2016

That's correct. No changes that I'd expect to affect this.

@promi
Copy link

promi commented Aug 24, 2016

The error is back for the trusty user (still version 1533), so the problem was not fixed between e7ff17b...2aec53d.

It also doesn't always happen as we first thought.

@mkrautz mkrautz modified the milestones: 1.4.0, 1.3.0 Nov 25, 2016
@promi
Copy link

promi commented Jun 24, 2019

The error is now also present on my system (Gentoo Linux), a user with Debian GNU/Linux and a third user with ArchLinux.

Its also present in the current Git version.

When I restart Mumble it works for a while, but after some time it starts to scroll to the top or near the top instead of the bottom where it should.

@promi
Copy link

promi commented Jun 24, 2019

I don't think the Qt version matters, but for completeness, my system has Qt 5.11.3-r2.

The ArchLinux system has 5.12.

@promi
Copy link

promi commented Jun 30, 2019

I have done some testing and debugging on this.

After starting Mumble everything is fine, up until a point where about 5000 lines are in the log widget. The scroll mechanism then scrolls more towards to the middle than towards the bottom. Clearing the log doesn't help either.

The log widget (LogTextBrowser) is a class derived from QTextBrowser in CustomElements.h.

When a new log message is written to the log widget the scrollLogToBottom method is called. There is some extra code in Log.cpp that detects whether the scroll position was already at the bottom, but that works as expected.

All that the scrollLogToBottom (in CustomElements.cpp) does is essentially this:

verticalScrollBar()->setValue(verticalScrollBar()->maximum());

This should work just fine and it does initially, but stops working after a while (see above).

As far as I can see this is an upstream bug in Qt. I can't find an upstream bug report though. The thing is I haven't written a Qt application before, so I can't easilly create a test application.

Maybe someone would like to supply a test application? I currently don't have enough free time, but may get back to this later.

@davidebeatrici
Copy link
Member

Thank you very much for investigating the issue!

I can write a test application, no problem.

@Krzmbrzl
Copy link
Member

@davidebeatrici (or anyone else) do you know whether this bug has been reported in the meantime?

In the meatime I'll close this issue as apparently there's nothing we can do on our side to fix this (besides some dirty workarounds maybe but I'd vote against that)...

@promi
Copy link

promi commented Apr 17, 2020

I have created an upstream bug request here:

https://bugreports.qt.io/browse/QTBUG-83622

@Krzmbrzl
Copy link
Member

@promi Thank you :)

@promi
Copy link

promi commented Dec 17, 2021

I have a bit more information on this:

When the bug occurs you can resize the chat edit by pressing Shift + Enter (entering a newline) and the log then scrolls to the bottom again. When a new chat message is received it stops working again. You can repeat this by adding / removing new lines so that the chat edit resizes.

I suspect that the problem has something to do with the layout as well as the QTextBrowser.

@philippbaumm
Copy link

just ran into this issue in my work related project(pyqt6 tho):
https://www.qtcentre.org/threads/32852-How-can-I-always-keep-the-scroll-bar-at-the-bottom-of-a-QScrollArea

connecting the rangeChanged signal worked for me!

@Irets
Copy link

Irets commented Jul 20, 2023

Is there a way to hack the fix into QT somehow (if so, what needs to be changed?) or do we have to wait for QT to fix this?

@Krzmbrzl
Copy link
Member

Krzmbrzl commented Jul 20, 2023

Well - it will certainly not be fixed in Qt5 anymore. Maybe it's fixed in Qt6, but we'll only see once we port Mumble to it.
Other than that, Qt is open source, so technically you could contribute a bug fix to Qt yourself instead of waiting on them to fix it on their own 👀

@Irets
Copy link

Irets commented Jul 20, 2023

I could look into it but I don't have very high hopes.
How far are we from Mumble on QT6?

@Krzmbrzl
Copy link
Member

How far are we from Mumble on QT6?

Before we can start to work on that, we'll have to get Mumble to compile with C++17. Then we can start porting.
We have not yet (really) started with this though.

dexgs added a commit to dexgs/mumble that referenced this issue Dec 28, 2023
When the chat log is detected to be scrolled all the way to the bottom,
it is set to the bottom again when new entries are added to the chat
log.

The detection works properly, but sometimes (especially when an image is
pasted into the chat box) the scroll bar ends up in an incorrect
position.

When the chat log ends up in this incorrect state, subsequent calls to
`scrollLogToBottom` still work correctly, i.e. resizing the window
(which triggers this function) will correctly set the chat log scroll bar
to the bottom.

Therefore, the issue seems to be caused by the scrollbar sometimes being
in an inconsistent state *immediately* after a message is sent, but not
any longer.

My workaround is to add a call to `QApplication::processEvents` before
setting the scrollbar to its maximum position to avoid the inconsistent
state.

As discussed before, this seems to be an upstream bug in Qt, but my
workaround nevertheless seems to fix the issue in my testing.

I have not been able to reproduce the bug locally since applying my
changes.

Fixes mumble-voip#2504
dexgs added a commit to dexgs/mumble that referenced this issue Dec 29, 2023
When the chat log is detected to be scrolled all the way to the bottom,
it is set to the bottom again when new entries are added to the chat
log.

The detection works properly, but sometimes (especially when an image is
pasted into the chat box) the scroll bar ends up in an incorrect
position.

When the chat log ends up in this incorrect state, subsequent calls to
`scrollLogToBottom` still work correctly, i.e. resizing the window
(which triggers this function) will correctly set the chat log scroll bar
to the bottom.

Therefore, the issue seems to be caused by the scrollbar sometimes being
in an inconsistent state *immediately* after a message is sent, but not
any longer.

My workaround is to add a call to `QApplication::processEvents` before
setting the scrollbar to its maximum position to avoid the inconsistent
state.

As discussed before, this seems to be an upstream bug in Qt, but my
workaround nevertheless seems to fix the issue in my testing.

I have not been able to reproduce the bug locally since applying my
changes.

Fixes mumble-voip#2504
dexgs added a commit to dexgs/mumble that referenced this issue Dec 29, 2023
When the chat log is detected to be scrolled all the way to the bottom,
it is set to the bottom again when new entries are added to the chat
log.

The detection works properly, but sometimes (especially when an image is
pasted into the chat box) the scroll bar ends up in an incorrect
position.

When the chat log ends up in this incorrect state, subsequent calls to
`scrollLogToBottom` still work correctly, i.e. resizing the window
(which triggers this function) will correctly set the chat log scroll bar
to the bottom.

Therefore, the issue seems to be caused by the scrollbar sometimes being
in an inconsistent state *immediately* after a message is sent, but not
any longer.

My workaround is to add a call to `QApplication::processEvents` before
setting the scrollbar to its maximum position to avoid the inconsistent
state.

The call to `scrollLogToBottom` is removed from the resize event handler
on LogTextBrowser because calling `processEvents` from within an event
handler causes a stack overflow.

As discussed before, this seems to be an upstream bug in Qt, but my
workaround nevertheless seems to fix the issue in my testing.

I have not been able to reproduce the bug locally since applying my
changes.

Fixes mumble-voip#2504
dexgs added a commit to dexgs/mumble that referenced this issue Dec 29, 2023
Don't call `scrollLogToBottom` in the chat log's resize event handler.
This is done for two reasons:

First, the user should be able to resize their chat log without losing
their scrollbar position.

Second, it allows a simpler fix to mumble-voip#2504. The fix entails adding a call
to `QApplication::processEvents` in scrollLogToBottom, but calling this
function from within an event handler causes an endless recursion and
therefore a stack overflow.

This change eliminates an edge case that would have to be accounted for
in the fix.
dexgs added a commit to dexgs/mumble that referenced this issue Dec 29, 2023
When the chat log is detected to be scrolled all the way to the bottom,
it is set to the bottom again when new entries are added to the chat
log.

The detection works properly, but sometimes (especially when an image is
pasted into the chat box) the scroll bar ends up in an incorrect
position.

When the chat log ends up in this incorrect state, subsequent calls to
`scrollLogToBottom` still work correctly, i.e. resizing the window
(which triggers this function) will correctly set the chat log scroll bar
to the bottom.

Therefore, the issue seems to be caused by the scrollbar sometimes being
in an inconsistent state *immediately* after a message is sent, but not
any longer.

My workaround is to add a call to `QApplication::processEvents` before
setting the scrollbar to its maximum position to avoid the inconsistent
state.

As discussed before, this seems to be an upstream bug in Qt, but my
workaround nevertheless seems to fix the issue in my testing.

I have not been able to reproduce the bug locally since applying my
changes.

Fixes mumble-voip#2504
dexgs added a commit to dexgs/mumble that referenced this issue Dec 29, 2023
The previous implementation of `resizeEvent` included only a call to
`scrollLogToBottom` before calling into the default `resizeEvent`
implementation.

The re-implementation of the method is removed for the following
reasons:

First, the user should be able to resize their chat log without losing
their scrollbar position.

Second, it allows a simpler fix to mumble-voip#2504. The fix entails adding a call
to `QApplication::processEvents` in scrollLogToBottom, but calling this
function from within an event handler causes an endless recursion and
therefore a stack overflow.

This change eliminates an edge case that would have to be accounted for
in the fix.
dexgs added a commit to dexgs/mumble that referenced this issue Dec 29, 2023
When the chat log is detected to be scrolled all the way to the bottom,
it is set to the bottom again when new entries are added to the chat
log.

The detection works properly, but sometimes (especially when an image is
pasted into the chat box) the scroll bar ends up in an incorrect
position.

When the chat log ends up in this incorrect state, subsequent calls to
`scrollLogToBottom` still work correctly, i.e. resizing the window
(which triggers this function) will correctly set the chat log scroll bar
to the bottom.

Therefore, the issue seems to be caused by the scrollbar sometimes being
in an inconsistent state *immediately* after a message is sent, but not
any longer.

My workaround is to add a call to `QApplication::processEvents` before
setting the scrollbar to its maximum position to avoid the inconsistent
state.

As discussed before, this seems to be an upstream bug in Qt, but my
workaround nevertheless seems to fix the issue in my testing.

I have not been able to reproduce the bug locally since applying my
changes.

Fixes mumble-voip#2504
dexgs added a commit to dexgs/mumble that referenced this issue Jan 15, 2024
The previous implementation of `resizeEvent` included only a call to
`scrollLogToBottom` before calling into the default `resizeEvent`
implementation.

The re-implementation of the method is removed for the following
reasons:

First, the user should be able to resize their chat log without losing
their scrollbar position.

Second, it allows a simpler fix to mumble-voip#2504. The fix entails adding a call
to `QApplication::processEvents` in scrollLogToBottom, but calling this
function from within an event handler causes an endless recursion and
therefore a stack overflow.

This change eliminates an edge case that would have to be accounted for
in the fix.
dexgs added a commit to dexgs/mumble that referenced this issue Jan 15, 2024
When the chat log is detected to be scrolled all the way to the bottom,
it is set to the bottom again when new entries are added to the chat
log.

The detection works properly, but sometimes (especially when an image is
pasted into the chat box) the scroll bar ends up in an incorrect
position.

When the chat log ends up in this incorrect state, subsequent calls to
`scrollLogToBottom` still work correctly, i.e. resizing the window
(which triggers this function) will correctly set the chat log scroll bar
to the bottom.

Therefore, the issue seems to be caused by the scrollbar sometimes being
in an inconsistent state *immediately* after a message is sent, but not
any longer.

My workaround is to add a call to `QApplication::processEvents` before
setting the scrollbar to its maximum position to avoid the inconsistent
state.

As discussed before, this seems to be an upstream bug in Qt, but my
workaround nevertheless seems to fix the issue in my testing.

I have not been able to reproduce the bug locally since applying my
changes.

Fixes mumble-voip#2504
dexgs added a commit to dexgs/mumble that referenced this issue Jan 15, 2024
In my previous PR (mumble-voip#6290) addressing this bug, I wrote:
> The issue seems to be caused by the scrollbar sometimes being in an
> inconsistent state immediately after a message is sent, but not any
> longer.
>
> My workaround is to add a call to QApplication::processEvents before
> setting the scrollbar to its maximum position to avoid the inconsistent
> state.

I have found that this change only reduces the likelihood of the
incorrect scroll behaviour from happening: it is still reproducible
after the log grows past a certain size.

In my testing since then, the cause of the "inconsistent state" described
previously seems to be the following: When images (whose contents are
stored in a base64-encoded data URL) are inserted into the log, they are
not loaded immediately. Instead, they are "fetched" (although no actual
network request occurs) by the `loadResource` implementation on
`LogDocument`. The "fetching" is triggered by the call to `setHtml` in
the `Log::validHtml` method. However, the code which sets the scroll bar
to stay at the bottom runs immediately (i.e. not asynchronously) in the
`Log::log` method.

The intended behaviour appears to be that when a log entry is added the
log will scroll to the bottom if the entry is a message sent by the user
themselves, OR if the log was already scrolled to the bottom before
the entry was added.

The previous solution in place addressing the asynchronous image loading
was dispatching a `LogDocumentResourceAddedEvent` when the image is
loded whose handler scrolls the log to the bottom, but this causes 2
problems:

First, the handler for this event always scrolls the log to the bottom
no matter what, which means messages containing images are handled
differently from the intended scrolling behaviour, i.e. messages
containing images ALWAYS cause the log to scroll to the bottom instead
of obeying the rules described above.

Second, it seemingly causes the issue described in mumble-voip#2504. It looks like
there's a race condition when the scroll position is set from the main
thread as well as in an event handler.

My previous solution (mumble-voip#6290) made the timing line up correctly more often,
but still failed after the log reached a certain size. This is probably
because calling `QApplication::processEvents` doesn't target a specific
event, i.e. there is still a case where the call "misses" and returns
before the image has loaded, resulting in the same incorrect scroll
behaviour.

The solution in this commit is to send an event to scroll the chat log
to the bottom instead of setting the scroll position directly in the
`Log::log` method.

The details of the changes are as follows:

`LogDocumentResourceAddedEvent` is renamed to `LogScrollEvent` and now
contains members `scrollToBottom` and `scrollPosition` so that the event
handler can maintain the same intended scrolling behaviour described
above.

`LogScrollEvent` is posted from the `Log::log` method rather than from
`LogDocument::finished` so that there is a single code path that causes
the log scrolling behaviour AND because `Log::log` can take the value of
`ownMessage` into account rather than always scrolling the log to the
bottom when an image loads. The event is posted after image loading is
initiated and everything gets handled in the correct order in my
testing.

The `getLogScrollMaximum`, `setLogScroll` and `scrollLogToBottom`
methods are all removed, since their functionality has been moved into
the event handler for `LogScrollEvent` and there is no longer any need
to call these functions from outside the `LogTextBrowser` class.

Setting the log to the bottom uses `INT_MAX` instead of the result of
`maximum()` for the scroll bar because Qt already clamps the scroll value
within the legal range so `INT_MAX` will always result in the scroll bar
being set to its current maximum.
(https://doc.qt.io/qt-5/qabstractslider.html#value-prop)

Fixes mumble-voip#2504 (for real this time)
dexgs added a commit to dexgs/mumble that referenced this issue Jan 15, 2024
In my previous PR (mumble-voip#6290) addressing this bug, I wrote:
> The issue seems to be caused by the scrollbar sometimes being in an
> inconsistent state immediately after a message is sent, but not any
> longer.
>
> My workaround is to add a call to QApplication::processEvents before
> setting the scrollbar to its maximum position to avoid the inconsistent
> state.

I have found that this change only reduces the likelihood of the
incorrect scroll behaviour from happening: it is still reproducible
after the log grows past a certain size.

In my testing since then, the cause of the "inconsistent state" described
previously seems to be the following: When images (whose contents are
stored in a base64-encoded data URL) are inserted into the log, they are
not loaded immediately. Instead, they are "fetched" (although no actual
network request occurs) by the `loadResource` implementation on
`LogDocument`. The "fetching" is triggered by the call to `setHtml` in
the `Log::validHtml` method. However, the code which sets the scroll bar
to stay at the bottom runs immediately (i.e. not asynchronously) in the
`Log::log` method.

The intended behaviour appears to be that when a log entry is added the
log will scroll to the bottom if the entry is a message sent by the user
themselves, OR if the log was already scrolled to the bottom before
the entry was added.

The previous solution in place addressing the asynchronous image loading
was dispatching a `LogDocumentResourceAddedEvent` when the image is
loded whose handler scrolls the log to the bottom, but this causes 2
problems:

First, the handler for this event always scrolls the log to the bottom
no matter what, which means messages containing images are handled
differently from the intended scrolling behaviour, i.e. messages
containing images ALWAYS cause the log to scroll to the bottom instead
of obeying the rules described above.

Second, it seemingly causes the issue described in mumble-voip#2504. It looks like
there's a race condition when the scroll position is set from the main
thread as well as in an event handler.

My previous solution (mumble-voip#6290) made the timing line up correctly more often,
but still failed after the log reached a certain size. This is probably
because calling `QApplication::processEvents` doesn't target a specific
event, i.e. there is still a case where the call "misses" and returns
before the image has loaded, resulting in the same incorrect scroll
behaviour.

The solution in this commit is to send an event to scroll the chat log
to the bottom instead of setting the scroll position directly in the
`Log::log` method.

The details of the changes are as follows:

`LogDocumentResourceAddedEvent` is renamed to `LogScrollEvent` and now
contains members `scrollToBottom` and `scrollPosition` so that the event
handler can maintain the same intended scrolling behaviour described
above.

`LogScrollEvent` is posted from the `Log::log` method rather than from
`LogDocument::finished` so that there is a single code path that causes
the log scrolling behaviour AND because `Log::log` can take the value of
`ownMessage` into account rather than always scrolling the log to the
bottom when an image loads. The event is posted after image loading is
initiated and everything gets handled in the correct order in my
testing.

The `getLogScrollMaximum`, `setLogScroll` and `scrollLogToBottom`
methods are all removed, since their functionality has been moved into
the event handler for `LogScrollEvent` and there is no longer any need
to call these functions from outside the `LogTextBrowser` class.

Setting the log to the bottom uses `INT_MAX` instead of the result of
`maximum()` for the scroll bar because Qt already clamps the scroll value
within the legal range so `INT_MAX` will always result in the scroll bar
being set to its current maximum.
(https://doc.qt.io/qt-5/qabstractslider.html#value-prop)

Fixes mumble-voip#2504 (for real this time)
dexgs added a commit to dexgs/mumble that referenced this issue Jan 15, 2024
In my previous PR (mumble-voip#6290) addressing this bug, I wrote:
> The issue seems to be caused by the scrollbar sometimes being in an
> inconsistent state immediately after a message is sent, but not any
> longer.
>
> My workaround is to add a call to QApplication::processEvents before
> setting the scrollbar to its maximum position to avoid the inconsistent
> state.

I have found that this change only reduces the likelihood of the
incorrect scroll behaviour from happening: it is still reproducible
after the log grows past a certain size.

In my testing since then, the cause of the "inconsistent state" described
previously seems to be the following: When images (whose contents are
stored in a base64-encoded data URL) are inserted into the log, they are
not loaded immediately. Instead, they are "fetched" (although no actual
network request occurs) by the `loadResource` implementation on
`LogDocument`. The "fetching" is triggered by the call to `setHtml` in
the `Log::validHtml` method. However, the code which sets the scroll bar
to stay at the bottom runs immediately (i.e. not asynchronously) in the
`Log::log` method.

The intended behaviour appears to be that when a log entry is added the
log will scroll to the bottom if the entry is a message sent by the user
themselves, OR if the log was already scrolled to the bottom before
the entry was added.

The previous solution in place addressing the asynchronous image loading
was dispatching a `LogDocumentResourceAddedEvent` when the image is
loded whose handler scrolls the log to the bottom, but this causes 2
problems:

First, the handler for this event always scrolls the log to the bottom
no matter what, which means messages containing images are handled
differently from the intended scrolling behaviour, i.e. messages
containing images ALWAYS cause the log to scroll to the bottom instead
of obeying the rules described above.

Second, it seemingly causes the issue described in mumble-voip#2504. It looks like
there's a race condition when the scroll position is set from the main
thread as well as in an event handler.

My previous solution (mumble-voip#6290) made the timing line up correctly more often,
but still failed after the log reached a certain size. This is probably
because calling `QApplication::processEvents` doesn't target a specific
event, i.e. there is still a case where the call "misses" and returns
before the image has loaded, resulting in the same incorrect scroll
behaviour.

The solution in this commit is to send an event to scroll the chat log
to the bottom instead of setting the scroll position directly in the
`Log::log` method.

The details of the changes are as follows:

`LogDocumentResourceAddedEvent` is renamed to `LogScrollEvent` and now
contains members `scrollToBottom` and `scrollPosition` so that the event
handler can maintain the same intended scrolling behaviour described
above.

`LogScrollEvent` is posted from the `Log::log` method rather than from
`LogDocument::finished` so that there is a single code path that causes
the log scrolling behaviour AND because `Log::log` can take the value of
`ownMessage` into account rather than always scrolling the log to the
bottom when an image loads. The event is posted after image loading is
initiated and everything gets handled in the correct order in my
testing.

The `getLogScrollMaximum`, `setLogScroll` and `scrollLogToBottom`
methods are all removed, since their functionality has been moved into
the event handler for `LogScrollEvent` and there is no longer any need
to call these functions from outside the `LogTextBrowser` class.

Setting the log to the bottom uses `INT_MAX` instead of the result of
`maximum()` for the scroll bar because Qt already clamps the scroll value
within the legal range so `INT_MAX` will always result in the scroll bar
being set to its current maximum.
(https://doc.qt.io/qt-5/qabstractslider.html#value-prop)

Fixes mumble-voip#2504 (for real this time)
dexgs added a commit to dexgs/mumble that referenced this issue Jan 15, 2024
In my previous PR (mumble-voip#6290) addressing this bug, I wrote:
> The issue seems to be caused by the scrollbar sometimes being in an
> inconsistent state immediately after a message is sent, but not any
> longer.
>
> My workaround is to add a call to QApplication::processEvents before
> setting the scrollbar to its maximum position to avoid the inconsistent
> state.

I have found that this change only reduces the likelihood of the
incorrect scroll behaviour from happening: it is still reproducible
after the log grows past a certain size.

In my testing since then, the cause of the "inconsistent state" described
previously seems to be the following: When images (whose contents are
stored in a base64-encoded data URL) are inserted into the log, they are
not loaded immediately. Instead, they are "fetched" (although no actual
network request occurs) by the `loadResource` implementation on
`LogDocument`. The "fetching" is triggered by the call to `setHtml` in
the `Log::validHtml` method. However, the code which sets the scroll bar
to stay at the bottom runs immediately (i.e. not asynchronously) in the
`Log::log` method.

The intended behaviour appears to be that when a log entry is added the
log will scroll to the bottom if the entry is a message sent by the user
themselves, OR if the log was already scrolled to the bottom before
the entry was added.

The previous solution in place addressing the asynchronous image loading
was dispatching a `LogDocumentResourceAddedEvent` when the image is
loded whose handler scrolls the log to the bottom, but this causes 2
problems:

First, the handler for this event always scrolls the log to the bottom
no matter what, which means messages containing images are handled
differently from the intended scrolling behaviour, i.e. messages
containing images ALWAYS cause the log to scroll to the bottom instead
of obeying the rules described above.

Second, it seemingly causes the issue described in mumble-voip#2504. It looks like
there's a race condition when the scroll position is set from the main
thread as well as in an event handler.

My previous solution (mumble-voip#6290) made the timing line up correctly more often,
but still failed after the log reached a certain size. This is probably
because calling `QApplication::processEvents` doesn't target a specific
event, i.e. there is still a case where the call "misses" and returns
before the image has loaded, resulting in the same incorrect scroll
behaviour.

The solution in this commit is to send an event to scroll the chat log
to the bottom instead of setting the scroll position directly in the
`Log::log` method.

The details of the changes are as follows:

`LogDocumentResourceAddedEvent` is renamed to `LogScrollEvent` and now
contains members `scrollToBottom` and `scrollPosition` so that the event
handler can maintain the same intended scrolling behaviour described
above.

`LogScrollEvent` is posted from the `Log::log` method rather than from
`LogDocument::finished` so that there is a single code path that causes
the log scrolling behaviour AND because `Log::log` can take the value of
`ownMessage` into account rather than always scrolling the log to the
bottom when an image loads. The event is posted after image loading is
initiated and everything gets handled in the correct order in my
testing.

The `getLogScrollMaximum`, `setLogScroll` and `scrollLogToBottom`
methods are all removed, since their functionality has been moved into
the event handler for `LogScrollEvent` and there is no longer any need
to call these functions from outside the `LogTextBrowser` class.

Setting the log to the bottom uses `INT_MAX` instead of the result of
`maximum()` for the scroll bar because Qt already clamps the scroll value
within the legal range so `INT_MAX` will always result in the scroll bar
being set to its current maximum.
(https://doc.qt.io/qt-5/qabstractslider.html#value-prop)

Fixes mumble-voip#2504 (for real this time)
dexgs added a commit to dexgs/mumble that referenced this issue Jan 15, 2024
In my previous PR (mumble-voip#6290) addressing this bug, I wrote:
> The issue seems to be caused by the scrollbar sometimes being in an
> inconsistent state immediately after a message is sent, but not any
> longer.
>
> My workaround is to add a call to QApplication::processEvents before
> setting the scrollbar to its maximum position to avoid the inconsistent
> state.

I have found that this change only reduces the likelihood of the
incorrect scroll behaviour from happening: it is still reproducible
after the log grows past a certain size.

In my testing since then, the cause of the "inconsistent state" described
previously seems to be the following: When images (whose contents are
stored in a base64-encoded data URL) are inserted into the log, they are
not loaded immediately. Instead, they are "fetched" (although no actual
network request occurs) by the `loadResource` implementation on
`LogDocument`. The "fetching" is triggered by the call to `setHtml` in
the `Log::validHtml` method. However, the code which sets the scroll bar
to stay at the bottom runs immediately (i.e. not asynchronously) in the
`Log::log` method.

The intended behaviour appears to be that when a log entry is added the
log will scroll to the bottom if the entry is a message sent by the user
themselves, OR if the log was already scrolled to the bottom before
the entry was added.

The previous solution in place addressing the asynchronous image loading
was dispatching a `LogDocumentResourceAddedEvent` when the image is
loded whose handler scrolls the log to the bottom, but this causes 2
problems:

First, the handler for this event always scrolls the log to the bottom
no matter what, which means messages containing images are handled
differently from the intended scrolling behaviour, i.e. messages
containing images ALWAYS cause the log to scroll to the bottom instead
of obeying the rules described above.

Second, it seemingly causes the issue described in mumble-voip#2504. It looks like
there's a race condition when the scroll position is set from the main
thread as well as in an event handler.

My previous solution (mumble-voip#6290) made the timing line up correctly more often,
but still failed after the log reached a certain size. This is probably
because calling `QApplication::processEvents` doesn't target a specific
event, i.e. there is still a case where the call "misses" and returns
before the image has loaded, resulting in the same incorrect scroll
behaviour.

The solution in this commit is to send an event to scroll the chat log
to the bottom instead of setting the scroll position directly in the
`Log::log` method.

The details of the changes are as follows:

`LogDocumentResourceAddedEvent` is renamed to `LogScrollEvent` and now
contains members `scrollToBottom` and `scrollPosition` so that the event
handler can maintain the same intended scrolling behaviour described
above.

`LogScrollEvent` is posted from the `Log::log` method rather than from
`LogDocument::finished` so that there is a single code path that causes
the log scrolling behaviour AND because `Log::log` can take the value of
`ownMessage` into account rather than always scrolling the log to the
bottom when an image loads. The event is posted after image loading is
initiated and everything gets handled in the correct order in my
testing.

The `getLogScrollMaximum`, `setLogScroll` and `scrollLogToBottom`
methods are all removed, since their functionality has been moved into
the event handler for `LogScrollEvent` and there is no longer any need
to call these functions from outside the `LogTextBrowser` class.

Setting the log to the bottom uses `INT_MAX` instead of the result of
`maximum()` for the scroll bar because Qt already clamps the scroll value
within the legal range so `INT_MAX` will always result in the scroll bar
being set to its current maximum.
(https://doc.qt.io/qt-5/qabstractslider.html#value-prop)

Fixes mumble-voip#2504 (for real this time)
dexgs added a commit to dexgs/mumble that referenced this issue Jan 15, 2024
In my previous PR (mumble-voip#6290) addressing this bug, I wrote:
> The issue seems to be caused by the scrollbar sometimes being in an
> inconsistent state immediately after a message is sent, but not any
> longer.
>
> My workaround is to add a call to QApplication::processEvents before
> setting the scrollbar to its maximum position to avoid the inconsistent
> state.

I have found that this change only reduces the likelihood of the
incorrect scroll behaviour from happening: it is still reproducible
after the log grows past a certain size.

In my testing since then, the cause of the "inconsistent state" described
previously seems to be the following: When images (whose contents are
stored in a base64-encoded data URL) are inserted into the log, they are
not loaded immediately. Instead, they are "fetched" (although no actual
network request occurs) by the `loadResource` implementation on
`LogDocument`. The "fetching" is triggered by the call to `setHtml` in
the `Log::validHtml` method. However, the code which sets the scroll bar
to stay at the bottom runs immediately (i.e. not asynchronously) in the
`Log::log` method.

The intended behaviour appears to be that when a log entry is added the
log will scroll to the bottom if the entry is a message sent by the user
themselves, OR if the log was already scrolled to the bottom before
the entry was added.

The previous solution in place addressing the asynchronous image loading
was dispatching a `LogDocumentResourceAddedEvent` when the image is
loded whose handler scrolls the log to the bottom, but this causes 2
problems:

First, the handler for this event always scrolls the log to the bottom
no matter what, which means messages containing images are handled
differently from the intended scrolling behaviour, i.e. messages
containing images ALWAYS cause the log to scroll to the bottom instead
of obeying the rules described above.

Second, it seemingly causes the issue described in mumble-voip#2504. It looks like
there's a race condition when the scroll position is set from the main
thread as well as in an event handler.

My previous solution (mumble-voip#6290) made the timing line up correctly more often,
but still failed after the log reached a certain size. This is probably
because calling `QApplication::processEvents` doesn't target a specific
event, i.e. there is still a case where the call "misses" and returns
before the image has loaded, resulting in the same incorrect scroll
behaviour.

The solution in this commit is to send an event to scroll the chat log
to the bottom instead of setting the scroll position directly in the
`Log::log` method.

The details of the changes are as follows:

`LogDocumentResourceAddedEvent` is renamed to `LogScrollEvent` and now
contains members `scrollToBottom` and `scrollPosition` so that the event
handler can maintain the same intended scrolling behaviour described
above.

`LogScrollEvent` is posted from the `Log::log` method rather than from
`LogDocument::finished` so that there is a single code path that causes
the log scrolling behaviour AND because `Log::log` can take the value of
`ownMessage` into account rather than always scrolling the log to the
bottom when an image loads. The event is posted after image loading is
initiated and everything gets handled in the correct order in my
testing.

The `getLogScrollMaximum`, `setLogScroll` and `scrollLogToBottom`
methods are all removed, since their functionality has been moved into
the event handler for `LogScrollEvent` and there is no longer any need
to call these functions from outside the `LogTextBrowser` class.

Setting the log to the bottom uses `INT_MAX` instead of the result of
`maximum()` for the scroll bar because Qt already clamps the scroll value
within the legal range so `INT_MAX` will always result in the scroll bar
being set to its current maximum.
(https://doc.qt.io/qt-5/qabstractslider.html#value-prop)

Fixes mumble-voip#2504 (for real this time)
dexgs added a commit to dexgs/mumble that referenced this issue Feb 17, 2024
n my previous PR (mumble-voip#6290) addressing this bug, I wrote:
> The issue seems to be caused by the scrollbar sometimes being in an
> inconsistent state immediately after a message is sent, but not any
> longer.
>
> My workaround is to add a call to QApplication::processEvents before
> setting the scrollbar to its maximum position to avoid the inconsistent
> state.

I have found that this change only reduces the likelihood of the
incorrect scroll behaviour from happening: it is still reproducible
after the log grows past a certain size.

In my testing since then, the cause of the "inconsistent state" described
previously seems to be the following: When images (whose contents are
stored in a base64-encoded data URL) are inserted into the log, they are
not loaded immediately. Instead, they are "fetched" (although no actual
network request occurs) by the `loadResource` implementation on
`LogDocument`. The "fetching" is triggered by the call to `setHtml` in
the `Log::validHtml` method. However, the code which sets the scroll bar
to stay at the bottom runs immediately (i.e. not asynchronously) in the
`Log::log` method. There also seems to be a hack that forces a re-layout
by sending a "font changed" event.

The intended behaviour appears to be that when a log entry is added the
log will scroll to the bottom if the entry is a message sent by the user
themselves, OR if the log was already scrolled to the bottom before
the entry was added.

The previous solution in place addressing the asynchronous image loading
was dispatching a `LogDocumentResourceAddedEvent` when the image is
loded whose handler scrolls the log to the bottom, but this causes 2
problems:

First, the handler for this event always scrolls the log to the bottom
no matter what, which means messages containing images are handled
differently from the intended scrolling behaviour, i.e. messages
containing images ALWAYS cause the log to scroll to the bottom instead
of obeying the rules described above.

Second, it seemingly causes the issue described in mumble-voip#2504. It looks like
there's a race condition when the scroll position is set from the main
thread as well as in an event handler. I couldn't get to the bottom of
the exact source of the issue because there is so much deferred
execution going on, but I am confident that adding resources/forcing
re-layout events from an event handler is to blame.

My current solution is to remove as much of this asynchronous code as
possible. The use of a `QNetworkReply` seems to be leftover from when
Mumble supported remote images in chat. As described above, this
implementation was problematic and its complexity is no longer needed,
so I changed the implementation of `LogDocument::loadResource` to simply
check that the image URL is valid and has the `data://` scheme and then
just fall back to QTextDocument::loadResource`.

Implementing the solution this way eliminates the need for
`LogDocumentResourceAddedEvent` because everything happens in order as
expected and there is no need to "correct" the scroll value after the
image gets loaded. I therefore removed this class and the associated
code.

The default behaviour of `QTextEdit` is that it scrolls to the bottom
whenever contents are added. `LogTextBrowser::scrollLogToBottom` seems
to be a workaround to try to get the log to stay at the bottom despite
the scroll bar jumping issue. With my solution, this method can be
removed, and we only need to worry about "restoring" the previous scroll
value when a message from another user arrives and the log isn't already
scrolled to the bottom.
dexgs added a commit to dexgs/mumble that referenced this issue Feb 17, 2024
n my previous PR (mumble-voip#6290) addressing this bug, I wrote:
> The issue seems to be caused by the scrollbar sometimes being in an
> inconsistent state immediately after a message is sent, but not any
> longer.
>
> My workaround is to add a call to QApplication::processEvents before
> setting the scrollbar to its maximum position to avoid the inconsistent
> state.

I have found that this change only reduces the likelihood of the
incorrect scroll behaviour from happening: it is still reproducible
after the log grows past a certain size.

In my testing since then, the cause of the "inconsistent state" described
previously seems to be the following: When images (whose contents are
stored in a base64-encoded data URL) are inserted into the log, they are
not loaded immediately. Instead, they are "fetched" (although no actual
network request occurs) by the `loadResource` implementation on
`LogDocument`. The "fetching" is triggered by the call to `setHtml` in
the `Log::validHtml` method. However, the code which sets the scroll bar
to stay at the bottom runs immediately (i.e. not asynchronously) in the
`Log::log` method. There also seems to be a hack that forces a re-layout
by sending a "font changed" event.

The intended behaviour appears to be that when a log entry is added the
log will scroll to the bottom if the entry is a message sent by the user
themselves, OR if the log was already scrolled to the bottom before
the entry was added.

The previous solution in place addressing the asynchronous image loading
was dispatching a `LogDocumentResourceAddedEvent` when the image is
loded whose handler scrolls the log to the bottom, but this causes 2
problems:

First, the handler for this event always scrolls the log to the bottom
no matter what, which means messages containing images are handled
differently from the intended scrolling behaviour, i.e. messages
containing images ALWAYS cause the log to scroll to the bottom instead
of obeying the rules described above.

Second, it seemingly causes the issue described in mumble-voip#2504. It looks like
there's a race condition when the scroll position is set from the main
thread as well as in an event handler. I couldn't get to the bottom of
the exact source of the issue because there is so much deferred
execution going on, but I am confident that adding resources/forcing
re-layout events from an event handler is to blame.

My current solution is to remove as much of this asynchronous code as
possible. The use of a `QNetworkReply` seems to be leftover from when
Mumble supported remote images in chat. As described above, this
implementation was problematic and its complexity is no longer needed,
so I changed the implementation of `LogDocument::loadResource` to simply
check that the image URL is valid and has the `data://` scheme and then
just fall back to QTextDocument::loadResource`.

Implementing the solution this way eliminates the need for
`LogDocumentResourceAddedEvent` because everything happens in order as
expected and there is no need to "correct" the scroll value after the
image gets loaded. I therefore removed this class and the associated
code.

The default behaviour of `QTextEdit` is that it scrolls to the bottom
whenever contents are added. `LogTextBrowser::scrollLogToBottom` seems
to be a workaround to try to get the log to stay at the bottom despite
the scroll bar jumping issue. With my solution, this method can be
removed, and we only need to worry about "restoring" the previous scroll
value when a message from another user arrives and the log isn't already
scrolled to the bottom.
dexgs added a commit to dexgs/mumble that referenced this issue Feb 17, 2024
In my previous PR (mumble-voip#6290) addressing this bug, I wrote:
> The issue seems to be caused by the scrollbar sometimes being in an
> inconsistent state immediately after a message is sent, but not any
> longer.
>
> My workaround is to add a call to QApplication::processEvents before
> setting the scrollbar to its maximum position to avoid the inconsistent
> state.

I have found that this change only reduces the likelihood of the
incorrect scroll behaviour from happening: it is still reproducible
after the log grows past a certain size.

In my testing since then, the cause of the "inconsistent state" described
previously seems to be the following: When images (whose contents are
stored in a base64-encoded data URL) are inserted into the log, they are
not loaded immediately. Instead, they are "fetched" (although no actual
network request occurs) by the `loadResource` implementation on
`LogDocument`. The "fetching" is triggered by the call to `setHtml` in
the `Log::validHtml` method. However, the code which sets the scroll bar
to stay at the bottom runs immediately (i.e. not asynchronously) in the
`Log::log` method. There also seems to be a hack that forces a re-layout
by sending a "font changed" event.

The intended behaviour appears to be that when a log entry is added the
log will scroll to the bottom if the entry is a message sent by the user
themselves, OR if the log was already scrolled to the bottom before
the entry was added.

The previous solution in place addressing the asynchronous image loading
was dispatching a `LogDocumentResourceAddedEvent` when the image is
loded whose handler scrolls the log to the bottom, but this causes 2
problems:

First, the handler for this event always scrolls the log to the bottom
no matter what, which means messages containing images are handled
differently from the intended scrolling behaviour, i.e. messages
containing images ALWAYS cause the log to scroll to the bottom instead
of obeying the rules described above.

Second, it seemingly causes the issue described in mumble-voip#2504. It looks like
there's a race condition when the scroll position is set from the main
thread as well as in an event handler. I couldn't get to the bottom of
the exact source of the issue because there is so much deferred
execution going on, but I am confident that adding resources/forcing
re-layout events from an event handler is to blame.

My current solution is to remove as much of this asynchronous code as
possible. The use of a `QNetworkReply` seems to be leftover from when
Mumble supported remote images in chat. As described above, this
implementation was problematic and its complexity is no longer needed,
so I changed the implementation of `LogDocument::loadResource` to simply
check that the image URL is valid and has the `data://` scheme and then
just fall back to QTextDocument::loadResource`.

Implementing the solution this way eliminates the need for
`LogDocumentResourceAddedEvent` because everything happens in order as
expected and there is no need to "correct" the scroll value after the
image gets loaded. I therefore removed this class and the associated
code.

The default behaviour of `QTextEdit` is that it scrolls to the bottom
whenever contents are added. `LogTextBrowser::scrollLogToBottom` seems
to be a workaround to try to get the log to stay at the bottom despite
the scroll bar jumping issue. With my solution, this method can be
removed, and we only need to worry about "restoring" the previous scroll
value when a message from another user arrives and the log isn't already
scrolled to the bottom.
dexgs added a commit to dexgs/mumble that referenced this issue Feb 17, 2024
In my previous PR (mumble-voip#6290) addressing this bug, I wrote:
> The issue seems to be caused by the scrollbar sometimes being in an
> inconsistent state immediately after a message is sent, but not any
> longer.
>
> My workaround is to add a call to QApplication::processEvents before
> setting the scrollbar to its maximum position to avoid the inconsistent
> state.

I have found that this change only reduces the likelihood of the
incorrect scroll behaviour from happening: it is still reproducible
after the log grows past a certain size.

In my testing since then, the cause of the "inconsistent state" described
previously seems to be the following: When images (whose contents are
stored in a base64-encoded data URL) are inserted into the log, they are
not loaded immediately. Instead, they are "fetched" (although no actual
network request occurs) by the `loadResource` implementation on
`LogDocument`. The "fetching" is triggered by the call to `setHtml` in
the `Log::validHtml` method. However, the code which sets the scroll bar
to stay at the bottom runs immediately (i.e. not asynchronously) in the
`Log::log` method. There also seems to be a hack that forces a re-layout
by sending a "font changed" event.

The intended behaviour appears to be that when a log entry is added the
log will scroll to the bottom if the entry is a message sent by the user
themselves, OR if the log was already scrolled to the bottom before
the entry was added.

The previous solution in place addressing the asynchronous image loading
was dispatching a `LogDocumentResourceAddedEvent` when the image is
loaded whose handler scrolls the log to the bottom, but this causes 2
problems:

First, the handler for this event always scrolls the log to the bottom
no matter what, which means messages containing images are handled
differently from the intended scrolling behaviour, i.e. messages
containing images ALWAYS cause the log to scroll to the bottom instead
of obeying the rules described above.

Second, it seemingly causes the issue described in mumble-voip#2504. It looks like
there's a race condition when the scroll position is set from the main
thread as well as in an event handler. I couldn't get to the bottom of
the exact source of the issue because there is so much deferred
execution going on, but I am confident that adding resources/forcing
re-layout events from an event handler is to blame.

My current solution is to remove as much of this asynchronous code as
possible. The use of a `QNetworkReply` seems to be leftover from when
Mumble supported remote images in chat. As described above, this
implementation was problematic and its complexity is no longer needed,
so I changed the implementation of `LogDocument::loadResource` to simply
check that the image URL is valid and has the `data://` scheme and then
just fall back to QTextDocument::loadResource`.

Implementing the solution this way eliminates the need for
`LogDocumentResourceAddedEvent` because everything happens in order as
expected and there is no need to "correct" the scroll value after the
image gets loaded. I therefore removed this class and the associated
code.

The default behaviour of `QTextEdit` is that it scrolls to the bottom
whenever contents are added. `LogTextBrowser::scrollLogToBottom` seems
to be a workaround to try to get the log to stay at the bottom despite
the scroll bar jumping issue. With my solution, this method can be
removed, and we only need to worry about "restoring" the previous scroll
value when a message from another user arrives and the log isn't already
scrolled to the bottom.
dexgs added a commit to dexgs/mumble that referenced this issue Feb 17, 2024
In my previous PR (mumble-voip#6290) addressing this bug, I wrote:
> The issue seems to be caused by the scrollbar sometimes being in an
> inconsistent state immediately after a message is sent, but not any
> longer.
>
> My workaround is to add a call to QApplication::processEvents before
> setting the scrollbar to its maximum position to avoid the inconsistent
> state.

I have found that this change only reduces the likelihood of the
incorrect scroll behaviour from happening: it is still reproducible
after the log grows past a certain size.

In my testing since then, the cause of the "inconsistent state" described
previously seems to be the following: When images (whose contents are
stored in a base64-encoded data URL) are inserted into the log, they are
not loaded immediately. Instead, they are "fetched" (although no actual
network request occurs) by the `loadResource` implementation on
`LogDocument`. The "fetching" is triggered by the call to `setHtml` in
the `Log::validHtml` method. However, the code which sets the scroll bar
to stay at the bottom runs immediately (i.e. not asynchronously) in the
`Log::log` method. There also seems to be a hack that forces a re-layout
by sending a "font changed" event.

The intended behaviour appears to be that when a log entry is added the
log will scroll to the bottom if the entry is a message sent by the user
themselves, OR if the log was already scrolled to the bottom before
the entry was added.

The previous solution in place addressing the asynchronous image loading
was dispatching a `LogDocumentResourceAddedEvent` when the image is
loaded whose handler scrolls the log to the bottom, but this causes 2
problems:

First, the handler for this event always scrolls the log to the bottom
no matter what, which means messages containing images are handled
differently from the intended scrolling behaviour, i.e. messages
containing images ALWAYS cause the log to scroll to the bottom instead
of obeying the rules described above.

Second, it seemingly causes the issue described in mumble-voip#2504. It looks like
there's a race condition when the scroll position is set from the main
thread as well as in an event handler. I couldn't get to the bottom of
the exact source of the issue because there is so much deferred
execution going on, but I am confident that adding resources/forcing
re-layout events from an event handler is to blame.

My current solution is to remove as much of this asynchronous code as
possible. The use of a `QNetworkReply` seems to be leftover from when
Mumble supported remote images in chat. As described above, this
implementation was problematic and its complexity is no longer needed,
so I changed the implementation of `LogDocument::loadResource` to simply
check that the image URL is valid and has the `data://` scheme and then
just fall back to QTextDocument::loadResource`.

Implementing the solution this way eliminates the need for
`LogDocumentResourceAddedEvent` because everything happens in order as
expected and there is no need to "correct" the scroll value after the
image gets loaded. I therefore removed this class and the associated
code.

The default behaviour of `QTextEdit` is that it scrolls to the bottom
whenever contents are added. `LogTextBrowser::scrollLogToBottom` seems
to be a workaround to try to get the log to stay at the bottom despite
the scroll bar jumping issue. With my solution, this method can be
removed, and we only need to worry about "restoring" the previous scroll
value when a message from another user arrives and the log isn't already
scrolled to the bottom.
dexgs added a commit to dexgs/mumble that referenced this issue Feb 17, 2024
In my previous PR (mumble-voip#6290) addressing this bug, I wrote:
> The issue seems to be caused by the scrollbar sometimes being in an
> inconsistent state immediately after a message is sent, but not any
> longer.
>
> My workaround is to add a call to QApplication::processEvents before
> setting the scrollbar to its maximum position to avoid the inconsistent
> state.

I have found that this change only reduces the likelihood of the
incorrect scroll behaviour from happening: it is still reproducible
after the log grows past a certain size.

In my testing since then, the cause of the "inconsistent state" described
previously seems to be the following: When images (whose contents are
stored in a base64-encoded data URL) are inserted into the log, they are
not loaded immediately. Instead, they are "fetched" (although no actual
network request occurs) by the `loadResource` implementation on
`LogDocument`. The "fetching" is triggered by the call to `setHtml` in
the `Log::validHtml` method. However, the code which sets the scroll bar
to stay at the bottom runs immediately (i.e. not asynchronously) in the
`Log::log` method. There also seems to be a hack that forces a re-layout
by sending a "font changed" event.

The intended behaviour appears to be that when a log entry is added the
log will scroll to the bottom if the entry is a message sent by the user
themselves, OR if the log was already scrolled to the bottom before
the entry was added.

The previous solution in place addressing the asynchronous image loading
was dispatching a `LogDocumentResourceAddedEvent` when the image is
loaded whose handler scrolls the log to the bottom, but this causes 2
problems:

First, the handler for this event always scrolls the log to the bottom
no matter what, which means messages containing images are handled
differently from the intended scrolling behaviour, i.e. messages
containing images ALWAYS cause the log to scroll to the bottom instead
of obeying the rules described above.

Second, it seemingly causes the issue described in mumble-voip#2504. It looks like
there's a race condition when the scroll position is set from the main
thread as well as in an event handler. I couldn't get to the bottom of
the exact source of the issue because there is so much deferred
execution going on, but I am confident that adding resources/forcing
re-layout events from an event handler is to blame.

My current solution is to remove as much of this asynchronous code as
possible. The use of a `QNetworkReply` seems to be leftover from when
Mumble supported remote images in chat. As described above, this
implementation was problematic and its complexity is no longer needed,
so I changed the implementation of `LogDocument::loadResource` to simply
check that the image URL is valid and has the `data://` scheme and then
just fall back to QTextDocument::loadResource`.

Implementing the solution this way eliminates the need for
`LogDocumentResourceAddedEvent` because everything happens in order as
expected and there is no need to "correct" the scroll value after the
image gets loaded. I therefore removed this class and the associated
code.

The default behaviour of `QTextEdit` is that it scrolls to the bottom
whenever contents are added. `LogTextBrowser::scrollLogToBottom` seems
to be a workaround to try to get the log to stay at the bottom despite
the scroll bar jumping issue. With my solution, this method can be
removed, and we only need to worry about "restoring" the previous scroll
value when a message from another user arrives and the log isn't already
scrolled to the bottom.
dexgs added a commit to dexgs/mumble that referenced this issue Feb 17, 2024
In my previous PR (mumble-voip#6290) addressing this bug, I wrote:
> The issue seems to be caused by the scrollbar sometimes being in an
> inconsistent state immediately after a message is sent, but not any
> longer.
>
> My workaround is to add a call to QApplication::processEvents before
> setting the scrollbar to its maximum position to avoid the inconsistent
> state.

I have found that this change only reduces the likelihood of the
incorrect scroll behaviour from happening: it is still reproducible
after the log grows past a certain size.

In my testing since then, the cause of the "inconsistent state" described
previously seems to be the following: When images (whose contents are
stored in a base64-encoded data URL) are inserted into the log, they are
not loaded immediately. Instead, they are "fetched" (although no actual
network request occurs) by the `loadResource` implementation on
`LogDocument`. The "fetching" is triggered by the call to `setHtml` in
the `Log::validHtml` method. However, the code which sets the scroll bar
to stay at the bottom runs immediately (i.e. not asynchronously) in the
`Log::log` method. There also seems to be a hack that forces a re-layout
by sending a "font changed" event.

The intended behaviour appears to be that when a log entry is added the
log will scroll to the bottom if the entry is a message sent by the user
themselves, OR if the log was already scrolled to the bottom before
the entry was added.

The previous solution in place addressing the asynchronous image loading
was dispatching a `LogDocumentResourceAddedEvent` when the image is
loaded whose handler scrolls the log to the bottom, but this causes 2
problems:

First, the handler for this event always scrolls the log to the bottom
no matter what, which means messages containing images are handled
differently from the intended scrolling behaviour, i.e. messages
containing images ALWAYS cause the log to scroll to the bottom instead
of obeying the rules described above.

Second, it seemingly causes the issue described in mumble-voip#2504. It looks like
there's a race condition when the scroll position is set from the main
thread as well as in an event handler. I couldn't get to the bottom of
the exact source of the issue because there is so much deferred
execution going on, but I am confident that adding resources/forcing
re-layout events from an event handler is to blame.

My current solution is to remove as much of this asynchronous code as
possible. The use of a `QNetworkReply` seems to be leftover from when
Mumble supported remote images in chat. As described above, this
implementation was problematic and its complexity is no longer needed,
so I changed the implementation of `LogDocument::loadResource` to simply
check that the image URL is valid and has the `data://` scheme and then
just fall back to QTextDocument::loadResource`.

Implementing the solution this way eliminates the need for
`LogDocumentResourceAddedEvent` because everything happens in order as
expected and there is no need to "correct" the scroll value after the
image gets loaded. I therefore removed this class and the associated
code.

The default behaviour of `QTextEdit` is that it scrolls to the bottom
whenever contents are added. `LogTextBrowser::scrollLogToBottom` seems
to be a workaround to try to get the log to stay at the bottom despite
the scroll bar jumping issue. With my solution, this method can be
removed, and we only need to worry about "restoring" the previous scroll
value when a message from another user arrives and the log isn't already
scrolled to the bottom.
dexgs added a commit to dexgs/mumble that referenced this issue Feb 18, 2024
In my previous PR (mumble-voip#6290) addressing this bug, I wrote:
> The issue seems to be caused by the scrollbar sometimes being in an
> inconsistent state immediately after a message is sent, but not any
> longer.
>
> My workaround is to add a call to QApplication::processEvents before
> setting the scrollbar to its maximum position to avoid the inconsistent
> state.

I have found that this change only reduces the likelihood of the
incorrect scroll behaviour from happening: it is still reproducible
after the log grows past a certain size.

In my testing since then, the cause of the "inconsistent state" described
previously seems to be the following: When images (whose contents are
stored in a base64-encoded data URL) are inserted into the log, they are
not loaded immediately. Instead, they are "fetched" (although no actual
network request occurs) by the `loadResource` implementation on
`LogDocument`. The "fetching" is triggered by the call to `setHtml` in
the `Log::validHtml` method. However, the code which sets the scroll bar
to stay at the bottom runs immediately (i.e. not asynchronously) in the
`Log::log` method. There also seems to be a hack that forces a re-layout
by sending a "font changed" event.

The intended behaviour appears to be that when a log entry is added the
log will scroll to the bottom if the entry is a message sent by the user
themselves, OR if the log was already scrolled to the bottom before
the entry was added.

The previous solution in place addressing the asynchronous image loading
was dispatching a `LogDocumentResourceAddedEvent` when the image is
loaded whose handler scrolls the log to the bottom, but this causes 2
problems:

First, the handler for this event always scrolls the log to the bottom
no matter what, which means messages containing images are handled
differently from the intended scrolling behaviour, i.e. messages
containing images ALWAYS cause the log to scroll to the bottom instead
of obeying the rules described above.

Second, it seemingly causes the issue described in mumble-voip#2504. It looks like
there's a race condition when the scroll position is set from the main
thread as well as in an event handler. I couldn't get to the bottom of
the exact source of the issue because there is so much deferred
execution going on, but I am confident that adding resources/forcing
re-layout events from an event handler is to blame.

My current solution is to remove as much of this asynchronous code as
possible. The use of a `QNetworkReply` seems to be leftover from when
Mumble supported remote images in chat. As described above, this
implementation was problematic and its complexity is no longer needed,
so I changed the implementation of `LogDocument::loadResource` to simply
check that the image URL is valid and has the `data://` scheme and then
just fall back to QTextDocument::loadResource`.

Implementing the solution this way eliminates the need for
`LogDocumentResourceAddedEvent` because everything happens in order as
expected and there is no need to "correct" the scroll value after the
image gets loaded. I therefore removed this class and the associated
code.

The default behaviour of `QTextEdit` is that it scrolls to the bottom
whenever contents are added. `LogTextBrowser::scrollLogToBottom` seems
to be a workaround to try to get the log to stay at the bottom despite
the scroll bar jumping issue. With my solution, this method can be
removed, and we only need to worry about "restoring" the previous scroll
value when a message from another user arrives and the log isn't already
scrolled to the bottom.
dexgs added a commit to dexgs/mumble that referenced this issue Feb 18, 2024
In my previous PR (mumble-voip#6290) addressing this bug, I wrote:
> The issue seems to be caused by the scrollbar sometimes being in an
> inconsistent state immediately after a message is sent, but not any
> longer.
>
> My workaround is to add a call to QApplication::processEvents before
> setting the scrollbar to its maximum position to avoid the inconsistent
> state.

I have found that this change only reduces the likelihood of the
incorrect scroll behaviour from happening: it is still reproducible
after the log grows past a certain size.

In my testing since then, the cause of the "inconsistent state" described
previously seems to be the following: When images (whose contents are
stored in a base64-encoded data URL) are inserted into the log, they are
not loaded immediately. Instead, they are "fetched" (although no actual
network request occurs) by the `loadResource` implementation on
`LogDocument`. The "fetching" is triggered by the call to `setHtml` in
the `Log::validHtml` method. However, the code which sets the scroll bar
to stay at the bottom runs immediately (i.e. not asynchronously) in the
`Log::log` method. There also seems to be a hack that forces a re-layout
by sending a "font changed" event.

The intended behaviour appears to be that when a log entry is added the
log will scroll to the bottom if the entry is a message sent by the user
themselves, OR if the log was already scrolled to the bottom before
the entry was added.

The previous solution in place addressing the asynchronous image loading
was dispatching a `LogDocumentResourceAddedEvent` when the image is
loaded whose handler scrolls the log to the bottom, but this causes 2
problems:

First, the handler for this event always scrolls the log to the bottom
no matter what, which means messages containing images are handled
differently from the intended scrolling behaviour, i.e. messages
containing images ALWAYS cause the log to scroll to the bottom instead
of obeying the rules described above.

Second, it seemingly causes the issue described in mumble-voip#2504. It looks like
there's a race condition when the scroll position is set from the main
thread as well as in an event handler. I couldn't get to the bottom of
the exact source of the issue because there is so much deferred
execution going on, but I am confident that adding resources/forcing
re-layout events from an event handler is to blame.

My current solution is to remove as much of this asynchronous code as
possible. The use of a `QNetworkReply` seems to be leftover from when
Mumble supported remote images in chat. As described above, this
implementation was problematic and its complexity is no longer needed,
so I changed the implementation of `LogDocument::loadResource` to simply
check that the image URL is valid and has the `data://` scheme and then
just fall back to QTextDocument::loadResource`.

Implementing the solution this way eliminates the need for
`LogDocumentResourceAddedEvent` because everything happens in order as
expected and there is no need to "correct" the scroll value after the
image gets loaded. I therefore removed this class and the associated
code.

The default behaviour of `QTextEdit` is that it scrolls to the bottom
whenever contents are added. `LogTextBrowser::scrollLogToBottom` seems
to be a workaround to try to get the log to stay at the bottom despite
the scroll bar jumping issue. With my solution, this method can be
removed, and we only need to worry about "restoring" the previous scroll
value when a message from another user arrives and the log isn't already
scrolled to the bottom.
mryamac pushed a commit to mryamac/mumble that referenced this issue Apr 6, 2024
In my previous PR (mumble-voip#6290) addressing this bug, I wrote:
> The issue seems to be caused by the scrollbar sometimes being in an
> inconsistent state immediately after a message is sent, but not any
> longer.
>
> My workaround is to add a call to QApplication::processEvents before
> setting the scrollbar to its maximum position to avoid the inconsistent
> state.

I have found that this change only reduces the likelihood of the
incorrect scroll behaviour from happening: it is still reproducible
after the log grows past a certain size.

In my testing since then, the cause of the "inconsistent state" described
previously seems to be the following: When images (whose contents are
stored in a base64-encoded data URL) are inserted into the log, they are
not loaded immediately. Instead, they are "fetched" (although no actual
network request occurs) by the `loadResource` implementation on
`LogDocument`. The "fetching" is triggered by the call to `setHtml` in
the `Log::validHtml` method. However, the code which sets the scroll bar
to stay at the bottom runs immediately (i.e. not asynchronously) in the
`Log::log` method. There also seems to be a hack that forces a re-layout
by sending a "font changed" event.

The intended behaviour appears to be that when a log entry is added the
log will scroll to the bottom if the entry is a message sent by the user
themselves, OR if the log was already scrolled to the bottom before
the entry was added.

The previous solution in place addressing the asynchronous image loading
was dispatching a `LogDocumentResourceAddedEvent` when the image is
loaded whose handler scrolls the log to the bottom, but this causes 2
problems:

First, the handler for this event always scrolls the log to the bottom
no matter what, which means messages containing images are handled
differently from the intended scrolling behaviour, i.e. messages
containing images ALWAYS cause the log to scroll to the bottom instead
of obeying the rules described above.

Second, it seemingly causes the issue described in mumble-voip#2504. It looks like
there's a race condition when the scroll position is set from the main
thread as well as in an event handler. I couldn't get to the bottom of
the exact source of the issue because there is so much deferred
execution going on, but I am confident that adding resources/forcing
re-layout events from an event handler is to blame.

My current solution is to remove as much of this asynchronous code as
possible. The use of a `QNetworkReply` seems to be leftover from when
Mumble supported remote images in chat. As described above, this
implementation was problematic and its complexity is no longer needed,
so I changed the implementation of `LogDocument::loadResource` to simply
check that the image URL is valid and has the `data://` scheme and then
just fall back to QTextDocument::loadResource`.

Implementing the solution this way eliminates the need for
`LogDocumentResourceAddedEvent` because everything happens in order as
expected and there is no need to "correct" the scroll value after the
image gets loaded. I therefore removed this class and the associated
code.

The default behaviour of `QTextEdit` is that it scrolls to the bottom
whenever contents are added. `LogTextBrowser::scrollLogToBottom` seems
to be a workaround to try to get the log to stay at the bottom despite
the scroll bar jumping issue. With my solution, this method can be
removed, and we only need to worry about "restoring" the previous scroll
value when a message from another user arrives and the log isn't already
scrolled to the bottom.
mryamac pushed a commit to mryamac/mumble that referenced this issue May 21, 2024
In my previous PR (mumble-voip#6290) addressing this bug, I wrote:
> The issue seems to be caused by the scrollbar sometimes being in an
> inconsistent state immediately after a message is sent, but not any
> longer.
>
> My workaround is to add a call to QApplication::processEvents before
> setting the scrollbar to its maximum position to avoid the inconsistent
> state.

I have found that this change only reduces the likelihood of the
incorrect scroll behaviour from happening: it is still reproducible
after the log grows past a certain size.

In my testing since then, the cause of the "inconsistent state" described
previously seems to be the following: When images (whose contents are
stored in a base64-encoded data URL) are inserted into the log, they are
not loaded immediately. Instead, they are "fetched" (although no actual
network request occurs) by the `loadResource` implementation on
`LogDocument`. The "fetching" is triggered by the call to `setHtml` in
the `Log::validHtml` method. However, the code which sets the scroll bar
to stay at the bottom runs immediately (i.e. not asynchronously) in the
`Log::log` method. There also seems to be a hack that forces a re-layout
by sending a "font changed" event.

The intended behaviour appears to be that when a log entry is added the
log will scroll to the bottom if the entry is a message sent by the user
themselves, OR if the log was already scrolled to the bottom before
the entry was added.

The previous solution in place addressing the asynchronous image loading
was dispatching a `LogDocumentResourceAddedEvent` when the image is
loaded whose handler scrolls the log to the bottom, but this causes 2
problems:

First, the handler for this event always scrolls the log to the bottom
no matter what, which means messages containing images are handled
differently from the intended scrolling behaviour, i.e. messages
containing images ALWAYS cause the log to scroll to the bottom instead
of obeying the rules described above.

Second, it seemingly causes the issue described in mumble-voip#2504. It looks like
there's a race condition when the scroll position is set from the main
thread as well as in an event handler. I couldn't get to the bottom of
the exact source of the issue because there is so much deferred
execution going on, but I am confident that adding resources/forcing
re-layout events from an event handler is to blame.

My current solution is to remove as much of this asynchronous code as
possible. The use of a `QNetworkReply` seems to be leftover from when
Mumble supported remote images in chat. As described above, this
implementation was problematic and its complexity is no longer needed,
so I changed the implementation of `LogDocument::loadResource` to simply
check that the image URL is valid and has the `data://` scheme and then
just fall back to QTextDocument::loadResource`.

Implementing the solution this way eliminates the need for
`LogDocumentResourceAddedEvent` because everything happens in order as
expected and there is no need to "correct" the scroll value after the
image gets loaded. I therefore removed this class and the associated
code.

The default behaviour of `QTextEdit` is that it scrolls to the bottom
whenever contents are added. `LogTextBrowser::scrollLogToBottom` seems
to be a workaround to try to get the log to stay at the bottom despite
the scroll bar jumping issue. With my solution, this method can be
removed, and we only need to worry about "restoring" the previous scroll
value when a message from another user arrives and the log isn't already
scrolled to the bottom.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants