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

Revamp Preferences, implement Minimalist Mode and Qt widget gallery to test GUI changes #2289

Merged
merged 70 commits into from
Jan 18, 2023

Conversation

kleinerpirat
Copy link
Contributor

This adds a gallery similar to this one to the debug dialog.

image

image

It allows users to change the styling approach across the application via a dropdown and see the effects immediately on various Qt widgets.

There are three choices:

  • Anki (new code path with custom stylesheets)
  • Fusion (code path of 2.1.54 and below)
  • Native (same code path as current macOS Qt6)

If the "Force Style" box is checked, the style will be kept across profiles.


@tatsumoto-ren I don't know how much of the GTK theme Anki should pick up. Please tell me if the "Native/Fusion" options are working for you. The UI looks the same as on 2.1.49 for me. Missing styles for the "Anki" option will be added in future PRs.

@tatsumoto-ren
Copy link
Contributor

Your branch doesn't build for me. The dialog looks handy though.

./run

Finished dev [unoptimized + debuginfo] target(s) in 0.19s
[1/21; 1 active; 1.604s] ftl:repo:core
FAILED: /home/ren/Downloads/anki_git/out/git/ftl/core-repo
/home/ren/Downloads/anki_git/out/rust/debug/runner run --stamp=/home/ren/Downloads/anki_git/out/git/ftl/core-repo git submodule update --init ftl/core-repo
Command failed:

fatal: transport 'file' not allowed
fatal: Fetched in submodule path 'ftl/core-repo', but it did not contain 2d13bfbe9e60dd365c4300f1e0589100b8b044af. Direct fetching of that commit failed.

ninja: build stopped: subcommand failed.
Finished dev [unoptimized + debuginfo] target(s) in 0.17s
Running `out/rust/debug/configure`
[1/21; 1 active; 0.583s] ftl:repo:core
FAILED: /home/ren/Downloads/anki_git/out/git/ftl/core-repo
/home/ren/Downloads/anki_git/out/rust/debug/runner run --stamp=/home/ren/Downloads/anki_git/out/git/ftl/core-repo git submodule update --init ftl/core-repo
Command failed:

fatal: transport 'file' not allowed
fatal: Fetched in submodule path 'ftl/core-repo', but it did not contain 2d13bfbe9e60dd365c4300f1e0589100b8b044af. Direct fetching of that commit failed.

ninja: build stopped: subcommand failed.

Build failed.

When it comes to changing the theme, my idea is that the selector in "Tools" > "Preferences" could allow the user to choose between not only "system", "dark" and "light", but also other presets. Also, system currently doesn't do anything so I'd like to fix that.

@dae
Copy link
Member

dae commented Dec 30, 2022

@tatsumoto-ren Matthias has not changed anything that would cause that error. It seems the error is caused by a recent change to git's behaviour. From the repo root, if you run git submodule update --init ftl/core-repo, do you get the same error? What about if you run git -c protocol.file.allow=always submodule update --init ftl/core-repo?

@dae
Copy link
Member

dae commented Dec 30, 2022

@kleinerpirat I get an error on startup, and had to run the following first:

>>> mw.pm.meta["force_anki_styles"] = False

<no output>

>>> mw.pm.meta["force_fusion_styles"] = False

<no output>

>>> mw.pm.meta["force_native_styles"] = False

<no output>
    if self.meta[f"force_{AnkiStyles(member).name.lower()}_styles"]:
KeyError: 'force_native_styles'

If those three options are mutually exclusive, I think we should probably be storing them in a single key instead of three separate booleans, eg meta.get("widget_style") would return "anki" | "fusion" | "native" or 0 | 1 | 2.

Rest of the PR looks good; I can see how this would be helpful for debugging styling issues moving forward.

@kleinerpirat
Copy link
Contributor Author

Yeah, I think a single key will be the more elegant implementation, although it's less intuitive to the user.

@tatsumoto-ren
Copy link
Contributor

@dae thanks for the tip. After running the second command, i was able to build successfully.

The KeyError: 'force_anki_styles' also affected me, but I fixed that too. @kleinerpirat, maybe if you use dict.setdefault, you could prevent the errors.

For testing it would be handy if the table widget had a few rows with some text in them. Earlier I ran into a problem with text being unreadable when using a custom table widget in 2.1.55.

Since Anki doesn't pick up my GTK theme currently, I don't see much difference between "fusion" and "native".

When testing custom Qtwidgets, I'd really like this menu to appear globally under "tools" > "preferences" too. I'm sure addon devs would agree.
menu

@dae
Copy link
Member

dae commented Jan 3, 2023

