rename "common" to "document" & simplify "new window" window logic #208

deeplow · 2022-09-19T10:18:55Z

Simplify code logic for creating and handling documents through the renaming of Common class to DocumentHolder and associated simplifications.

When we support bulk document conversion we'll need to keep track of each document's state. This refactor opens way for that. A document holder does not represent a document in itself (e.g. can be instantiated without a document) but stores the input name, output name, and in the future possibly the conversion progress.

Clarification renames: some of the names were changed to better reflect the classes' or files' purpose.

renames: common.py::Common -> document.py::DocumentHandler
renames: global_common.py::GlobalCommon ->logic.py::DangerzoneCore
renames: gui/common.py::GuiCommon -> gui/common.py::DangerzoneGui

Associated logic simplication:

deduplicates input / output filename validation now done in DocumentHandler, when a new filename is added, throwing an DocumentFilenameException if that validation fails. (code was previously duplicated in gui_main and cli_main).
simplifies "new window" creation logic (and associated DocumentHandler instantiation)
now when the users click new window, it simply opens a new window without extra validation to see if any window is already open and doesn't have a document associated to it.

And lastly, it adds unit tests for document.py (100% coverage)

deeplow · 2022-09-19T12:15:36Z

I was getting errors with the last commit when we ran the cli without a full path. This is now fixed.

$ poetry run dev_scripts/dangerzone-cli tests/test_docs/sample.pdf 
╭──────────────────────────╮
│           ▄██▄           │
│          ██████          │
│         ███▀▀▀██         │
│        ███   ████        │
│       ███   ██████       │
│      ███   ▀▀▀▀████      │
│     ███████  ▄██████     │
│    ███████ ▄█████████    │
│   ████████████████████   │
│    ▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀    │
│                          │
│    Dangerzone v0.3.2     │
│ https://dangerzone.rocks │
╰──────────────────────────╯

Converting document to safe PDF
> /usr/bin/podman run --network none -u dangerzone --security-opt no-new-privileges --userns keep-id --cap-drop all --rm -v tests/test_docs/sample.pdf:/tmp/input_file -v /home/user/.config/dangerzone/tmp/tmpzkabfaa4/pixels:/dangerzone dangerzone.rocks/dangerzone /usr/bin/python3 /usr/local/bin/dangerzone.py document-to-pixels
Invalid JSON returned from container: Error: error creating named volume "tests/test_docs/sample.pdf": error running volume create option: names must match [a-zA-Z0-9][a-zA-Z0-9_.-]*: invalid argument

documents-to-pixels failed

Failed to convert document

tests/test_document.py

apyrgio · 2022-10-04T12:17:30Z

I'm testing out this PR in order to see if #204 is resolved, and I have some quick questions:

What is the proper way to test this on Windows environments? In 0.3.2, when I click on New Window from the systray, I don't see any window being opened, nor an exception somewhere (I checked the Event Log Viewer as well). I suppose that the exception is logged somewhere, but I haven't found where yet. Any pointers would be helpful.
In Linux, I have reproduced the error, and I'm seeing that with your fix, two windows get opened. However, when you close either of them, both of them close, which is unexpected. Am I missing something here?

deeplow · 2022-10-04T14:02:35Z

What is the proper way to test this on Windows environments? In 0.3.2, when I click on New Window from the systray, I don't see any window being opened, nor an exception somewhere (I checked the Event Log Viewer as well). I suppose that the exception is logged somewhere, but I haven't found where yet. Any pointers would be helpful.

I think it would be logged on the terminal from which you launch dangerzone. But I've also had issues where exceptions are consumed somewhere along the codepath. It could be that.

2. In Linux, I have reproduced the error, and I'm seeing that with your fix, two windows get opened.

So two windows get open when only one should open? Or that part is working?

However, when you close either of them, both of them close, which is unexpected.

Yes, that is #205 which was present in linux in 0.3.1. This is fixed in the other PR, I think.

