Tracking upstream issue QTBUG-85498

[sssoleileraaa]

Description

This is an issue to track upstream issue: https://bugreports.qt.io/browse/QTBUG-85498

We use QLabels in various places, including our message and reply speech bubbles. QLabel provides a wrapping feature but does not automatically wrap strings that are longer than the width of the label and contains no spaces, dashes, and other unknown wrapping-point characters (wrapping points are not documented or specified in the Qt code so it's a bit of an unknown).

More info

QLabel knows to wrap based on the length of a line of text and where the nearest wrapping-point character is within the bounds of the width of the label. Unfortunately, there isn't Qt support to adjust the wrapping policy to say that we prefer wrapping over cutting off strings that don't contain wrapping points.

We've learned that QTextEdit and QPlainTextEdit support the kind of wrapping we are interested in by default, however as you can see in #1050 there is the problem with recalculating and setting the height of a QTextEdit to fit the contents of its internal document.

Test data can be found here: https://github.com/freedomofpress/securedrop-client/wiki/Test-plan#input-testing

[sssoleileraaa]

I think the desired behavior is to use word wrapping over a horizontal scrollbar

[eloquence]

Marked as release blocker due to information loss from the user's perspective.

[eloquence]

Pulled into current sprint per discussion on #799.

[ntoll]

I've been able to reproduce. I suspect a word-wrap strategy flag needs to be set to the appropriate value. Will try to get as close to @creviera's suggestion as possible in coming up PR. 👍

[ntoll]

Hmmm... https://doc.qt.io/qt-5/qlabel.html#wordWrap-prop :-(

It looks like Qt's default word-wrap only works on whitespace, not as @creviera specifies. However, I notice (see screenie) that if I setWordWrap(True) on the QLabel in our SpeechBubble widget I notice that things do start to wrap on a "-" character, giving rise to the possibility that if there is a single word > X characters (where X is our current max char width) then we insert a "-" at that point so Qt's word-wrapping kicks in. I'll code up this approach into a PR.

[ntoll]

FWIW, this also effects summary and messages too:

[ntoll]

Thank you St.Guido van Rossum: https://docs.python.org/3.8/library/textwrap.html

[eloquence]

Reopening per findings in #998.

[sssoleileraaa]

QLabel knows to wrap based on the length of a line of text and where the nearest wrapping-point character is within the bounds of the width of the label. Unfortunately, there isn't Qt support to adjust the wrapping policy to say that we prefer wrapping over cutting off strings that don't contain wrapping points.

We've learned that QTextEdit and QPlainTextEdit support the kind of wrapping we are interested in by default, however as you can see in #1050 there is the problem with recalculating and setting the height of a QTextEdit to fit the contents of its internal document.

As of now, we've looked into the following possible solutions:

• Use the library called textwrap to wrap message text every 70 characters. We saw problems with the way this was implemented the first time, e.g. blank lines in a message were removed, but if we set replace_whitespace to false, then this won't occur. However lines wrap in unexpected places once replace_whitespace is set to false (as you can see in the screenshot below where there shouldn't be newlines after "Gumbo" or "Turnip"), so this needs more investigation.

• Use QPlainTextEdit and its default wrapping feature that supports the kind of wrapping we want (a wrapping feature that never allows words to be cut off). See Adds wordwrap and text selectability for messages and replies #1050 to understand the complications with updating the height of QPlainTextEdit to fit its contents. This also needs more investigation.

• Create our own wrapping method that allows us to wrap before cutting off a string if it doesn't already contain a wrapping point character. So basically we want to copy the automatic height resizing that QLabel supports and the automatic wrapping that never cuts off text that QPlainTextEdit supports.

As follow-up, @kushaldas and I will paste links to the Qt C++ code where all the relevant behavior takes place when it comes to wrapping and resizing.

[eloquence]

Cross-ref additional Qt bug filed by @creviera: https://bugreports.qt.io/browse/QTBUG-85498

Nothing much to do here but wait a bit, so keeping this in the near-term backlog.

[eloquence]

Per quick chat with @creviera, based on upstream feedback https://bugreports.qt.io/browse/QTBUG-85498?focusedCommentId=520391&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-520391, it may make sense to do an additional brief implementation spike to implement custom wrapping logic for special cases Qt doesn't handle well, nominating for discussion tomorrow at sprint planning.

[eloquence]

^ Per the above, @creviera has committed to allocating a 2 hour timebox during the 7/23-8/5 sprint to see if this approach is viable.

[sssoleileraaa]

Just pointing out that this workaround is similar to #838, but instead of adding newlines every 70 characters we can just add newlines only on strings that do not contain a space (later we can make sure to avoid inserting a new line on strings that will wrap because they have other wrapping point characters such as a dash, a slash, etc. - a space is not the only wrapping point) and are more than a specified length. We want to avoid adding newlines if we don't have to because it alters the text that a user might copy and paste somewhere else. Another issue with this workaround is that we will have to insert newlines in the middle of a line in the speech bubble in order to account for speech bubble resizing behavior that we will introduce in order to better support smaller screen resolutions. This will not look as nice as wrapping once the line of text reaches the length of the speech bubble.