The choices change based on platform. On Mac, "Anki" is mostly native widgets, with "Fusion" being the less-native styling that was used in dark mode in old releases, and I presume we don't need a separate native option. System Qt themes are not a concern on Windows either, so presumably we'd only need 2 options - or even just 1? The push for access to the old fusion theme seems to have come from macOS users IIRC, so perhaps we don't even need a setting on Windows?

@kleinerpirat would it make sense to expose this as a dropdown in the prefs screen, with 2 options on Mac, 3 on Linux, and either hide it on Windows, or have 2 options? That removes any confusion about manually changing enum settings. I'd originally hoped to keep the macOS fusion option hidden in the debug console, but AnKing went and promoted it to his followers, so any hope of keeping that obscure rapidly went out the door :-)

@dae dae added this to the 2.1.57 milestone Jan 9, 2023
@kleinerpirat
Copy link
Contributor Author

kleinerpirat commented Jan 11, 2023

A little update on what I'm working on:

image

The Preferences screen will be revamped a bit. I intend to make more use of QGroupWidget to bring more order into the tabs.

Minimalist mode will add a body class to AnkiWebView. Properties like large border radii, glass effects or box-shadows of containers are disabled in that mode.

@dae
Copy link
Member

dae commented Jan 11, 2023

Thanks for working on this. I think we'll probably want to include the UI language there too, and move that page to the first tab.

@dae dae mentioned this pull request Jan 11, 2023
3 tasks
and make former large radius a bit smaller.
Also:
- create additional and missing widget styles and tweak existing ones
- use single profile entry to set widget styles and reduce choices to Anki and Native
@kleinerpirat
Copy link
Contributor Author

kleinerpirat commented Jan 12, 2023

Update

Styles

  • reduced styling choices:
    • Anki: Fusion + Palette + Stylesheets, default on Linux and Windows
    • Native: only Palette, default on macOS
  • added custom styling for QSlider and QGroupBox, prefixed a few missing selectors to existing rules, which I also tweaked a bit
  • simplified the stylesheet imports in theme.py
  • added border-radius-medium prop (12px) and used it for containers where border-radius-large (15px) had been previously used

Preferences

The preferences are reordered quite a bit now.

  • added Appearance tab, which contains settings related to the UI
  • used QGroupBox to categorize related settings
  • added a dropdown to select the widget style (Anki, Native)
  • added a minimalist mode checkbox
  • made settings involving class changes to webview bodies respond immediately

Minimalist Mode

This mode removes the body class fancy from Anki's webviews. Good-looking, but unnecessary CSS can now be defined like this:

.sample-element {
  // common definitions
  padding: 1rem;
  font-weight: bold;

  .fancy & {
    // extra definitions omitted in minimalist mode
    border-radius: var(--border-radius-medium);
    filter: drop-shadow(0 0 0.75rem var(--border-focus));
  }
}



etc.

The mode also unsets box-shadow and backdrop-filter definitions globally with !important.

qt/aqt/main.py Outdated
@@ -1068,9 +1068,9 @@ def setupStyle(self) -> None:
if is_lin:
# On Linux, the check requires invoking an external binary,
# which we don't want to be doing frequently
interval_secs = 300
interval_secs = 10
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

300s seemed overkill. I don't know how much of a performance impact invoking the external binary has, but I didn't notice anything when I set it to 2 on my Gnome system. 5 or 10 would also suffice to achieve a relatively smooth transition between system themes.

Automatic theme adjustments are a very nice thing to have, especially if you switch themes regularly via shortcut or with a sunset timer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I was probably a little too conservative there :-) They appear to take a few ms at most, so once every 5-10s isn't going to contribute much to global warming/exhaust user's batteries.

@kleinerpirat kleinerpirat changed the title Add Qt widget gallery for testing design changes and forcing styles via GUI Revamp Preferences, implement Minimalist Mode and Qt widget gallery to test GUI changes Jan 12, 2023
@kleinerpirat
Copy link
Contributor Author

Unfortunately the Preferences screen crashes when using Shift+Tab to focus the previous widget. This does not happen in the widget gallery, so I'm positive it can be resolved with some testing.

@dae
Copy link
Member

dae commented Jan 13, 2023

It looks like the shift+tab issue exists in the main branch as well, so that doesn't seem to be your fault. Wierd!

The preferences screen looks cleaner - those groups do help with things. I do find it a bit strange to have a Reviewer section in Appearance and then a separate Review section in Basic. One idea would be to move the review and scheduler things into a single Review tab, and rename Basic to Editing, with browsing and import/export covered under that tab. WDYT?

Regarding the minimal mode, I don't have personal preferences here, so we'll need to see how people react to it. Some possible objections they might raise are that buttons don't have outlines anymore, and that there's no border at the top. @tatsumoto-ren any feedback at this stage?

