brainrom / kokovp

Modern video player based on libmpv and Qt
GNU General Public License v2.0
8 stars 1 forks source link

Add an option to configure the ui & icon themes in the preferences. #13

Open bionade24 opened 1 week ago

bionade24 commented 1 week ago

~Not yet working:~

~- Resetting to "" theme~ ~- UI theming~

Which version of clang-format are u using? With the latest version in the Arch repos (18.1.8) it errored out.

brainrom commented 6 days ago

I fixed clang-format file, should work now. UI and PrefAppearance class looks good, but some code aspects needs enhancements before merge.

  1. Don't mix std::string and QString, unless you have very hard reasons.
  2. Avoid using global constants. Config keys are okay as inline constants. However if you still want to have config keys as constants, make it public static const QString fields in PrefAppearance class.
  3. Use empty string as designation for "use system theme". "<System default>" must be UI-only text and used only inline in tr().
  4. Instead of comparing with constant on config reading, save empty line or theme's name directly on PrefAppearance. Empty QString as \<System default>'s data should be added for this.
    ui->cbIconTheme->addItem(tr("<System default>"), QString());
    ...
    QString uiTheme = ui->cbUiTheme->currentData().isValid() ? ui->cbUiTheme->currentData().toString() : ui->cbUiTheme->currentText();
    Config::i().set(uiThemeConfigKey, uiTheme);
  5. Setting default values is not the responsibility of prefs dialogs and should be done only in code which uses parameter's value. For user-configurable options, it's KokoVP::readConfig() method.
  6. Save system themes names (in static const local variable, for example) at app start to use it in theme setting methods, if config contains empty string (system theme). Cancel button should revert any config changes (including changing from system default to some theme) without app restart, no matter what was configured.
  7. Please avoid using force push. Because of force-push I need to review entire PR, instead of only new commits. On merge I can squash the commits to make Git history look good.
bionade24 commented 5 days ago
1. Don't mix std::string and QString, unless you have very hard reasons.

I did so because tr() doesn't take QString, though I guess that's now irrelevant because of the other design changes. Still, wouldn't QByteArray be the better fit here as we can't guarantee that the filesystem with the themes stored on supports UTF-8?

2. Avoid using global constants. Config keys are okay as inline constants. However if you still want to have config keys as constants, make it public `static const QString` fields in `PrefAppearance` class.

Fixed.

3. Use empty string as designation for "use system theme". `"<System default>"` must be UI-only text and used only inline in tr().

How exactly should this and the conversion from/to it be done? I obviously can't save some enum(-alike) value in the config and have to store the full text name. 2 private funcs for forth- & back conversion? Or rather put them in an anon namespace?

  1. Setting default values is not the responsibility of prefs dialogs and should be done only in code which uses parameter's value. For user-configurable options, it's KokoVP::readConfig() method.

Done

6. Save system themes names (in `static const` local variable, for example) at app start to use it in theme setting methods, if config contains empty string (system theme). Cancel button should revert any config changes (including changing from system default to some theme) without app restart, no matter what was configured.

Currently, when you press abort without pressing apply first, nothing changes. Isn't this the wanted behaviour? Except for the <System default> string I don't see a need to mirror the state here. And this one thing can be solved without it. I'll stop force-pushing

brainrom commented 5 days ago

I don't see any code change, maybe you forgot to push?

brainrom commented 5 days ago
  1. Qt in general and QString in particular encoding-agnostic.

  2. ui->cbIconTheme->addItem(tr("<System default>"), QString());
    ...
    QString uiTheme = ui->cbUiTheme->currentData().isValid() ? ui->cbUiTheme->currentData().toString() : ui->cbUiTheme->currentText();
    Config::i().set(uiThemeConfigKey, uiTheme);

    Example how to add data item with data and how return data if present, otherwise return text.

  3. Wanted behavior: Apply button puts configuration into action. Cancel button reverts to the state until config editor was opened and close the editor. OK button applies configuration, save configuration to file and close the editor.

bionade24 commented 5 days ago

6. Cancel button reverts to the state until config editor was opened and close the editor.

So the application of the new settings should be reverted on pressing cancel? Or that apply shouldn't store anything if ok wasn't pressed?

bionade24 commented 5 days ago

Instead of comparing with constant on config reading, save empty line or theme's name directly on PrefAppearance. Empty QString as 's data should be added for th

~I already wanted to object to this earlier, but when I tested it again, it seemling worked. Now on that I've pulled the code on my laptop, the problem and reason for wanting to object earliert apperared again. Setting QString() or "" as iconTheme will lead to the icon theme not being found, thus Qt switches to the fallback. For the UI theming it seems to work with an empty string, for the icon theming no theme at all has to be set.~

~With this in mind, how should applying the <System default> icon theme again be implemented when the logic should move from kokovp.cpp to prefappearance.cpp? ~

Edit: I have to furthe investigate this.

Was caused by the change from storing "<System default>" to QString()

bionade24 commented 3 days ago

@brainrom You wanted me to leave the mechanism of backing up the old config, saving the new config on apply and restoring the old one on cancel, right? Then it should be all done now.