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

Elide source designation at lower window widths #1145

Merged
merged 2 commits into from
Sep 24, 2020

Conversation

eloquence
Copy link
Member

@eloquence eloquence commented Sep 4, 2020

Resolves #1120

Status

Ready for review

Description

Elides the source designation if needed as the window is resized, while allocating a fixed width to the label to ensure that layout is not disrupted.

Testing

  • Test resizing the window on Qubes for multiple sources to ensure it behaves as shown in the GIF animation below.
  • Test switching to offline mode and back to online mode to ensure no regression in the placeholder display.
  • Test sending replies from multiple sources to ensure no regression in placeholder hide/show behavior.

resize

Checklist

  • No update to the AppArmor profile is required for these changes
  • No database schema changes are needed

@eloquence eloquence force-pushed the 1120-elide-is-a-good-word branch 2 times, most recently from 7807daf to df72972 Compare September 4, 2020 21:13
@eloquence
Copy link
Member Author

eloquence commented Sep 4, 2020

OK, this was pretty painful to debug, but it seems like the best way to avoid layout problems while resizing the window is to allocate a generous fixed width to the label while its actual contents change. That way Qt won't attempt to reflow the composite placeholder widget in sometimes broken ways.

Now to test this in Qubes!

@eloquence eloquence changed the title [WIP] Elide source designation at lower window widths Elide source designation at lower window widths Sep 4, 2020
@eloquence
Copy link
Member Author

In my testing resize behavior is correct in Qubes :)

@eloquence
Copy link
Member Author

test-buster fail is due to coverage. I would appreciate a first pass on the implementation -- does this approach make sense, or is there a simpler, more resilient or more Qt-y way to do this? If the approach is sound, will add required tests.

Copy link
Contributor

@sssoleileraaa sssoleileraaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did a first pass on your implementation and agree with your approach on changing max_length based on window size. all you need is test coverage and i'd say this is good to go in. let me know if you'd like help on writing the tests.

source_name.setObjectName("ReplyTextEditPlaceholder_bold_blue")
self.source_name = source_name
self.source_name_label = SecureQLabel(
source_name, wordwrap=False, max_length=self.INITIAL_MAX_WIDTH
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, setting max_length here will turn on elided text

@@ -3133,8 +3133,25 @@ def setText(self, text):
self.placeholder.hide()
super(ReplyTextEdit, self).setPlainText(text)

def resizeEvent(self, event):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hooking into the resize event works as expected

@eloquence eloquence force-pushed the 1120-elide-is-a-good-word branch from cd3e680 to bcd1971 Compare September 11, 2020 00:20
@eloquence eloquence marked this pull request as ready for review September 11, 2020 00:26
@eloquence
Copy link
Member Author

Awesome, thanks for the review @creviera! :) We're at 100% coverage now and the test verifies the basic behavior implemented here. Please let me know if it's consistent with our approach to testing these widgets, happy to tweak.

Copy link
Contributor

@sssoleileraaa sssoleileraaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Elide long source designations with "..."
2 participants