@dae
Copy link
Member

dae commented Jan 18, 2023

I've pushed a fix for the conflicts; will try to return to this later today.

@dae
Copy link
Member

dae commented Jan 18, 2023

Sorry, I need to apologize about the scrollbar comments. I was in a rush when testing, and having looked at it more closely now, the same issue exists in 2.1.54/qt5. I thought that the scrollbar auto-hid in Qt 5.14, but it seems my memory was wrong. 😓

I'll do some more testing now and post back in a bit. One issue I noticed so far: when switching between light/dark mode with style set to native on Linux, the distractions area becomes difficult to read.
2023-01-18T15:46:25,066641193+10:00

On Mac with native theme, both Qt5 and Qt6 look correct already. On
the Anki theme, without this change, we get the fusion-style scrollbars
instead of the rounded ones.
@dae
Copy link
Member

dae commented Jan 18, 2023

Well, that was fun. Qt is being really weird here - it seems like self._apply_palette(app) alternates between two different appearances shades for the canvas. I've checked the code and we're setting the same RGB values each time, so this seems like a Qt bug. If I modify this function to run every time the theme check is made, I get this:

    def apply_style_if_system_style_changed(self) -> None:
        theme = aqt.mw.pm.theme()
        if theme != Theme.FOLLOW_SYSTEM:
            return
        if True: # self._determine_night_mode() != self.night_mode:
            self.apply_style()

@dae
Copy link
Member

dae commented Jan 18, 2023

Screen.Recording.2023-01-18.at.5.16.18.pm.mov

@dae
Copy link
Member

dae commented Jan 18, 2023

So, the question is how to resolve it. The following seems to work for me:

diff --git a/qt/aqt/theme.py b/qt/aqt/theme.py
index 9979d70b9..ca8c36c4d 100644
--- a/qt/aqt/theme.py
+++ b/qt/aqt/theme.py
@@ -61,6 +61,7 @@ class ThemeManager:
     _icon_size = 128
     _dark_mode_available: bool | None = None
     _default_style: str | None = None
+    _current_widget_style: AnkiStyles | None = None
 
     def rtl(self) -> bool:
         return is_rtl(anki.lang.current_lang)
@@ -218,16 +219,19 @@ class ThemeManager:
                 return get_linux_dark_mode()
 
     def apply_style_if_system_style_changed(self) -> None:
-        theme = aqt.mw.pm.theme()
-        if theme != Theme.FOLLOW_SYSTEM:
-            return
-        if self._determine_night_mode() != self.night_mode:
-            self.apply_style()
+        self.apply_style()
 
     def apply_style(self) -> None:
         "Apply currently configured style."
         app = aqt.mw.app
-        self.night_mode = self._determine_night_mode()
+        new_theme = self._determine_night_mode()
+        theme_changed = self.night_mode != new_theme
+        new_style = aqt.mw.pm.get_widget_style()
+        style_changed = self._current_widget_style != new_style
+        if not theme_changed and not style_changed:
+            return
+        self.night_mode = new_theme
+        self._current_widget_style = new_style
         if not self._default_style:
             self._default_style = app.style().objectName()
         self._apply_palette(app)

Do you think that would work? Apologies if it seems like an obvious question; I fear I haven't yet fully understood all the issues you're trying to deal with here.

Incidentally, I think we could improve on the naming - "style" and "theme" are easily confused. How about something like AnkiStyles->WidgetStyle?

@kleinerpirat
Copy link
Contributor Author

I fear I haven't yet fully understood all the issues you're trying to deal with here.

Do you mean the whole PR? I apologize for my poor documentation. If you want, I can create a comprehensive list of changes.

How about something like AnkiStyles->WidgetStyle?

Sounds good.

@dae
Copy link
Member

dae commented Jan 18, 2023

I think I'm ok with the outline of everything, it's just the specific handling of follow_system vs light/dark mode that I don't seem to have fully understood yet. Would the patch I posted above cause problems? If not, would you like me to make that change or would you like to?

@kleinerpirat
Copy link
Contributor Author

kleinerpirat commented Jan 18, 2023

I already did the writeup, so I'll post it anyway:

Qt Widget Gallery

Added a button in the debug console to open a dialog filled with all important Qt Widgets. It can be expanded in the future and could do with sample items in QListView, QTreeView and QTableView.

New Preferences Layout

  • Added an Appearance tab
  • Renamed Scheduling tab to Review
  • Grouped individual settings with QGroupBox. Several options were moved between tabs

Widget Style Setting

