FIX(ui): resolve text/icon scaling issues #5820
Conversation
dexgs
force-pushed
the
master
branch
2 times, most recently
from
a813744 to
369d963
Compare
|
These are two separate fixes. Therefore, I would ask you to split the changes into two separate commits as well. You don't have to create a separate PR though |
|
The minimum Icon size might be a good idea, but I wonder why this broke in the first place? Also, do the TalkingUI icons work as expected as they are created in the same way IIRC. Edit: I would recommend to first find the discrepancy in the logic rather than using a hard-coded minimum size. If e.g. the "make font size configurable" is implemented, the minimum size will also break. Is the icon scaling problem a Windows thing? |
I'm not so sure about that. The icon size should equal the font size. Always.
This could be another case of Qt stupidity. I fought very long and hard against such issues in the TalkingUI as well. As it turns out, the font of widgets is not initialized when the widget is initialized but some time later. Thus, when taking the font from a freshly constructed widget to measure the font height, then what is returned is some sort of default font size instead of the actual font size. My best guess would be that we are seeing a similar issue here. |
Interesting. This would be curious, as it did never happen while testing so I wonder if this is a platform/qt-version thing. I am currently trying to create a build environment, in which I can build the 1.4 branch (because 1.4 fails to compile with OpenSSL 3.0) |
|
@Krzmbrzl I think I found the logic issue regarding the icon size and will try to prepare a merge request |
Great, I'll remove my change to the icon sizing from this PR. |
|
Okay, done. This PR is now just the fix for the log. I've also changed the commit message to reflect this (although the pr message is unchanged) |
|
Could you give a quick summary as to what was the underlying problem? Aka: why do your changes affect the font size? (Note: This is not an attempt of blaming you for this! I'm simply hoping to get a better understanding of the situation) |
@Krzmbrzl Sure: The scaling problemThe issue with the font size has to do with the previous deletion of ae097d6#diff-368234adaec42dd85fef69ff070b7ba375dbedc09a42b0b17f5f41c95f4eb846L645-L647 It's some Qt magic, but i think removing this messed up the styles which get passed on to the inserted log entry. Re-introducing that snippet is what fixed the scaling during my testing. The selecting problemThe initial assessment of the log pollution from my first PR is correct: the problem is caused by inserting block elements such as lists into QTextBlock. The solution from that PR is also correct (putting all messages into QTextFrame instead of QTextBlock). However, QTextEdit will always insert a blank line before each QTextFrame which disrupts the spacing of the chat window. Initially, I resolved this by setting a negative top margin on each frame equal to the height of each frame. This works visually, but it causes mouse events to be blocked because there are now overlapping elements. The change in this PR is to instead set the frame's position to float which simply displaces the blank line without creating any overlap. |
|
Actually, even setting the position to float seems to cause some weird behaviour with text selection. I think a better solution is that since each message is in a QTextFrame instead of a QTextBlock, we can set the font size for QTextBlocks to 0 for the chat log without messing up the font size of log entries. This seems to resolve the blank lines between log entries when using QTextFrame without introducing any regression. I've updated this PR with the above changes. I'm confident this solution fixes the previous issues without introducing new problems, but please take a look to make sure I didn't miss anything. |
|
Also one small detail: Could you turn your commit message into a |
|
I've removed the static annotation and updated the commit message as requested. |
|
Could you add a line |
Fixed chat log scaling issue introduced by PR mumble-voip#5619 (my fault, whoops) The bug fixed by that commit is still fixed after this PR, but the regression introduced has been resolved. Fixes mumble-voip#5818
|
Thanks for fixing this 👍 |
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
|
Yeah, I'm aware of this. The problem doesn't affect linux, but I observed it on windows. I suspect the problem is with setting font sizes on windows. Qt docs say anything > 0 is valid, but I'm thinking that the windows implementation is more picky. |
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 (mumble-voip#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 mumble-voip#5820) set the font size to near zero for the blank lines between entries. This worked on Linux, but broke on (at least) Windows (mumble-voip#5886). PR mumble-voip#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 (mumble-voip#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 mumble-voip#6045
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 (mumble-voip#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 mumble-voip#5820) set the font size to near zero for the blank lines between entries. This worked on Linux, but broke on (at least) Windows (mumble-voip#5886). PR mumble-voip#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 (mumble-voip#6045). This PR 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 mumble-voip#6045
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 (mumble-voip#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 mumble-voip#5820) set the font size to near zero for the blank lines between entries. This worked on Linux, but broke on (at least) Windows (mumble-voip#5886). PR mumble-voip#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 (mumble-voip#6045). This PR 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 mumble-voip#6045
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 (mumble-voip#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 mumble-voip#5820) set the font size to near zero for the blank lines between entries. This worked on Linux, but broke on (at least) Windows (mumble-voip#5886). PR mumble-voip#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 (mumble-voip#6045). This PR 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 mumble-voip#6045
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 (mumble-voip#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 mumble-voip#5820) set the font size to near zero for the blank lines between entries. This worked on Linux, but broke on (at least) Windows (mumble-voip#5886). PR mumble-voip#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 (mumble-voip#6045). This PR 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 mumble-voip#6045
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 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
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 (mumble-voip#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 mumble-voip#5820) set the font size to near zero for the blank lines between entries. This worked on Linux, but broke on (at least) Windows (mumble-voip#5886). PR mumble-voip#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 (mumble-voip#6045). This PR 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 mumble-voip#6045
Fixed chat log scaling issue introduced by PR #5619 (my fault, whoops)
The bug fixed by that commit is still fixed after this PR, but the
regression introduced has been resolved.
Added minimum icon size for channel status icons which now scale since PR #5772
Initially, icons were too small at 100% scale.
Checks