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

Autoscroll doesn't always work (correctly) on latest master #4638

Closed
Gert-dev opened this issue Dec 16, 2020 · 11 comments
Closed

Autoscroll doesn't always work (correctly) on latest master #4638

Gert-dev opened this issue Dec 16, 2020 · 11 comments
Labels
bug A bug (error) in the software client external-bug Bugs caused by things outside of our control - by dependencies, "upstream" in technical jargon ui

Comments

@Gert-dev
Copy link
Contributor

Describe the bug
Several of my fellow Mumblers updated to an unstable build a few days ago to get access to stereo support and apparently some bugfixes related to image pasting, and we noticed that sometimes autoscrolling doesn't behave well when images are pasted.

Usually, if A pastes an image, and whether B's scrollbar is scrolled down as much as possible or not, B's chat view will scroll further down (to the bottom), bringing the image into view. However, sometimes, B's chat view will scroll to a (seemingly) arbitrary point in the chat view. This "abitrary" point always seems to be the same point, once this starts happening.

Steps to Reproduce
Steps to reproduce the behavior:

  1. Join a server with two clients.
  2. Start pasting images (e.g. by copying them from a browser and pasting them in the chat box input using Ctrl-V) and observe the behavior.

Because this doesn't always happen, I understand this may not be easily reproducible. It seems to happen about half of the time. Sometimes it randomly fixes itself and several pastes will work fine, and it might break again at some other point.

I've (unsuccessfully) tried pinpointing the exact thing that makes this occur:

  • Images of various widths and heights, but nope.
  • Clearing the chat view when this starts happening, also nope.

Expected behavior
When someone says something or pastes an image:

  • Was it you? Then always jump to the bottom.
  • Was it someone else?
    • Is your scrollbar already at the bottom? Move further to the bottom after inserting the text or image.
    • Is your scrollbar not at the bottom? Maintain it in such a way that your viewport stays the same and you can keep reading what you were reading and are not forced down every time.

Desktop (please complete the following information):

  • OS: Arch Linux 64-bit for me, Windows 10 for my fellow Mumblers.
  • Version: master at commit c5b5c12 (1.4.0 dev). This also still appears to happen on commits of a couple of days ago.

Additional context
May be related to or a regression of #1258 or #1969.

@Krzmbrzl Krzmbrzl added bug A bug (error) in the software client ui labels Dec 18, 2020
@Krzmbrzl
Copy link
Member

However, sometimes, B's chat view will scroll to a (seemingly) arbitrary point in the chat view. This "abitrary" point always seems to be the same point, once this starts happening.

Is this arbitrary position really arbitrary or is it e.g. always lower or equal to the current scroll position?

Does this only happen if B is scrolled down to the bottom at the time the image arrives or does this also happen regardless of B's current scroll position?


When having a quick look at the code that is probably responsible for the scrolling behavior, it seems that it pretty much uses the exact same logic you just described:

mumble/src/mumble/Log.cpp

Lines 661 to 707 in d408deb

const int oldscrollvalue = tlog->getLogScroll();
const bool scroll = (oldscrollvalue == tlog->getLogScrollMaximum());
tc.movePosition(QTextCursor::End);
if (qdDate != dt.date()) {
qdDate = dt.date();
tc.insertBlock();
tc.insertHtml(
tr("[Date changed to %1]\n").arg(qdDate.toString(Qt::DefaultLocaleShortDate).toHtmlEscaped()));
tc.movePosition(QTextCursor::End);
}
// Convert CRLF to unix-style LF and old mac-style LF (single \r) to unix-style as well
QString fixedNLPlain =
plain.replace(QLatin1String("\r\n"), QLatin1String("\n")).replace(QLatin1String("\r"), QLatin1String("\n"));
if (fixedNLPlain.contains(QRegExp(QLatin1String("\\n[ \\t]*$")))) {
// If the message ends with one or more blank lines (or lines only containing whitespace)
// paint a border around the message to make clear that it contains invisible parts.
// The beginning of the message is clear anyway (the date and potentially the "To XY" part)
// so we don't have to care about that.
QTextFrameFormat qttf;
qttf.setBorder(1);
qttf.setPadding(2);
qttf.setMargin(msgMargin);
qttf.setBorderStyle(QTextFrameFormat::BorderStyle_Dashed);
tc.insertFrame(qttf);
} else if (!tc.block().text().isEmpty()) {
// Only insert a block if the current block is not empty. It may be empty because
// it is the default block of an empty document. Another cause might be that apparently
// a new empty block is automatically inserted after a frame.
tc.insertBlock();
}
const QString timeString = dt.time().toString(QLatin1String(g.s.bLog24HourClock ? "HH:mm:ss" : "hh:mm:ss AP"));
tc.insertHtml(Log::msgColor(QString::fromLatin1("[%1] ").arg(timeString.toHtmlEscaped()), Log::Time));
validHtml(console, &tc);
tc.movePosition(QTextCursor::End);
g.mw->qteLog->setTextCursor(tc);
if (scroll || ownMessage)
tlog->scrollLogToBottom();
else
tlog->setLogScroll(oldscrollvalue);
}

