Add type hints and refactor most *Common methods to be static or functional #166

gmarmstrong · 2022-06-05T01:25:19Z

This draft PR depends on a work-in-progress PR.

Builds upon commits from #164. This branch begins at 616ba55. See this diff for comparison.

Does some hefty refactoring:

First adds type hinting (verified by static type checking with the help of mypy).
Then refactors most methods of GlobalCommon and GuiCommon to be static.
Those static methods are then moved to a dangerzone.util module (alias "dzutil").
Ultimately, this allows classes in dangerzone.gui.main_window to avoid passing around objects except when necessary (i.e., in cases where window-specific settings need to share a state. The discovery here is that in most cases, classes could stand alone).
After much more refactoring, GlobalCommon has been deleted. Settings are now held by GuiCommon (the CLI never uses settings.json).
GuiCommon now subclasses Common.

Fixes #122

If this design doesn't feel right to others, feel free to speak up about it! It feels more intuitive to me, but it's a big change.

PySide2 5.15.2.1 added support for Python 3.10

PySide6 prebuilt binaries have support a wider range of CPU architectures (namely Apple Silicon/M1).

ConvertThread.finished conflicted with QThread.finished

With Qt 6, subclassing QApplication no longer seems to segfault

Was indicative of something that needs to be refactored, but this change resolves the ImportError

Object needed to be bound to some variable

Application moved to application.py to avoid circular imports

Fixes freedomofpress#122

deeplow

Taking a look a this PR it seems it joins multiple things that should individual PRs, which make it quite painful to review. There are many improvements that do make sense, but I think the architecture restructure needs more thinking. So my suggestion is making individual PRs on stuff that is not related to this restructuring first. I'll try to break down the things so it's easier to see what I mean:

🟣 type hints 4978ab7
🟣 general code cleanup (linting)
🟣 code cleanup & logic improvement:
- bbce13d (done)
- 7eb1023
- 8eefbef
- 2f48e8a
- ce95dff
- 4e92a22
🟣 removing relative imports
🔴 settings.py -> gui/settings.py (for reasons explained bellow)

Legend:

🟣 suggest making individual PRs, based on main (as [WIP] Upgrade to Qt 6 #164 is blocked)
🔴 suggest dropping (see explanations bellow)

What's not in the list is what is actually the content of this PR. So in general my suggestion is to make individual PRs about the above topics and then the real contents of this PR and suggested code architecture change will become much clearer.

I think overall the proposal makes sense, but I'm thinking the code may need a restructuring to accommodate future requirements. So I'd hold off for now on this PR until that restructuring takes place (if it does).

Settings

One of the changes was moving settings.py into the GUI. While it's presently not being used at all by the GUI component, it the future, it would make sense for for the CLI or any other front-end to inherit it. So I'd keep it where it is.

Comment on common methods

Again, this is quite a big one and I'm still trying to make sense of all the changes proposed.

While trying to take a step back and looking at the goal, it seems to make sense. In the current app's design, common is like a context shared among a significant portion of components. Segregating this into common and gui_common would make sense (as was done), following this logic.

But it almost seems like "common" has lost purpose in the original code base. To the point where now that the "GUI" common doesn't actually serve to keep some sort of shared state, because all of it can be static or functional. This PR addresses this but I think if we're going in this direction, we should go full on. What I mean by this is that unless there is some shared state that needs to be kept, we should remove completely all the "common" and instead move all of its functionality into the new util.py. What little is left, I'd argue belongs in settings. For example the input file and output file is a transient setting, but still a setting nonetheless.

deeplow · 2022-07-14T13:22:28Z

dangerzone/util.py

+}
+
+
+def display_banner():


It seems like this method is exclusive for the cli. I suggest moving it to cli.py.

deeplow · 2022-07-14T13:27:29Z

dangerzone/util.py

+WINDOW_ICON_FILENAME = "dangerzone.ico" if SYSTEM == "Windows" else "icon.png"
+WINDOW_ICON_PATH = get_resource_path(WINDOW_ICON_FILENAME)


Both of these are GUI-specific code and should be instead in their respective GUI files.

deeplow · 2022-07-14T13:35:59Z

dangerzone/cli.py

-from .container import convert
+from dangerzone import container
+from dangerzone.common import Common
+import dangerzone.util as dzutil


Importing directly to make the code easier to read. This is also done in the securedrop-client.

Suggested change

import dangerzone.util as dzutil

from dangerzone.util import OCR_LANGUAGES

This refactor also needs to be propagated.

deeplow · 2022-07-15T13:14:36Z

@gmarmstrong if you're short on time, I don't mind opening these PRs. Just let me know if that's the case.

gmarmstrong · 2022-07-15T13:17:36Z

Thanks a bunch for taking the time to sort these out! My apologies for the disorder. I am pretty booked right now, feel free to make those PRs if you'd like.

deeplow · 2022-09-15T09:31:02Z

I'm re-implementing many of these changes in a new branch. I had to resort to branching off main as this has diverged too much.

@gmarmstrong

static methods that are used application-wide should belong to the utilities python file. inspired by @gmarmstrong's PR #166 on refactoring global_common methods to be static and have a dzutil.py

gmarmstrong · 2022-09-15T11:34:41Z

Take whatever you'd like, thanks again for trudging through it. Feel free to close this PR whenever.

deeplow · 2022-09-15T11:39:18Z

Take whatever you'd like, thanks again for trudging through it. Feel free to close this PR whenever.

