-
Notifications
You must be signed in to change notification settings - Fork 39
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
Adds wordwrap and text selectability for messages and replies #1050
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to make sure your fix does not reintroduce the issue of removing paragraphs. Also the lines should wrap when the reach teh width plus some padding of the QLabel - in this PR the lines wrap before they get close to the end of the label. I think we should continue to use Qt's wordwrapping feature and specifically fix the issue where long strings without spaces or dashes don't wrap.
@creviera any tips on how do you want to go ahead for this? |
|
For a note, https://github.com/qt/qtbase/blob/60feaae1967aca5c7b1b0f6cb732962ddd1b2115/src/widgets/widgets/qlabel.cpp#L1197 is the code doing the wrap inside of the Qt. |
The first method varies quite a bit with the font that we're using, meaning the widest character is a lot wider than the smallest character, so we would have to add a newline before the very long string with no spaces or dashes gets close to the end of the speech bubble. I ran into this issue with elided text, so you can see how this is done there. We've talked about the second way before, because it would give us more controller over the wrapping policy - with labels we are only given a boolean config to turn wrapping on or off. I'll see if there's a third option this morning. and we'd have to watch out any time we make changes to the |
After more research I think the cleanest thing to use is QTextEdit, and we should make a SecureQTextEdit class the way we did with SecureQLabel. And then we can update our speech bubble messages from SecureQLabel to SecureQTextEdit. This also means add a little CSS around speech bubble size to make sure it's fitted to the text. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changing all our QLabels into QTextEdits will make this a bigger change to test, so I think instead of switching SecureQLabel to derive from a QTextEdit we should create an entirely new SecureQTextEdit class and use it only for speech bubbles for now. for instance, one regression is that source selection doesn't work if you click on the preview snippet area.
I thought updating the size policy of the speech bubbles would fit the textedit to the text. I can give this a try and see why it isn't working for you (might be an issue with a parent's size policy). also we shouldn't see the scrollbar for messages or replies.
I currently just kept the same name |
I agree with @creviera that this PR should not alter the behavior of anything other than speech bubbles, which is the only instance we know of where there is currently a rendering issue. If we change widgets we may also want to compare rendering performance with long conversations (>50 but <100 messages). |
@kushal feel free to undo the last commit but i wanted you to see what i mean by only changing the |
@kushaldas last commit is a solution that sets the height of the textedit based on number of lines of each block in the document and line height. this is not a finalized solution rather an idea i want you to take a look at. the solution is to (1) get the height of the QPlainTextEdit after setting its text, (2) set the height of the QPlainTextEdit to what we calculate, (3) set the height of I'm not super happy with my implementation, but i think the overall idea we should consider keeping: create a generic way to get and set the height of a QPlainTextEdit based on the number of lines of text. |
We need a new reviewer for this since I added commits to get the resizing of the speech bubbles to work
Okay, now I'm happy with my implementation to fix the textedit resize issue, and I updated our unit tests, so this is ready for a review from someone other than me. I also updated the PR description to resolve #1052. We will need to add support for multi-message-select in a separate PR. |
153972f
to
974189d
Compare
How to test?In addition to the test plan above, send messages and replies with the following strings (plus any other test strings you can think of):
How to test select-copy-paste?This supports single message select. Even if you select multiple messages, you can only right-click copy over the selected text you want to copy and then you can test pasting by pasting it into the reply box. We can look into deselecting text from speech bubbles when you click outside of them in a separate PR after we spend more time discussing in this week's UX meeting. |
I see this too. When trying to come up with a way to get the real number of lines, which will allow us to resize the
line_count = math.ceil(block.length() / self.document().idealWidth())
total_line_count = total_line_count + line_count
So now I can finally answer why this message is calculated to be 4 lines instead of 5. We currently don't correctly calculate the total sum of the pixel width of each character in a block, because The dots character length is 130 and the W's is 42. So next we need to loop through each character in the block, get its width, and total everything up. Then we need to divide by the maximum pixel width allowed for a line. I'm still not 100% sure |
Thanks for this detailed explanation! This makes sense - if |
|
Thinking more, that's what we can use for getting the total sum of the pixel width of each character in a block, but we'll want to use |
Looks like UpdateI found out that But feel free to test this now to see how our example messages and replies look correct now... at least they do for me so far. I'll create an updated list of text strings to test. So far I've been using: #1050 (comment) and the following:
|
Looks like this isn't quite ready because
This looks like: But the length of each line, calculated using line 1=90 So here's what's left (cc @kushaldas, since you're in a diff timezone and in case you can take a look until I come back to this tomorrow):
|
Here is a screenshot with lines including one TAB and then multiple TABs.
The second bubble has less height, in my mind it should be having more as the length is bigger. |
Excellent progress, this is looking much closer.
There is a document margin that is 4 pixels ( |
This reverts commit f4202b3.
(One relevant finding on the tab business I want to drop here is that QFontMetrics.size is another method that returns pixel width that allows one to pass a flag ( |
We separated out adding a message-copy feature from this PR, which has already been merged (#1063). The work in this PR has entirely been focused on fixing the issue #815 (message copying was more of a side effect and distraction from the original problem since this was opened to solve #815). We are leaving this PR open while we look at taking a different path to fixing the QLabel wrapping issue described in #815. @kushaldas and I will be pasting research notes in the issue instead of in this PR. |
Update from today's research on the topic.
|
I think I have a break through. I will report back on the issue at night. |
Created a related issue about the size of the message https://github.com/freedomofpress/securedrop-client/issues/1072 |
Now I am rendering the text inside of QWebEngineView, with some amazing CSS hack from @SaptakS, this way we are stopping the overflow of the text, can show the emoji and random other text properly. I will work on the branch tomorrow to clean up more. |
Cross-referencing Allie's and Jen's comments here: Per those comments I'm moving this PR to the near-term backlog for now. I would actually advocate in favor of closing it and keeping the branch, for the avoidance of confusion, since the scope has changed significantly since the PR was opened. |
Closing but let's keep the branch around |
Description
Fixes #815
Wraps the text at the default 70 length
Test Plan
Follow the STR at #998
Initial test data:
Checklist
If these changes modify code paths involving cryptography, the opening of files in VMs or network (via the RPC service) traffic, Qubes testing in the staging environment is required. For fine tuning of the graphical user interface, testing in any environment in Qubes is required. Please check as applicable:
If these changes add or remove files other than client code, the AppArmor profile may need to be updated. Please check as applicable: