Better "dark mode" support

[garrettr]

Fixes #528—but for all platforms, not just macOS. I believe this approach should work on all platforms that provide an OS setting for a system-wide light or dark color mode, but I have only tested this change on macOS.

I decided to go with an approach that would work on both PySide2/Qt 5 and PySide6/Qt 6. If you want, I'm happy to file follow-up issues for:

1. Switching to QGuiApplication.styleHints().colorScheme(), only supported in Qt 6.5+.
2. Handling the (likely rare in typical usage) case where the user changes the OS color setting while the Dangerzone GUI is running, which requires the colorSchemeChanged signal added in Qt 6.5.

Before this change

Light Mode

Dark Mode

Exhibits the issues described in #528.

After this change

Light Mode

Looks the same as Light Mode before this change.

Dark Mode

Fixes the issues described in #528 and adds a custom style for QLabel[style="safe_extension_filename"] in dark mode that's consistent with the preexisting custom appearance in Light Mode.

[deeplow]

Thanks a lot for the contribution. It looks pretty solid to merge. I just have one suggestion to move the code to somewhere where it's easier to test the application logic.

I was also playing around a bit with the PySide6 QtGui.QStyleHints.colorScheme() and it's not very reliable. So I think it's best to go with the strategy you suggested, even in systems where PySide6 is available.

In my case (linux) it was unable to detect de color scheme and instead of outputting `Qt.ColorScheme.Unknown` it instead returned some random color scheme number:

>>> QtGui.QStyleHints().colorScheme()                                                                   
<ColorScheme.1634038370: 1634038370>
>>> QtCore.Qt.ColorScheme.Unknown
<ColorScheme.Unknown: 0>                                                                                
>>> QtCore.Qt.ColorScheme.Dark
<ColorScheme.Dark: 2>
>>> QtCore.Qt.ColorScheme.Light
<ColorScheme.Light: 1>
>>> QtGui.QStyleHints().colorScheme() == QtCore.Qt.ColorScheme.Unknown
False

[deeplow]

Perfect. And now almost everything in CI is passing. The only thing missing is a type error but it can be easily solved with something like this:

diff --git a/dangerzone/gui/logic.py b/dangerzone/gui/logic.py
index 3832c568..74d7d1e6 100644
--- a/dangerzone/gui/logic.py
+++ b/dangerzone/gui/logic.py
@@ -12,6 +12,8 @@ from colorama import Fore
 # FIXME: See https://github.com/freedomofpress/dangerzone/issues/320 for more details.
 if typing.TYPE_CHECKING:
     from PySide2 import QtCore, QtGui, QtWidgets
+
+    from . import Application
 else:
     try:
         from PySide6 import QtCore, QtGui, QtWidgets
@@ -35,7 +37,7 @@ class DangerzoneGui(DangerzoneCore):
     """
 
     def __init__(
-        self, app: QtWidgets.QApplication, isolation_provider: IsolationProvider
+        self, app: "Application", isolation_provider: IsolationProvider
     ) -> None:
         super().__init__(isolation_provider)

[deeplow]

@garrettr I can fix the lint when merging. Do you still want to do something else or can I merge it?

[garrettr]

@garrettr I can fix the lint when merging. Do you still want to do something else or can I merge it?

I'll fix the lint!

Also, I noticed a minor issue with poetry run make lint when I tried to run it locally. In my local dev environment I'm using a virtualenv stored in dangerzone/.venv, a common configuration. When I ran poetry run make lint the lint-isort step failed with numerous errors due to unsorted imports in the source files of dependencies stored in .venv—see the output of the command here: poetry_run_make_lint.txt.

I would expect isort to ignore virtualenvs by default, and indeed it does, but dangerzone's pyproject.toml has a [tool.isort] section that sets skip, so venvs are back on the menu:

[tool.isort]
profile = "black"
skip = [".gitignore"]

I can fix the problem by simply adding .venv to skip in pyproject.toml. @deeplow Is that something that would be appropriate to tack on to this change, or would you prefer I create a separate issue/PR for that?

[apyrgio]

Damn, I didn't know that --skip had a default, which we essentially overrode. Thanks a lot for pointing that out!

To me, it seems the proper way to solve this is to convert skip to extend_skip. Does this work in your environment? If yes, feel free to send an extra commit, and we can merge it along this PR.

[garrettr]

To me, it seems the proper way to solve this is to convert skip to extend_skip. Does this work in your environment? If yes, feel free to send an extra commit, and we can merge it along this PR.

Nice! I didn't see that option while I was skimming the isort docs. I agree that's the best solution. I confirmed it works for me locally and added it as an extra commit to this PR.

[garrettr]

@deeplow This is ready for re-review! Now that we I've fixed the few issues I think it's ready to merge.

[deeplow]

I'm afraid I messed up the rebase and force pushed to your remote branch without first having fetched the new commits. But I've managed to recover what I could. Sorry about that (Still getting the hang of merging forked dirs). If it's all good, I'll merge. Otherwise, feel free to force push from your local branch @garrettr to override my mistake.

[apyrgio]

... you nerd-sniped me @deeplow 🤓

Ok, this is something that @garrettr can easily solve by force pushing his branch, or with a git reflog command, if he has already pulled. Same goes to you @deeplow, you can easily recover @garrettr's branch via git reflog.

But what about other people who have never pulled @garrettr's repo? If you don't have access (in the fs-level) to the Git repository, this is not achievable. On GitHub though...

Git(Hub) Plumbing Galore 🔧

Interestingly, GitHub displays this event:

@deeplow force-pushed the feature/528-better-dark-mode-macos-qt5 branch from 050c86ato b48843b

This tells us that 050c86a is the latest correct commit. If we click on the URL, we can see its contents 🎉. This means that GitHub does not run git gc immediately. This also means that if you accidentally push something that is sensitive and decide to force push in order to erase it, you need to also contact GitHub to run gc on your repo 😱 . Moving on...

We can also click on the parent commit of the leaf commit, iteratively, until we reach the root of the PR. This means that we can re-construct the branch from the commits we see in the GitHub UI.

Re-creating the commits by hand though is tedious. Thankfully, GitHub has another trick upon its sleeve. If you append .diff to the URL of the commit, you can download a patch file. We can iteratively run git apply with these patch files, and recreate the original branch.

I have actually done so, and here's the diff that I see with the branch that @deeplow restored:

diff --git a/dangerzone/gui/__init__.py b/dangerzone/gui/__init__.py
index f65145b..655b13b 100644
--- a/dangerzone/gui/__init__.py
+++ b/dangerzone/gui/__init__.py
@@ -57,8 +57,6 @@ class Application(QtWidgets.QApplication):
             style = f.read()
         self.setStyleSheet(style)
         self.original_event = self.event
-        self.os_color_mode = self.infer_os_color_mode()
-        log.debug(f"Inferred {self.os_color_mode}")
 
         def monkeypatch_event(arg__1: QtCore.QEvent) -> bool:
             event = arg__1  # oddly Qt calls internally event by "arg__1"
@@ -76,6 +74,9 @@ class Application(QtWidgets.QApplication):
 
         self.event = monkeypatch_event  # type: ignore [method-assign]
 
+        self.os_color_mode = self.infer_os_color_mode()
+        log.debug(f"Inferred system color scheme as {self.os_color_mode}")
+
     def infer_os_color_mode(self) -> OSColorMode:
         """
         Qt 6.5+ explicitly provides the OS color scheme via QStyleHints.colorScheme(),
diff --git a/dangerzone/gui/logic.py b/dangerzone/gui/logic.py
index 5ea9969..74d7d1e 100644
--- a/dangerzone/gui/logic.py
+++ b/dangerzone/gui/logic.py
@@ -1,4 +1,3 @@
-import enum
 import logging
 import os
 import platform

The differences are minor but we should incorporate them regardless.

---

Now that I have the original branch, and we have the OK to merge, I'll squash some commits and merge them.

---

EDIT: I just saw that the GitHub UI offers the option to generate a Git patch by appending, with proper timestamps and author names. Instead of .diff, you can use .patch. If I had used it, I would have been able to fully recreate the original PR, up to its hash. Oh well 🤷

[deeplow]

Nice! Thanks for the git rescue ⛑️. I was aware of the .patch trick, but I didn't think to look too much into github.

[garrettr]

Sorry I didn't respond earlier to help with git! Glad to see you got it sorted. Thanks for merging! It was a pleasure working with both of you 😊.

[deeplow]

No worries and likewise! 🙂