@Gert-dev
Copy link
Contributor Author

Gert-dev commented Dec 18, 2020

Is this arbitrary position really arbitrary or is it e.g. always lower or equal to the current scroll position?

The position is (so far) always upwards, i.e. backwards in chat history, looking at older logs. A pasting an image will make B jump up to some older chat logs he isn't interested in or wasn't even looking at, as if some random point in the chat history is locked/chosen sometimes, and then that point is scrolled to instead of scrolling to the end of the chat log (if B was already at the end).

Does this only happen if B is scrolled down to the bottom at the time the image arrives or does this also happen regardless of B's current scroll position?

It happens when B is scrolled down to the bottom at the time the image arrives. When B is not, I only noticed Mumble forcing B's view down to the end (which is annoying, but currently perhaps still intended behavior); it may yet also happen here, but I've not (yet) noticed it, but I'll keep an eye out to see if it also happens there.

@Gert-dev
Copy link
Contributor Author

... but I've not (yet) noticed it, but I'll keep an eye out to see if it also happens there.

Had some more time to test, and it also happens when scrolled up; you are then forced down back to that specific position, rather than the end.

@Krzmbrzl
Copy link
Member

Hm okay. That's weird 👀

I guess in order to find out what's going on, we'd have to do a git bisect to find out where the problem was introduced...

@Argetlami
Copy link

Can confirm, the automatic-scroll-to-bottom does not seem to work on any linux distribution I've used in past years. Our mumble community heavily utilizes posting images into the mumble chat with a mumo chatimg-module. With that, this is very visible bug in our use scenario. This does not seem to happen on Windows-clients.

However, this might be an upstream bug as this issue is probably relevant to #2504
An upstream bug report was created from that issue https://bugreports.qt.io/browse/QTBUG-83622

@Krzmbrzl
Copy link
Member

Krzmbrzl commented Mar 5, 2021

@Argetlami thanks for the info. This does indeed look very related 🤔

In fact after checking the code again this seems to be the exact same issue. Inside Log.cpp we are using

mumble/src/mumble/Log.cpp

Lines 695 to 696 in 95d5182

if (scroll || ownMessage)
tlog->scrollLogToBottom();

and this function is defined as
void LogTextBrowser::scrollLogToBottom() {
verticalScrollBar()->setValue(verticalScrollBar()->maximum());
}

which is the exact same semantics as reported in that bug report.

Thus I am closing this issue as an upstream bug and as a duplicate of #2504

@Krzmbrzl Krzmbrzl closed this as completed Mar 5, 2021
@Krzmbrzl Krzmbrzl added the external-bug Bugs caused by things outside of our control - by dependencies, "upstream" in technical jargon label Mar 5, 2021
@Kissaki
Copy link
Member

Kissaki commented Mar 5, 2021

We may want to track this as a known-issue then? (Release notes or wherever we collect those.)

@Krzmbrzl
Copy link
Member

Krzmbrzl commented Mar 6, 2021

Yes indeed 👍

@Kissaki
Copy link
Member

Kissaki commented Mar 6, 2021

Will you add it @Krzmbrzl ?

@Krzmbrzl
Copy link
Member

Krzmbrzl commented Mar 6, 2021

Already done

@Gert-dev
Copy link
Contributor Author

Just wanted to follow up and mention that this happens to a friend on Windows, too, in case anyone is wondering.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A bug (error) in the software client external-bug Bugs caused by things outside of our control - by dependencies, "upstream" in technical jargon ui
Projects
None yet
Development

No branches or pull requests

4 participants