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

Refactor ensure signals are used as recommended by Qt #1506

Closed
wants to merge 2 commits into from

Conversation

gonzalo-bulnes
Copy link
Contributor

Description

The controller used to imperatively emit signals on the Export instances. This breaks the encapsulation provided by the Export class and defeats one of the main benefits of using signal: de-coupling components.

That, in turn, is one of the situations that made it more difficult for me to wrap my head around this code.

From Qt's documentation:

Signals are public access functions and can be emitted
from anywhere, but we recommend to only emit them
from the class that defines the signal and its subclasses.

The controller now emits a signal indicating that a preflight check is desirable (for example) and the Export instance connects its own slots to that signal.

See https://doc.qt.io/qt-5/signalsandslots.html

Note: As soon as we think of signals as a representation of events, it becomes a lot easier to conventionally name them as past tense phrases (e.g. some_event_happened), like we've been trying to do for a little while.

Test Plan

  • Confirm that the signal names are meaningful (I think they make a lot more sense than they used to, if we think of them as events that happened, which is how Qt uses them everywhere.)
  • Verify that the test suite is green 🍏
  • Confirm that you can export a file as usual
  • Confirm that you can print a file as usual

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 controller used to imperatively emit signals
on the Export instances. This breaks the encapsulation
provided by the Export class and defeats one of
the main benefits of using signal: de-coupling components.

From Qt's documentation:

    Signals are public access functions and can be emitted
    from anywhere, but we recommend to only emit them
    from the class that defines the signal and its subclasses.

The controller now emits a signal indicating that
a preflight check is desirable (for example) and the Export
instance connects its own slots to that signal.

See https://doc.qt.io/qt-5/signalsandslots.html
We currently follow the convention of naming the slots
after the signals that are connected to them.
@gonzalo-bulnes gonzalo-bulnes requested a review from a team as a code owner June 1, 2022 06:26
@gonzalo-bulnes gonzalo-bulnes added this to In Development in SecureDrop Team Board Jun 2, 2022
@gonzalo-bulnes gonzalo-bulnes removed this from In Development in SecureDrop Team Board Jun 2, 2022
@gonzalo-bulnes
Copy link
Contributor Author

gonzalo-bulnes commented Jun 2, 2022

It was a mistake to try and make this PR independent from #1501, it is not. I'll re-do. Mini-rant: I think we really need to find better ways to handle our review bottleneck. The work I'm doing aims at making our code easier to work with and review, but it's really hard when changes are being withheld on a regular basis.

Because I'm conscious of that bottleneck, I'm trying to keep PRs small, and make them independent so that more than one can be reviewed before requiring rebases (with roughly a day turnaround). However, by definition or scope, I'm working on related topics that touch on related files. Ultimately, the refactoring on this branch is moot because the tests in test_logic.py file don't have any assertions and don't test anything. (That's what #1501 fixes.) There is no refactoring without a test suite 🤷‍♀️ but rebasing this branch after the fact (now that I realized I made the mistake) is going to trigger conflicts in pretty much every test line. The point of my rant is that even if the mistake is undoubtedly mine, it wouldn't have happened if #1501 had been in main, or if I wasn't trying to chunk the PRs that aggressively to give them a chance of being reviewed and merged within a reasonable time frame. 😞

@gonzalo-bulnes gonzalo-bulnes changed the title Refactor ensure signals are used as such Refactor ensure signals are used as recommended by Qt Jun 4, 2022
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.

None yet

1 participant