Refactor export service to accomodate polling printer and disk #1615
Conversation
gonzalo-bulnes
force-pushed
the
refactor-export-service-to-accomodate-polling-printer-and-disk
branch
7 times, most recently
from
9e3abe2
to
fbc4327
Compare
gonzalo-bulnes
force-pushed
the
refactor-export-service-to-accomodate-polling-printer-and-disk
branch
2 times, most recently
from
6080b55
to
984e137
Compare
gonzalo-bulnes
force-pushed
the
refactor-export-service-to-accomodate-polling-printer-and-disk
branch
3 times, most recently
from
5438466
to
9ff352c
Compare
The new directory structure will make extension easier.
Besides being the dedicated tool to assert on Qt signal emissions,
QSignalSpy can track when signals are emitted from other signals,
which the mocking strategy misses.
Example:
some_signal.connect(some_other_signal)
# verify that some_other_signal is emitted
See https://doc.qt.io/qt-5.15/qsignalspy.html
I'll use an expand and contract pattern to refactor the export.Service tests, and keeping this file distinct while it is used as a reference will be more convenient and minimize the change sets. Important: this file is still fully operational and will be kept as such until all deprecated methods have been removed from export.Export (soon to be renamed export.Service).
Refactor tests to: - distinguish testing of GUI API, and CLI API internals - improve readability
Decorate static methods (from what didn't need to be class methods) Tests were minially modified: patching static methods must be done unsing unittest.mock.patch.object to preserve the independence of the unit tests. Assigning to the class attributes directly does modify the class for the entire test session. (Not desirable.)
I am leaving the deprecated tests as untouched as possible to make easier to review the changes.
This will ensure that one one instance of the export service queries the sd-devices CLI. That was already the case, but nothing was done to suggest it. Additionally, this will allow to simplify the export service injection by making possible to retrieve it like a logger. Note: I haven't stopped injecting the export_service into the main.Window because I couldn't figure out how to prevent what seems to be a fixture scope error that affects only half of the integration tests. That seems like something that can be resolved, so I don't consider it to be a sign of export.Service being a bad idea.
…ce (1) These are now replaced by "printer check" and "disk check" which have the advantage of referring to concrete physical devices.
gonzalo-bulnes
force-pushed
the
refactor-export-service-to-accomodate-polling-printer-and-disk
branch
from
9ff352c
to
c634cee
Compare
|
(rebased onto latest @rocodes I need to go through this PR myself for a self-review, but everything should be pretty much in place. Edit: I'll mark the PR as ready for review even with the caveat above, and let's start wrapping it up. |
|
|
||
| class _Poller(QStateMachine): | ||
| """Allow a function to ge called repeatedly, with a delay between calls. | ||
|
|
There was a problem hiding this comment.
[nit]: typo, "ge" for "be"
| from .cli import Error as ExportError # noqa: F401 | ||
| from .cli import Status as ExportStatus # noqa: F401 |
There was a problem hiding this comment.
These two wouldn't be exported if it was up to me, because they leak an implementation detail IMHO. But I decided that fixing that was out of scope. Please let me know if you disagree, I'd happily fix it!!
| dialog = conversation.PrintFileDialog(self._export_device, self.uuid, self.file.filename) | ||
| file_location = self.file.location(self.controller.data_dir) | ||
| dialog = conversation.PrintFileDialog(self._printer, file_location, self.file.filename) |
There was a problem hiding this comment.
Note: mypy didn't catch when conversation.PrintFileDialog was receiving an export.Device but requiring an export.Printer. : /
There was a problem hiding this comment.
Hi @gonzalo-bulnes, thank you for preparing this. This is an initial visual review- I plan to step through regression testing tomorrow and may have more feedback then, but will finish my review before end of week. I also committed to looking at the testing a bit and that's still on my plate. :)
A few of the smaller inline comments are about documentation--since this code introduces some new patterns, it would be great to have more explanation in docstrings etc for those of us new to this style/changes.
One slightly larger question I have is surrounding service.py, disk.py, and printer.py. Since Disk and Printer are very similar, and are created/used in similar places, and rely on similar state machines, is there maybe an opportunity to DRY things up a bit? Or, further: do you think it's necessary to have all 3 classes, as opposed to moving the poll/state machine logic to service.py and dispensing with the other classes? (Essentially, I see a generic "Is the thing connected?" service, with configurable poll interval and a last known status, and I wonder if the printer and disk classes are adding extra complexity).
Another question I have is around the polling. Do you feel there's a benefit to introducing that code now, though we won't make use of it until farther down the line? (This is something I might tap another maintainer on for their opinion as well. I know there is precedent, since we introduced the service.py code in sd-export that is not yet in use, but my impression of polling was that it might be farther off as a milestone, since it would probably mean more client changes first. My inline comment goes into a bit more depth on that--would like to know your thoughts.)
Thank you again for all your hard work on this. More TK.
|
|
||
| paused --> started: registered_starting_signal_emitted | ||
| started --> paused: registered_pausing_signal_emitted | ||
| """ |
There was a problem hiding this comment.
Since this is the first time we are introducing this kind of architecture (QStateMachine) into the client, maybe we can include a bit more explanation here before the Mermaid diagram to outline the rationale for our future selves.
As well, in terms of polling, does it make sense to include this code now if we're not yet using polling? AIUI polling here will be a benefit/add-on that will only make it in once the 'wizard' workflow is working, or potentially concurrently with the wizard. Since that's a bigger change (i.e. we may not want to make an export tarball every time we send a qrexec command if we move to more frequent commands) I wonder if we should hold off on including this polling logic until we are closer to making use of it.
There was a problem hiding this comment.
(This comment is in kind of a weird place, it's meant to reference the Poller class)
There was a problem hiding this comment.
From offline conversation: I'll add a comment to explain that given there are more than two states to keep track of, with multiple possible transitions, a dedicated state machine makes the logic easier to audit.
|
|
||
|
|
||
| class Disk(QObject): | ||
| """Allows to export files to an ecrypted disk and track its availability.""" |
There was a problem hiding this comment.
It would be great to have a little more detail in this docstring, especially on specific expectations around use (eg expectation of singleton/same Disk object being returned?), especially given this comment around getDisk:
# Store disks to prevent concurrent access to the export service. See getDisk.
AIUI disk and export service are both singletons retrieved in main.py and injected into the export dialog? Maybe we can explain a little further how they're used, since with a name like Disk I would have expected something that represents a specific physically inserted USB, not a singleton object
There was a problem hiding this comment.
(And I think this question of documenting the usage of Disk (and Printer) is superseded by my later comment, about whether Disk, Printer, and Export Service are all needed, but in any case the extra comment/documentation will be helpful)
There was a problem hiding this comment.
From offline conversation: I'll add a note stating that the expected use of the Disk is getting an instance from getDisk.
| self.status = status | ||
|
|
||
|
|
||
| class CLI: |
There was a problem hiding this comment.
[Nit] Is this CLI, or more of a wrapper for Qrexec? (Same question for the filename)
There was a problem hiding this comment.
From offline conversation: CLI may imply parsing of options. Let's rename this to Qrexec.
| Status = NewType("Status", str) | ||
| StatusUnknown = Status("unknown-isw32") | ||
| StatusLUKSEncrypted = Status("luks-encrypted-8563d") | ||
| StatusUnreachable = Status("unreachable-ofbu4") |
There was a problem hiding this comment.
Is there something I'm missing about these status strings and their suffixes (ofbu4, 8563d)?
Also, similar to a comment I made elsewhere, do we want StatusLUKSEncrypted, or do we want to abstract away the encryption type (is this basically a "valid" device? A valid writable device?)?
There was a problem hiding this comment.
Is there something I'm missing about these status strings and their suffixes (
ofbu4,8563d)?
The suffixes are there to ensure we only compare them as enum constants, not by their string value which is meant to be an opaque string. In other words: if you ever find yourself wondering what the prefix was, you're doing something wrong and should use the constant instead (e.g. status == Disk.StatusUnreachable, not status == "unreachable").
I do that as standard practice whenever values are cast automatically instead of raising errors if comparisons are made that involve mismatched types. (e.g. comparing status: Disk.Status with "unreachable": str)
gonzalo-bulnes
force-pushed
the
refactor-export-service-to-accomodate-polling-printer-and-disk
branch
from
1cbf493
to
6d08613
Compare
Description
Towards #1435, #590, #910, #988, #990, #1605, #1021
Potentially towards #1349, #1088
@cfm The preliminary refactoring in #1584 was already voluminous, and keeping the legacy interfaces was making the API large enough that I think it is worth pushing it until the legacy interfaces can be removed, as a refactoring, and from there add the "Print conversation" action, dialogs etc. A very large fraction of the
export.Printercode in this PR comes directly from #1584.Scope
This PR doesn't intend to address the issues it works towards, only enable future PRs to fix them (e.g. #1584).
Test Plan