Refactor extract conversation FileWidget #1369

gonzalo-bulnes · 2021-12-07T08:12:19Z

Description

This is preliminary work relevant to #1354. This PR moves (but doesn't otherwise modify) the widgets involved in drawing the files (FileWidget) in the conversation timeline. Providing user feedback while downloading the files will expand on existing FileWidget behavior, and it will be easier to manage change in a small file than it would be in widgets.py.

Goal: Make clearer which widgets depend on which, and package together the ones that are likely to change together. For the package names, I went for "domain concepts" (e.g. Source, Conversation, authentication...) and features (e.g. export, print...). The idea is that the domain should only change slowly (stability is good) and features can be added or removed all at once, so it's nice when they're relatively independent and clearly identified. Naming is hard and YMMV, please comment what you think!! 🙂

Package structure at this point

tree securedrop_client/gui/

securedrop_client/gui/     # These UI components are known externally as...
├── __init__.py
├── auth
│   ├── __init__.py
│   └── dialog.py          # ...LoginDialog
├── base   # Contains generic components (e.g. PasswordEdit, toggle button...)
│   ├── __init__.py
│   ├── buttons.py
│   ├── dialogs.py 
│   ├── inputs.py 
│   └── misc.py     
├── conversation
│   ├── __init__.py
│   ├── export
│   │   ├── __init__.py
│   │   └── dialog.py      # ...ExportDialog
│   ├── file.py            # ...FileWidget
│   └── print
│       ├── __init__.py
│       └── dialog.py      # ...PrintDialog, eventually print.Dialog etc.
├── main.py
└── widgets.py

Closes #1340

Test Plan

Confirm that CI build is green
Confirm that the FileWidget appearance hasn't changed
Confirm that the Login, Export and Print dialogs looks the same as usual
Check that the documentation headers of the new packages make sense

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

The FileWidget is only ever used as part of a conversation, and the new package structure reflects that. I only touched the related files and I grouped them around domain concepts and features.

The packages provide namespacing, and class names don't need to be globally unique. Simpler names also make easier to highlight the parallels between different packages.

gonzalo-bulnes · 2021-12-13T22:38:27Z

tests/integration/test_styles_sdclient.py

-
-def test_class_name_matches_css_object_name_for_export_dialog(export_dialog):
-    assert "ExportDialog" == export_dialog.__class__.__name__
+def test_preferred_name_matches_css_object_name_for_export_dialog(export_dialog):
    assert "ExportDialog" in export_dialog.passphrase_form.objectName()