A workaround for the first issue mentioned above for copy and paste would be to create a copy function that grabs the original, unaltered text rather than the text from the speech bubble. A workaround for the second issue to support responsive speech bubbles would be to listen for a resize event and to re-calculate the cut-off point based on the new width - this is not great, so we'll probably just stick to adding newlines wherever our minimum speech bubble width is set.

The underlying fix for this issue is being tracked here: https://bugreports.qt.io/browse/QTBUG-85498.

[eloquence]

(later we can make sure to avoid inserting a new line on strings that will wrap because they have other wrapping point characters such as a dash, a slash, etc. - a space is not the only wrapping point)

I agree it would be a nice iteration to not wrap in cases where we can detect that Qt's built-in behavior will do a better job than our insertion of a newline at the n character mark.

we'll probably just stick to adding newlines wherever our minimum speech bubble width is set.

That makes sense to me. I don't think we need to ever support a lower window size than 1280x1024. It would be good to know what line length we can render at that resolution (given maximum character width), and whether or not that's a reasonable wrapping point.

I think the priority of changing the copy/paste behavior would depend on how likely a user is to encounter this behavior in the real world. If it's extremely unlikely except for highly unusual URLs or strings, I don't think we need to prioritize changing the copy/paste behavior to copy the raw (unwrapped) original.

[sssoleileraaa]

That makes sense to me. I don't think we need to ever support a lower window size than 1280x1024. It would be good to know what line length we can render at that resolution (given maximum character width), and whether or not that's a reasonable wrapping point.

We did end up adding support for a 1366 x 768 resolution, see #1074 (comment), but as of now you have to horizontally scroll in order to see the full width of the messages and replies. If we want to support showing all of the text in the speech bubbles, then we'll either have to change the speech bubble width to make it more responsive, or change the size of the sourcelist, or change the size/hide the left bar, or leave the horizontal scrolling as it is now. Hopefully by the time we get to this the Qt bug will be resolved. For now, we can just assume the width of the speech bubbles are static.

[sssoleileraaa]

here is a spike i did of speech bubbles that change in size based on size of window, for example:

[sssoleileraaa]

i'm going to stop the spike at this point so we can check in tomorrow:

• as we found in Support 1366x768 screen resolution #1074 not all characters are the same width so some long words will still get cut off. to get around this, we can use a very short max line length (in characters) that would make it less likely to happen.
• we can get word length by looking at fm.size(Qt.TextSingleLine, word).width() but then we need to figure out where to add the newline, so what we actually want to do is parse each character in the text, which is simiilar to the workaround offered in the bug report, but it doesn't seem to take into account non-word characters and it's much less sophisticated than Qt's current implementation for wrapping
• i still think we should stick to the plan to document this as a known upstream bug and wait for our bug report to be addressed while we work on larger securedrop issues

[eloquence]

Thanks much for the spike @creviera and the upstream follow-up. We checked in on this issue with @emkll today and agreed to again park this again on the backlog for now, giving upstream additional time to respond.

To add to Allie's recap: The workaround to preempt wrapping certain long strings before Qt does it risks introducing regressions for some long strings that are currently wrapped nicely (e.g., very long URLs wrapped at a /). In those cases we'd go from a clean wrap without a newline, to a dirty (potentially mid-word) wrap with a newline. It also seems rather finicky to match custom wrapping against Qt's.

We don't even fully understand under what circumstances we'd be overriding Qt's behavior, since we still don't have a full list of Qt's wrap point characters.

A simpler workaround may be to just enable horizontal scrollbars for a message bubble iff (if and only if) a message includes an overlong, unwrapped string. We would want to ensure that other strings that are part of the same message are still wrapped correctly, without a need for scrolling. This is not a very high priority issue but one we'll ultimately want to resolve, so we'll check in again in the n+1 or n+2 sprint.

[eloquence]

I'm not sure if this is a regression or not, but when following the test plan for freedomofpress/securedrop-workstation#751 on securedrop-client_0.6.0-dev-20220331-060621, I observed that when sending a reply like

0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789

I cannot scroll to reveal its whole contents. I can use Ctrl+A/Ctrl+C to select the whole content and then copy and paste it into the app to see it, though. Is that the currently expected behavior, or was it previously possible to scroll within these replies to see the contents?

[sssoleileraaa]

Yup this is exactly the issue being tracked upstream in https://bugreports.qt.io/browse/QTBUG-85498 (I updated the title to make that more clear). If I recall correctly, the workarounds are to either switch to QPlainTextEdit which will wrap long strings of alphanumerics that have no spaces or to create or own QLabel wrapping function that adds newlines in appropriate locations (which we would want to remove when a user copies the content of the message).

[eloquence]

OK cool. I'll update our test plan to make it clearer that long, uninterrupted strings will be cut off until this issue is fixed.