apyrgio · 2022-10-04T15:25:09Z

I think it would be logged on the terminal from which you launch dangerzone. But I've also had issues where exceptions are consumed somewhere along the codepath. It could be that.

It might be that I'm testing it out by running the .exe, not following the instructions on BUILD.md. I'll check that out.

So two windows get open when only one should open? Or that part is working?

I meant that this part is working correctly, yes.

Yes, that is #205 which was present in linux in 0.3.1. This is fixed in the other PR, I think.

Oh, I thought that #205 was affecting windows that closed due to conversion completion, not due to a user action. Ok, then we'll see this later on.

deeplow · 2022-10-04T15:28:09Z

Yes, that is #205 which was present in linux in 0.3.1. This is fixed in the other PR, I think.

Oh, I thought that #205 was affecting windows that closed due to conversion completion, not due to a user action. Ok, then we'll see this later on.

That issue happened because it would close the application as soon as the first document was converted, regardless of whether any other was in progress.

It might be that I'm testing it out by running the .exe, not following the instructions on BUILD.md. I'll check that out.

Well, I've been testing this mostly by running from the dev_scripts/dangerzone, not the .exe.

dangerzone/document.py

dangerzone/gui/__init__.py

dangerzone/document.py

dangerzone/cli.py

dangerzone/document.py

tests/test_document.py

dangerzone/logic.py

apyrgio · 2022-10-05T14:36:37Z

Also, I see the following commits:

Common -> Document                                                                             
Document -> DocumentHolder                                                                     
rename (Global)Common & related to "dangerzone"                                                
rename global_common.py -> logic.py

Might if I make their names a bit more consistent, such as "Rename X module/class to Y"?

deeplow · 2022-10-06T11:33:14Z

@apyrgio I have addressed all comments in the parallel branch simplify-common-2

apyrgio · 2022-10-06T20:38:06Z

@deeplow Thanks a lot for the changes. I'll take a look and report back.

apyrgio · 2022-10-12T00:48:39Z

I have done some changes of my own, based on the above discussion, which you can find on the simplify-common-3 branch. I have tested it on Linux and it works. I need to iron out one small detail, and then it will be good to merge.

deeplow · 2022-10-13T10:55:23Z

Whoops. I have accidentally deleted this while trying to force push and I see no way to reopen this via the UI.

deeplow · 2022-10-26T10:28:34Z

Rebased w/ newly included windows tests
added type hints to test_documents.py (because tests now have type hinting requirements)
fixed args.py and errors.py type hints

@apyrgio I think the last thing missing is you changing your author email, if you so desire.

deeplow · 2022-10-26T10:37:52Z

Interesting. Mypy is throwing some errors but it doesn't throw them on my system.