Thanks! I'm going to open the PR in some minutes. Would you like to give feedback on it? Or shall I just go ahead and merge it?

gmarmstrong · 2022-09-15T11:40:01Z

Take whatever you'd like, thanks again for trudging through it. Feel free to close this PR whenever.

Thanks! I'm going to open the PR in some minutes. Would you like to give feedback on it? Or shall I just go ahead and merge it?

Happy to take a look! :)

@gmarmstrong

Reduce "global_common" coupling by moving methods that could be static onto "semantically-closer" py files. Based on work initially made by @gmarmstrong on PR #166: - moves container-specific code out of global_common.py and into container.py - creates a util.py for static methods used through the whole app - move banner code from global_common onto cli.py given that it's only displayed there - updates tests to reflect these changes - move ocr_languages from global_common onto its own json file in share/ocr-languages.json to simplify global_common logic

eloquence · 2022-09-15T17:20:21Z

Superseded by #203.

gmarmstrong added 27 commits May 28, 2022 14:28

Support Python 3.10

c088951

PySide2 5.15.2.1 added support for Python 3.10

Upgrade from PySide2 (Qt 5) to PySide6 (Qt 6)

851124e

PySide6 prebuilt binaries have support a wider range of CPU architectures (namely Apple Silicon/M1).

Fix TypeError in ConvertThread.run

bd33f16

ConvertThread.finished conflicted with QThread.finished

Unwrap QApplication

fc8d2af

With Qt 6, subclassing QApplication no longer seems to segfault

Fix circular import

16dfa47

Was indicative of something that needs to be refactored, but this change resolves the ImportError

Fix missing SysTray

bdc08d7

Object needed to be bound to some variable

Add more type annotations

616ba55

Application moved to application.py to avoid circular imports

Refactor GlobalCommon methods to be static

3006537

Access methods statically in dangerzone.gui.common

42a1f01

Refactor static methods as functions in util module

17ff3a8

Fix typo in CONTAINER_COMMAND value

815c2ef

Add typing extensions to developer dependencies for mypy support

4978ab7

Make GlobalCommon completely static except for __init__

2da6ce0

Use pathlib internally in dzutil

bbce13d

Inline container.exec into container.exec_container

7eb1023

Remove unused dangerzone.gui.common.Alert

2f48e8a

Remove all instantiations of GlobalCommon, move Settings to GuiCommon

519b8fd

Remove GlobalCommon, convert its methods (all static) to functions

ca5d175

Delete global_common.py, move its functions to dzutil

ba51647

Run black dangerzone to fix up style

8eefbef

Undo accidental tweak of platform_args

4fd233e

Convert relative imports to absolute

52e5154

Have GuiCommon subclass Common

c655d3a

Move is_container_installed and install_container to container module

1042c51

Expand mypy compliance, ignore unsupported packages

801d98f

Don't hardcode "Docker" in help text

ce95dff

Fixes freedomofpress#122

Use xdg.BaseDirectory in _find_pdf_viewers

4e92a22

gmarmstrong mentioned this pull request Jun 9, 2022

Add unit tests #168

Closed

gmarmstrong changed the title ~~[WIP] Add type hints and refactor most *Common methods to be static or functional~~ Add type hints and refactor most *Common methods to be static or functional Jun 14, 2022

deeplow reviewed Jul 15, 2022

View reviewed changes

gmarmstrong mentioned this pull request Aug 16, 2022

[WIP] Upgrade to Qt 6 #164

Closed

3 tasks

deeplow mentioned this pull request Sep 15, 2022

Reduce "global_common" coupling by moving methods that could be static onto "semantically-closer" py files #203

Merged

eloquence closed this Sep 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add type hints and refactor most *Common methods to be static or functional #166

Add type hints and refactor most *Common methods to be static or functional #166

gmarmstrong commented Jun 5, 2022 •

edited

Loading

deeplow left a comment •

edited

Loading

deeplow Jul 14, 2022

deeplow Jul 14, 2022

deeplow Jul 14, 2022

deeplow commented Jul 15, 2022

gmarmstrong commented Jul 15, 2022

deeplow commented Sep 15, 2022

gmarmstrong commented Sep 15, 2022

deeplow commented Sep 15, 2022

gmarmstrong commented Sep 15, 2022

eloquence commented Sep 15, 2022

		WINDOW_ICON_FILENAME = "dangerzone.ico" if SYSTEM == "Windows" else "icon.png"
		WINDOW_ICON_PATH = get_resource_path(WINDOW_ICON_FILENAME)

	import dangerzone.util as dzutil
	from dangerzone.util import OCR_LANGUAGES

		}


		def display_banner():

Add type hints and refactor most *Common methods to be static or functional #166

Add type hints and refactor most *Common methods to be static or functional #166

Conversation

gmarmstrong commented Jun 5, 2022 • edited Loading

deeplow left a comment • edited Loading

Choose a reason for hiding this comment

Settings

Comment on common methods

deeplow Jul 14, 2022

Choose a reason for hiding this comment

deeplow Jul 14, 2022

Choose a reason for hiding this comment

deeplow Jul 14, 2022

Choose a reason for hiding this comment

deeplow commented Jul 15, 2022

gmarmstrong commented Jul 15, 2022

deeplow commented Sep 15, 2022

gmarmstrong commented Sep 15, 2022

deeplow commented Sep 15, 2022

gmarmstrong commented Sep 15, 2022

eloquence commented Sep 15, 2022

gmarmstrong commented Jun 5, 2022 •

edited

Loading

deeplow left a comment •

edited

Loading