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

scale source list and preview as window resizes #1211

Merged
merged 1 commit into from
Feb 2, 2021
Merged

Conversation

sssoleileraaa
Copy link
Contributor

@sssoleileraaa sssoleileraaa commented Jan 8, 2021

Description

Fixes #1207
Towards scalable client
Merge before #1206
Known issue: #815

Test Plan

  1. Run the client against a server with some sources
  2. Log in
  3. Select a source and resize the client window, try double clicking and resizing to the smallest width and height
    • Confirm there is never horizontal scrollbar in the source list
    • Confirm that preview adjusts width as you resize the client window
  4. Replace existing sources with 200 new sources while still logged in
    • Confirm there is never horizontal scrollbar in the source list
    • Confirm that preview adjusts width as you resize the client window (make sure to scroll down the source list as the previews are updating and resize)
  5. Log out
    • Confirm there is never horizontal scrollbar in the source list
    • Confirm that preview adjusts width as you resize the client window
  6. Close client and log in in offline mode
  7. Log in, restart the server with new sources, wait until the client picks up the new sources and creates new source widgets while logged in
    • Confirm there is never horizontal scrollbar in the source list
    • Confirm that preview adjusts width as you resize the client window

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:

  • I have tested these changes in the appropriate Qubes environment
  • I do not have an appropriate Qubes OS workstation set up (the reviewer will need to test these changes)
  • These changes should not need testing in Qubes

If these changes add or remove files other than client code, the AppArmor profile may need to be updated. Please check as applicable:

  • I have updated the AppArmor profile
  • No update to the AppArmor profile is required for these changes
  • I don't know and would appreciate guidance

If these changes modify the database schema, you should include a database migration. Please check as applicable:

  • I have written a migration and upgraded a test database based on main and confirmed that the migration applies cleanly
  • I have written a migration but have not upgraded a test database based on main and would like the reviewer to do so
  • I need help writing a database migration
  • No database schema changes are needed

@sssoleileraaa sssoleileraaa changed the title resizable preview scale source list and preview as window resizes Jan 8, 2021
@sssoleileraaa
Copy link
Contributor Author