dangerzone/errors.py:4: error: Module "typing" has no attribute "ParamSpec"; maybe "_ParamSpec"?  [attr-defined]
dangerzone/errors.py:18: error: The first argument to Callable must be a list of types, parameter specification, or "..."  [misc]
dangerzone/errors.py:18: note: See https://mypy.readthedocs.io/en/stable/kinds_of_types.html#callable-types-and-lambdas
dangerzone/errors.py:22: error: Name "P.args" is not defined  [name-defined]
dangerzone/errors.py:22: error: Name "P.kwargs" is not defined  [name-defined]
```

If we take a look at the docs, it is well documented there: https://mypy.readthedocs.io/en/stable/generics.html?highlight=ParamSpec#declaring-decorators

deeplow · 2022-10-26T11:03:09Z

It looks like this happening because some of CI doesn't have python3.10 version running. We might have moved too soon with #161.

deeplow · 2022-10-26T19:58:38Z

Rebased and ready to be merged. Can I merge it @apyrgio?

apyrgio · 2022-10-27T08:21:34Z

Gimme a bit to take a last look, and running the unit tests on Windows. Also, we need to squash "FIXUP" commits, since they pollute the history.

deeplow · 2022-10-27T09:09:27Z

Also, we need to squash "FIXUP" commits, since they pollute the history.

Done

Rename the `common` module and `common.Common` class to `document` and `document.Document` respectively. Also, rename the variables that hold instances of this class. This change reflects the fact that the class is responsible for tracking the state of the document. When we add bulk document conversion, allowing us to keep track of a document's state will be key. This name change is a step towards that.

apyrgio · 2022-10-27T11:44:56Z

I'm doing some final changes (proper author attribution to some of the commits), and I'm testing on a Windows machine. I'll ping you when you can rebase, sign, and merge.

Avoid setting document's filename via document.filename and instead do it via object instantiation where possible. Incidentally this has to change some window logic. When select_document() is called it no longer checks if there is already an open window with no document selected yet. The user can open as many windows with unselected documents as they want.

Rename select_document() to new_window() to better encapsulate the fact that this function is opening a new Dangerzone window.

Factor out the filename validation logic and move it into the Document class. Previously, the filename validation logic was scattered across the CLI and GUI code. Also, introduce a new errors.py module whose purpose is to handle document-related errors, by providing: * A special exception for them (DocumentFilenameExcpetion) * A decorator that handles DocumentFilenameException, logs it and the underlying cause, and exits the program gracefully.

Implement Click's callback interface and create validators for the input/output filenames, using the logic from the Document class. This way, we can catch user errors as early as possible.

tests/test_document.py

Make the Document class always resolve relative input/output file paths, which are usually passed as arguments by users. Previously, resolving relative filepaths was a job left to the instantiators of the Document class. This was error-prone since this conversion must happen in all the places where we instantiated the Document class.

Let the Document class suggest the default filename for the safe PDF, based on the provided input filename, appended with the extension `-safe.pdf`. Previously, this logic was copy-pasted throughout the code, which made it difficult to maintain.

Rename the `global_common` module and `global_common.GlobalCommon` class to `logic` and `logic.Dangerzone` respectively. Also rename variables that hold instances of this class. This change is part of the initial refactor to make the Dangerzone class handle the core logic of the Dangerzone project.

Rename the `gui.common` module and `gui.common.GuiCommon` class to `gui.logic` and `gui.logic.DangerzoneGui` respectively. We keep as is the original names of the variables that hold instances of this class, since they will change in subsequent commits. This change is part of the initial refactor to make the DangerzoneGui class handle the GUI logic of the Dangerzone project.

Simplify state sharing by having all dangerzone core logic in one single class instead of two.

apyrgio · 2022-10-27T12:28:08Z

I've updated the branch with the following changes:

I properly attributed some commits to you. I had split some commits while reviewing and the author became myself, but I changed that back.
I fixed a Windows unit test error that I had actually fixed before on simplify-common-4. We probably didn't get the latest changes before force-pushing to simplify-common. That's ok, but in general it's good to have this in mind.
I checked that the Windows unit tests do pass. The only (minor) issue that we have is that test_lang_eng fails, because Tesseract times out. Since this test works on Linux, and Tesseract runs on a container, I'll attribute it to our timeouts configuration and my slow Windows VM.

deeplow force-pushed the simplify-common branch from 6c615a2 to c20021b Compare September 19, 2022 12:13

deeplow force-pushed the simplify-common branch 4 times, most recently from a9afa8d to 38d65c6 Compare September 21, 2022 10:50

deeplow mentioned this pull request Sep 27, 2022

Basic via-CLI mutli-document support #216

Merged

deeplow commented Sep 28, 2022

View reviewed changes

tests/test_document.py Outdated Show resolved Hide resolved

apyrgio self-requested a review September 30, 2022 13:03

apyrgio requested changes Oct 5, 2022

View reviewed changes

deeplow closed this Oct 13, 2022

deeplow deleted the simplify-common branch October 13, 2022 10:52

deeplow restored the simplify-common branch October 13, 2022 10:52

deeplow reopened this Oct 13, 2022

deeplow force-pushed the simplify-common branch from 38d65c6 to de19114 Compare October 13, 2022 11:06

deeplow force-pushed the simplify-common branch 2 times, most recently from 34b6031 to 7f583c6 Compare October 26, 2022 10:23

deeplow force-pushed the simplify-common branch from 7f583c6 to 51205d4 Compare October 26, 2022 18:06

deeplow force-pushed the simplify-common branch from 51205d4 to f8f01d3 Compare October 27, 2022 09:09

deeplow and others added 4 commits October 27, 2022 14:46

Rename select_document() function to new_window()

5110f6b

Rename select_document() to new_window() to better encapsulate the fact that this function is opening a new Dangerzone window.

Validate filename arguments through Click

4f4e716

Implement Click's callback interface and create validators for the input/output filenames, using the logic from the Document class. This way, we can catch user errors as early as possible.

apyrgio force-pushed the simplify-common branch 2 times, most recently from 0fbb274 to e2c9a9c Compare October 27, 2022 12:15

deeplow commented Oct 27, 2022

View reviewed changes

tests/test_document.py Outdated Show resolved Hide resolved

deeplow and others added 7 commits October 27, 2022 15:20

Add unit tests for document.py

3fe45fa

Make DangerzoneGui a subclass of DangerzoneCore

c2eeeb1

Simplify state sharing by having all dangerzone core logic in one single class instead of two.

Fix a minor typo

3376b57

apyrgio force-pushed the simplify-common branch from e2c9a9c to 3376b57 Compare October 27, 2022 12:20

apyrgio approved these changes Oct 27, 2022

View reviewed changes

deeplow merged commit 3376b57 into main Oct 27, 2022

eloquence mentioned this pull request Nov 9, 2022

Refactor application to acommodate bulk doc conversion #209

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rename "common" to "document" & simplify "new window" window logic #208

rename "common" to "document" & simplify "new window" window logic #208

deeplow commented Sep 19, 2022 •

edited

Loading

deeplow commented Sep 19, 2022

apyrgio commented Oct 4, 2022

deeplow commented Oct 4, 2022 •

edited

Loading

apyrgio commented Oct 4, 2022

deeplow commented Oct 4, 2022

apyrgio commented Oct 5, 2022

deeplow commented Oct 6, 2022

apyrgio commented Oct 6, 2022

apyrgio commented Oct 12, 2022

deeplow commented Oct 13, 2022 •

edited

Loading

deeplow commented Oct 26, 2022

deeplow commented Oct 26, 2022

deeplow commented Oct 26, 2022

deeplow commented Oct 26, 2022

apyrgio commented Oct 27, 2022

deeplow commented Oct 27, 2022

apyrgio commented Oct 27, 2022

apyrgio commented Oct 27, 2022

rename "common" to "document" & simplify "new window" window logic #208

rename "common" to "document" & simplify "new window" window logic #208

Conversation

deeplow commented Sep 19, 2022 • edited Loading

deeplow commented Sep 19, 2022

apyrgio commented Oct 4, 2022

deeplow commented Oct 4, 2022 • edited Loading

apyrgio commented Oct 4, 2022

deeplow commented Oct 4, 2022

apyrgio commented Oct 5, 2022

deeplow commented Oct 6, 2022

apyrgio commented Oct 6, 2022

apyrgio commented Oct 12, 2022

deeplow commented Oct 13, 2022 • edited Loading

deeplow commented Oct 26, 2022

deeplow commented Oct 26, 2022

deeplow commented Oct 26, 2022

deeplow commented Oct 26, 2022

apyrgio commented Oct 27, 2022

deeplow commented Oct 27, 2022

apyrgio commented Oct 27, 2022

apyrgio commented Oct 27, 2022

deeplow commented Sep 19, 2022 •

edited

Loading

deeplow commented Oct 4, 2022 •

edited

Loading

deeplow commented Oct 13, 2022 •

edited

Loading