FIX(client, ui): fix text selection in chat log

[dexgs]

In v1.4, the logic surrounding when entries in the chat log are put in their own text frame was changed, introducing a bug where elements such as lists would not be terminated correctly (#4491).

The fix for this issue was to put every chat log entry into its own text frame, but Qt inserts a blank line between adjacent frames resulting in too much space between subsequent chat entries.

There is no convenient Qt API (as far as I know) to disable this behaviour, so my second attempt to resolve this (PR #5820) set the font size to near zero for the blank lines between entries.

This worked on Linux, but broke on (at least) Windows (#5886).

PR #5927 fixed the issue by calling setVisible(false) on the blank lines instead of changing the font size, but this introduced a regression to text selection in the chat log (#6045).

This PR retains the setVisible(false) strategy by @bkaestner to remove the initial "gap" at the top of the chat log, but deals with the empty lines between entries by setting their line height to zero using a call to setLineHeight(0, QTextBlockFormat::FixedHeight)

https://doc.qt.io/qt-5/qtextblockformat.html#LineHeightTypes-enum

According to the Qt documentation, this method allows setting a fixed line height in pixels. Based on my testing on Linux, this solves the problem without introducing any new regressions.

This should still be tested on Windows before v1.5, just in case.

Fixes #6045

Checks

• My commits follow the commit guidelines

[bkaestner]

Thanks for picking this up again! Given that I don't have a Mumble development environment anymore, I picked up the artifacts from Azure from this job and tried it out.

From a first glance, it seems to work exactly as intended, so thank you @dexgs!

Tests

Test Environment:

Edition	      Windows 10 Pro
Version	      22H2
Install Date  09.‎01.‎2022
Build         19045.3803

Check after PR

Looks fine. I've posted an image to verify that images still work as expected, and it seems all is working as intended.

Pre-Check on current stable

For comparison, this is how it looks on the current stable:

Don't worry about the screenshot, it's actually a different one.

Concerns

There is one minor concern I have with the setLineHeight(0, QTextBlockFormat::FixedHeight) solution, namely Qt themes. I unfortunately don't have any means to style Qt applications at the moment, but I can imagine that certain accessibility features (like screen readers or high-contrast options) might still recognize the line. Especially high-contrast Qt themes could add some spacing there again, but I'm not sure about that. I don't know how much custom Qt styling is used in the wild, though.

The Windows high-contrast setting does not yield any change though (except for a hight contrast title bar).

[bkaestner]

I've noticed that I accidentally misremembered #6045 and didn't check the selection behaviour on Windows, I've only verified that it still looks like as in #5927. 🤦‍♂️

Unfortunately, I've already downgraded back to 1.4.287 on this machine but can retest later today (UTC) if necessary.

[dexgs]

I hadn't already thought about themes, but I think the call to setLineHeight will override any theme properties. However, there's still the top and bottom margins on the line. Maybe it's worth setting those to zero as well to address this.

I don't know how screen readers interact with the blank line, but I suspect they won't cause problems since they don't have any text in them.

[dexgs]

I updated the commit to set the top and bottom margins to zero to hopefully address any problems with themes.

I also switched to setting the QTextBlockFormat before and after each message instead of using setVisible to deal with the initial gap at the top of the text log because the latter strategy still leaves a gap at the top of the chat log after it is cleared, i.e. it only deals with the gap when the log is first initialized.

[Krzmbrzl]

Have you tested whether your changes also play nicely with the [Date changed to xy] message that inserts a new block instead of a new frame?

[dexgs]

@Krzmbrzl you're right and I hadn't thought about this. I'm checking on this now.

[dexgs]

@Krzmbrzl The simplest fix seems to be setting the "date changed" notification to use a text frame as well.

Here's how it looks (for testing, I modified the "date changed" notification to appear before every log entry)

Is this acceptable?

EDIT: I've updated the PR to include this change

[Krzmbrzl]

Is this acceptable?

Sure. So I take it that it is indeed essential that new text is always added as a new frame, correct? What happens to the date message when adding it as a block? Is it simply invisible due to the line height being zero?
I think the comment in the code should include a warning about this to ensure future coders are made aware of this assumption.

[dexgs]

without putting the date message into a frame, the text ends up overlapping the next log entry because the line height is zero

[Hartmnt]

I have compiled this PR and the result is decent. It fixes the selection bug for me. I can not comment on whether or not this works on Windows.

However one small thing I noticed is that if you select an entire line by triple-clicking, two line breaks (one at the start and one at the end) will also be selected. No idea if there is an easy fix for this, but I could live with it either way.

[dexgs]

@Krzmbrzl I edited the comment to add the warning you suggest

@Hartmnt I see this as well, not sure what can be done about it as setVisible(false) creates other problems.

[Krzmbrzl]

I tested these changes on Linux, Windows and macOS and it seems to work as expected on all of them. Nice work! 👍

[Krzmbrzl]

Thank you very much for looking at this again 👍