These two specific assertions are not met anymore. I decided not to touch the stylesheet in order to keep the changes scoped (they're large already 😬).
Eventually, I think that breaking the stylesheet into smaller files that follow the package structure would make maintenance easier, and also would make redundant the need to have global matching names for Python classes and QSS names.

Until then, PrintDialog and ExportDialog seems to me that they remain the most appropriate QSS names for conversation.ExportDialog and conversation.PrintDialog (even though they are called export.Dialog and print.Dialog as an implementaiton detail of the conversation package.) Does this make sense?

The convention, enforced by these tests, is to use the class name as the object name in the CSS to make it easier to find the relevant code. For now, let's stick to this convention and rename the CSS objects to match the new class names. This will make it cleaner when you're ready to move the relevant CSS code to the new project structure.

The direction of this PR is great! I just have one main concern, which is that this change, left as is, breaks a couple conventions:

tests should exist in a test structure that mirrors the project structure, e.g. tests that test securedrop_client/gui/dialogs.py need to go in tests/gui/test_dialogs.py, not tests/gui/test_widgets.py

CSS object names should match the Python classes they are used in

If you refactor one class at a time, you can migrate the relevant tests so that you don't break these conventions, and it's still small enough for the reviewer.

Also, regarding CSS, were you thinking of doing something like this eventually?

styles/gui ├── auth │ └── dialog.css ├── buttons.css ├── conversation │ └── print │ └── dialog.css ├── dialogs.css └── ...

Or would you move the CSS files into the project structure like this:

securedrop_client/gui ├── __init__.py ├── auth │ ├── __init__.py │ └── dialog.py │ └── dialog.css ├── buttons.py ├── buttons.css ├── conversation │ ├── __init__.py │ ├── export │ │ ├── __init__.py │ │ └── dialog.py │ ├── file.py │ └── print │ ├── __init__.py │ └── dialog.py ├── dialogs.py ├── dialogs.css └── ...

Or keep it flat:

styles ├── auth_dialog.css ├── print_dialog.css ├── dialogs.css └── ...

I can add those changes 👍

Regarding the stylesheets location, I have a strong preference for the second layout you described, where the all the parts of a given component are grouped together, for a few reasons:

That option splits concerns by component, which in our case closely aligns with the domain boundaries (e.g. auth, conversation, source, "conversation export feature", "source deletion feature" are concepts that are shared by the team beyond the code.)

That option also makes clear the fact that components are scoped to themselves. Because all the files are grouped together it makes less likely that dependencies will be drawn from unrelated places.

I find the separation by language just as arbitrary and not very convenient in practice when I work with components. After all, QSS and Python code do indeed change together (we even have tests to check object names and QSS names are in sync!). YMMV (Support material - that entire side deck is interesting IMHO)

What do you think @creviera? (and @rocodes @Er1kkkaaa!)

Note: this thread is relevant to #303

I agree with your points. 🚀

Since there's a lot of code moving around, it might make sense to break this up into multiple commits or PRs scoped to moving one class/file at a time, e.g. move ModalDialog and any associated css and tests from widgets.py, test_widgets.py, and some-name.css into dialogs.py, test_dialogs.py, and dialog.css. This should make both of our lives much easier during the migration and review of the migration. Otherwise, I'm down for a slower review where I piece it all together in the diffs.

Since there's a lot of code moving around, it might make sense to break this up into multiple commits or PRs scoped to moving one class/file at a time [...]

Yes! I was thinking the same, I'll "re-package" this PR (and #1373) into one-class-at-a-time chunks. 👍

For road mapping purposes, I'll proceed as follows:

misc (removing all class definitions from gui/__init__.py) Remove component definitions from package __init__ file #1377

auth (currently is the smaller package)

actions (made possible by ☝️ if I remember correctly)

conversation/file

conversation/export

conversation/print

I'm not 100% sure about the end of the sequence, but I'll update when we get there.

sssoleileraaa · 2021-12-15T18:28:28Z

@gonzalo-bulnes should we close this PR since you're breaking it up now?

gonzalo-bulnes · 2021-12-19T01:02:28Z

@creviera The review plan here is:

Refactor move generic widgets to 'gui.base' namespace #1382
Refactor extract PasswordEdit to 'gui.base' namespace #1378 (requires a rebase after the previous one is merged)
Refactor extract SignInButton to 'gui.auth.sign_in' namespace #1379 (requires a rebase after the previous one is merged)
Refactor extract LoginErrorBar to 'gui.auth.sign_in' namespace #1380 (requires a rebase after the previous one is merged)
Refactor extract LoginOfflineLink to 'gui.auth.use_offline' namespace #1381 (requires a rebase after the previous one is merged)
Refactor move LoginDialog to 'gui.auth' namespace #1383 (depends on all the above, so requires a rebase after the previous one is merged)

Happy to 🍐 on this (you review, I rebase the branches as we go).

⚠️ Note:

@creviera Before merging [any of these] PR[s], let's talk about signatures. It seems that the author signature gets stripped when merging without a merge commit, resulting in an unsigned commit in main. We could put extra attention on this one and see if we can merge while preserving the signatures on the commits that introduce diffs. (The automated merge commit should be empty because we update/rebase out branches before merging.) (original comment)

gonzalo-bulnes · 2022-05-17T03:41:39Z

I'll close this PR, as the updated checklist is done now 🙂

Note that nor all the moves were actually performed (initial intent), but I think those can be performed as we go.

Refactor extract conversation FileWidget

cc297bf

The FileWidget is only ever used as part of a conversation, and the new package structure reflects that. I only touched the related files and I grouped them around domain concepts and features.

gonzalo-bulnes requested a review from a team as a code owner December 7, 2021 08:12

gonzalo-bulnes requested review from sssoleileraaa and rocodes December 7, 2021 08:17

Refactor rename widgets to leverage namespaces

ef18321

The packages provide namespacing, and class names don't need to be globally unique. Simpler names also make easier to highlight the parallels between different packages.

gonzalo-bulnes mentioned this pull request Dec 8, 2021

Refactor extract conversation menu #1373

Closed

14 tasks

gonzalo-bulnes requested a review from Er1kkkaaa December 8, 2021 22:10

gonzalo-bulnes commented Dec 13, 2021

View reviewed changes

gonzalo-bulnes mentioned this pull request Dec 15, 2021

Remove component definitions from package __init__ file #1377

Merged

17 tasks

gonzalo-bulnes mentioned this pull request Dec 19, 2021

Refactor move LoginDialog to 'gui.auth' namespace #1383

Merged

16 tasks

gonzalo-bulnes added the on hold label Dec 20, 2021

This was referenced Jan 6, 2022

Refactor move ModalDialog to 'gui.base' namespace #1390

Merged

Refactor move DeleteConversationDialog to 'gui.conversation' namespace #1393

Merged

gonzalo-bulnes mentioned this pull request Jan 13, 2022

Refactor move DeleteSourceDialog to 'gui.source' namespace #1394

Merged

15 tasks

gonzalo-bulnes mentioned this pull request Jan 25, 2022

Refactor move user actions to 'gui.actions' namespace #1400

Merged

20 tasks

sssoleileraaa marked this pull request as draft March 15, 2022 23:10

gonzalo-bulnes closed this May 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Refactor extract conversation FileWidget #1369

Refactor extract conversation FileWidget #1369

Uh oh!

gonzalo-bulnes commented Dec 7, 2021 •

edited

Loading

Uh oh!

gonzalo-bulnes Dec 13, 2021

Uh oh!

sssoleileraaa Dec 14, 2021

Uh oh!

sssoleileraaa Dec 14, 2021 •

edited

Loading

Uh oh!

gonzalo-bulnes Dec 14, 2021 •

edited

Loading

Uh oh!

sssoleileraaa Dec 14, 2021

Uh oh!

gonzalo-bulnes Dec 14, 2021

Uh oh!

gonzalo-bulnes Dec 15, 2021 •

edited

Loading

Uh oh!

sssoleileraaa commented Dec 15, 2021

Uh oh!

gonzalo-bulnes commented Dec 19, 2021 •

edited

Loading

Uh oh!

gonzalo-bulnes commented May 17, 2022

Uh oh!

Uh oh!

Refactor extract conversation FileWidget #1369

Refactor extract conversation FileWidget #1369

Uh oh!

Conversation

gonzalo-bulnes commented Dec 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Package structure at this point

Test Plan

Checklist

Uh oh!

gonzalo-bulnes Dec 13, 2021

Choose a reason for hiding this comment

Uh oh!

sssoleileraaa Dec 14, 2021

Choose a reason for hiding this comment

Uh oh!

sssoleileraaa Dec 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gonzalo-bulnes Dec 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sssoleileraaa Dec 14, 2021

Choose a reason for hiding this comment

Uh oh!

gonzalo-bulnes Dec 14, 2021

Choose a reason for hiding this comment

Uh oh!

gonzalo-bulnes Dec 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sssoleileraaa commented Dec 15, 2021

Uh oh!

gonzalo-bulnes commented Dec 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gonzalo-bulnes commented May 17, 2022

Uh oh!

Uh oh!

gonzalo-bulnes commented Dec 7, 2021 •

edited

Loading

sssoleileraaa Dec 14, 2021 •

edited

Loading

gonzalo-bulnes Dec 14, 2021 •

edited

Loading

gonzalo-bulnes Dec 15, 2021 •

edited

Loading

gonzalo-bulnes commented Dec 19, 2021 •

edited

Loading