I need to write tests still, but this is ready for early review since this will go in first (before #1206 and the upcoming PR to fix #1205).

@emkll emkll self-assigned this Jan 11, 2021
@emkll
Copy link
Contributor

emkll commented Jan 11, 2021

Tested in Qubes per the test plan in the PR description. The resizing behavior here works very well, some comments/clarifications regarding the scrollbar and 1207 fix, see below:

  1. Run the client against a server with some sources
  2. Log in
  3. Select a source and resize the client window, try double clicking and resizing to the smallest width and height
    • ❗ Confirm there is never horizontal scrollbar in the source list
      I can see a scroll bar, see screenshots in [1] below. Setting a Horizontal scrollbar policy per the diff in [2] appears to resolve here, without any side effects that I can identify. When scrolling without the policy, it's only whitespace, is there a reason we shouldn't use the policy here?
    • Confirm that preview adjusts width as you resize the client window
  4. Replace existing sources with 200 new sources while still logged in
    • ❗ see above Confirm there is never horizontal scrollbar in the source list
    • Confirm that preview adjusts width as you resize the client window (make sure to scroll down the source list as the previews are updating and resize)
  5. Log out
    • ❗ Confirm there is never horizontal scrollbar in the source list
    • Confirm that preview adjusts width as you resize the client window
  6. Close client and log in in offline mode
    • Ensure Preview text shorter when starting offline mode #1207 is fixed
      There is a very significant improvement here, but there still appear to be some edge cases with punctuation/weird characters and whitespaces, as well as the right pane of the source list shifting ~5-10px between offline and online modes (This is only observable when quickly shuffling through the screenshots 1. and 2. in [1] below). For reference I've added screenshots of the current behavior on develop in 4. and 5. below: The right pane does not shift, but the discrepancy in the text is much greater.

[1]

  1. Offline Mode (this branch)
    test41scale

  2. Logged in (this branch)
    test42-scale

  3. Scaled down (this branch)
    client44-scalesmall

  4. Horizontal scroll bar policy
    test43-scale-scroll

  5. Offline Mode (main)
    test51

  6. Logged in (main)
    test52

[2] Set Horizontal Scroll Bar policy to disable the bar:


diff --git a/securedrop_client/gui/widgets.py b/securedrop_client/gui/widgets.py
index 79eaa20..fe483e8 100644
--- a/securedrop_client/gui/widgets.py
+++ b/securedrop_client/gui/widgets.py
@@ -823,6 +823,9 @@ class SourceList(QListWidget):
         # To hold references to SourceListWidgetItem instances indexed by source UUID.
         self.source_items = {}
+        # Disable horizontal scrollbar for SourceList widget
+        self.setHorizontalScrollBarPolicy(Qt.ScrollBarAlwaysOff)
+
     def resizeEvent(self, event):
         self.adjust_preview.emit(event.size().width())
         super().resizeEvent(event)

@sssoleileraaa
Copy link
Contributor Author

Oh you're right! That horizontal scrollbar isn't supposed to be there. Fixed.

  1. Close client and log in in offline mode
    • Ensure Preview text shorter when starting offline mode #1207 is fixed
      There is a very significant improvement here, but there still appear to be some edge cases with punctuation/weird characters and whitespaces, as well as the right pane of the source list shifting ~5-10px between offline and online modes (This is only observable when quickly shuffling through the screenshots 1. and 2. in [1] below). For reference I've added screenshots of the current behavior on develop in 4. and 5. below: The right pane does not shift, but the discrepancy in the text is much greater.

It sounds like an issue with how we calculate the width of a QLabel that contains certain special characters. However I don't see the issue. Here's what I'm trying to do to repro what you're describing here:

  1. Start the client in offline mode with a server that was initialized with 40 sources: NUM_SOURCES=40 make dev (screen will start off maximized)
  2. Log in (screen remains maximized)

I was expecting to see the width of the source list expand ~5-10px when switching to online mode, but it seems to stay the same. My guess is that you are doing something slightly different to test or you have different preview strings?

@sssoleileraaa
Copy link
Contributor Author

I noticed your screenshots have the horizontal scrollabar. Now that it's disabled, try again, and this is what I'd expect you to see: Peek 2021-01-29 10-51_1

@emkll
Copy link
Contributor

emkll commented Jan 29, 2021

Confirmed, removing the scrollbar did resolve the issue. I've successfully gone through the test plan on the latest revision.

@sssoleileraaa sssoleileraaa marked this pull request as ready for review January 30, 2021 02:12
@sssoleileraaa sssoleileraaa force-pushed the scale-sourcelist branch 3 times, most recently from 5c90208 to dba9711 Compare February 1, 2021 08:50
@emkll
Copy link
Contributor

emkll commented Feb 1, 2021

I looks like all the tests are passing in CI, but Black and Isort are not pleased:

Oh no! 💥 💔 💥
1 file would be reformatted, 80 files would be left unchanged.
make: *** [Makefile:15: check-black] Error 1
ERROR: /home/circleci/project/securedrop_client/gui/widgets.py Imports are incorrectly sorted.

Perhaps in the future it would make sense to have a separate CI target for linting, which would make detecting these issues much quicker. Happy to open a ticket if you agree.

@sssoleileraaa
Copy link
Contributor Author

I added one more test to the test plan to reflect a bug I found while running through this again. See number 7 in the PR description.

Copy link
Contributor

@emkll emkll left a comment

Choose a reason for hiding this comment

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

Sourcelist scaling looks great based on the test plan:

  1. Run the client against a server with some sources
  2. Log in
  3. Select a source and resize the client window, try double clicking and resizing to the smallest width and height
    • Confirm there is never horizontal scrollbar in the source list
    • Confirm that preview adjusts width as you resize the client window
  4. Replace existing sources with 200 new sources while still logged in
    • Confirm there is never horizontal scrollbar in the source list
    • Confirm that preview adjusts width as you resize the client window (make sure to scroll down the source list as the previews are updating and resize)
  5. Log out
    • Confirm there is never horizontal scrollbar in the source list
    • Confirm that preview adjusts width as you resize the client window
  6. Close client and log in in offline mode
  7. Log in, restart the server with new sources, wait until the client picks up the new sources and creates new source widgets while logged in
    • Confirm there is never horizontal scrollbar in the source list
    • Confirm that preview adjusts width as you resize the client window

@emkll emkll merged commit 124812b into main Feb 2, 2021
@emkll emkll deleted the scale-sourcelist branch February 2, 2021 14:21
@eloquence eloquence added this to the 0.4.1 milestone Feb 19, 2021
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.

Preview text shorter when starting offline mode
3 participants