Introduced a dropdown to choose the Qt widget style applied to the app.
Options:

  • Anki: custom styles based on Fusion (enabled by default on Win/Lin)
  • Native: system default Qt style (enabled by default on macOS)
    • If we find a way to apply GTK themes to Anki again, it should happen in this mode.

Minimalist Mode

When toggled on, this mode regresses the webviews to a style similar to < 2.1.54. Properties that should not be used in this mode:

  • prominent box-shadow
  • backdrop-filter: blur
  • large border-radius
  • "excessive" padding

Toolbar Changes

  • Added an option to collapse the bottom toolbar and made it possible to choose between Always and Full screen only separately for the top and bottom bar.
  • Made background copy from main webview more robust (adjusts to resized window).

Miscellaneous

  • Reduced the interval for the Linux system theme check from 300s to 5s. This enables theme transitions in a tolerable time frame.
  • Added hook body_classes_need_update to apply changes of settings involving webview body classes immediately. Such settings are:
    • Reduce motion
    • Minimalist mode
  • Menubar now hides in full screen mode to reduce distractions. It hides/shows with the top toolbar.
  • Added a bunch of missing Qt stylesheet definitions (e.g. to distinguish different button states) to reach parity with the Fusion style.
    • Added QSlider and QGroupBox styles.

@kleinerpirat
Copy link
Contributor Author

kleinerpirat commented Jan 18, 2023

I'm getting an error on ./run after 0d945cf:

    Updating crates.io index
error: failed to select a version for the requirement `async-compression = "^0.3.15"` (locked to 0.3.15)
candidate versions found which didn't match: 0.3.2, 0.3.1, 0.3.0, ...
location searched: crates.io index
required by package `anki v0.0.0 (/home/kleinerpirat/Documents/GitHub/anki/rslib)`

Edit: to be more precise, it seems to be caused by cf45cbf.

@dae
Copy link
Member

dae commented Jan 18, 2023

The write-up will come in handy when writing a summary for the beta anyway :-)

Hmm, that error is strange - https://crates.io/crates/async-compression is available on crates.io. What does 'cargo --version' print?

@kleinerpirat
Copy link
Contributor Author

It prints cargo 1.66.0.

@dae
Copy link
Member

dae commented Jan 18, 2023

Presumably it prints 1.65 if you run it inside Anki's source folder? Presumably Cargo.lock hasn't been modified since checking out the latest commit? If you remove out/ and build again, does that make a difference? If not, how about if you remove ~/.cargo/registry/index?

@kleinerpirat
Copy link
Contributor Author

Deleting out/ and build didn't fix the issue, but removing ~/.cargo/registry/index did. Anki is building right now.

Prior to the error, something had gone wrong on a build due to low system storage space. Perhaps that caused this glitch in the matrix. Thanks for the help!

kleinerpirat and others added 3 commits January 18, 2023 10:40
Co-Authored-By: Damien Elmes <dae@users.noreply.github.com>
Co-Authored-By: Damien Elmes <dae@users.noreply.github.com>
@kleinerpirat
Copy link
Contributor Author

I applied your changes and it seems to work well. Regarding the Qt5 text issue: It seems a bit random, because I got different issues than you. Dark theme displays fine, but Light looks like this:

image

It seems to be isolated to the Distractions group in the preferences, so it's likely just a dirty .ui file 🤔

that caused an error when opening the widget gallery on Qt5.
@kleinerpirat
Copy link
Contributor Author

Now I can't reproduce it anymore 👀

@dae
Copy link
Member

dae commented Jan 18, 2023

This is looking in pretty good shape now on Mac/Linux. Are you happy for me to merge it, or was there anything you wanted to do first?

A couple of points that will need addressing, but can be done in a follow-up PR:

  • the buttons are somewhat inconsistent at the moment - see the bottom of the deck list vs preferences vs top of editor in the following screenshot
  • native+dark is broken on Windows. Perhaps we should just hide the native option on Windows?

@dae
Copy link
Member

dae commented Jan 18, 2023

Screenshot 2023-01-18 at 8 23 46 pm

@dae
Copy link
Member

dae commented Jan 18, 2023

2023-01-18T20:32:53,280080207+10:00

also exclude native option from dropdown just in case.
@kleinerpirat
Copy link
Contributor Author

Perhaps we should just hide the native option on Windows?

I hid it now. We might get requests from Windows users who'd like to set Anki to Fusion, though. I guess we'll have to see. For now I'd be very happy if you could merge this in.

@dae dae merged commit f169ee0 into ankitects:main Jan 18, 2023
@tatsumoto-ren
Copy link
Contributor

Thank you! It's better now.

I still see some padding at the bottom of the toolbar but it could probably be fixed by adjusting height to fit.

screenshot-2023-01-19-02-08-57

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.

3 participants