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

qt: add flexible theming with sensible defaults #780

Merged
merged 15 commits into from
Feb 3, 2025

Conversation

Mikilio
Copy link
Contributor

@Mikilio Mikilio commented Jan 18, 2025

This is a continuation of PR #142. It includes some changes I've made in the past, but also my more recent and most important change that hopefully is a good step towards getting this merged. This PR should be carefully reviewed.

@Mikilio
Copy link
Contributor Author

Mikilio commented Jan 18, 2025

Local testing still in progress

Performed a rebase let me know if you want a merge instead.

@Mikilio Mikilio force-pushed the pr-142 branch 4 times, most recently from b6d4e66 to dd58e80 Compare January 18, 2025 18:46
@Mikilio Mikilio force-pushed the pr-142 branch 4 times, most recently from 8a1405f to d3865d9 Compare January 19, 2025 05:22
These changes make the PR apply the Kvatum theme only when appropriate
and creates the framework to allow for Qt styling based on the DE.
@Mikilio
Copy link
Contributor Author

Mikilio commented Jan 19, 2025

@trueNAHO @danth @bluskript @Jackaed Ready for review

@Mistrustfully
Copy link

If you don't have stylix.iconTheme.dark set in your config Stylix fails to build:

       … while evaluating a path segment
         at /nix/store/f7gh3ya0is8h5ds22dv1dskl1px7znsh-source/modules/qt/hm.nix:86:24:
           85|             style=${config.qt.style.name}
           86|             icon_theme=${iconTheme}
             |                        ^
           87|           '';

       error: cannot coerce null to a string: null

After setting it however it works fine for me.

@Mikilio
Copy link
Contributor Author

Mikilio commented Jan 23, 2025

@Mistrustfully What behavior would you have expected here? I kind of intended it to work like that, but if it was perceived as bad UX I'd like to change it.

Copy link
Collaborator

@trueNAHO trueNAHO left a comment

Choose a reason for hiding this comment

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

Considering that this PR may be somewhat fragile, it would be nice if people that are using or testing this PR could leave a comment whether it works for them, or what does not work. This hopefully gives more confidence in the correctness of this PR.

modules/qt/hm.nix Outdated Show resolved Hide resolved
modules/qt/hm.nix Outdated Show resolved Hide resolved
modules/qt/hm.nix Outdated Show resolved Hide resolved
modules/qt/hm.nix Outdated Show resolved Hide resolved
modules/qt/hm.nix Outdated Show resolved Hide resolved
modules/qt/hm.nix Outdated Show resolved Hide resolved
modules/qt/nixos.nix Outdated Show resolved Hide resolved
modules/qt/nixos.nix Show resolved Hide resolved
modules/qt/nixos.nix Outdated Show resolved Hide resolved
modules/qt/nixos.nix Outdated Show resolved Hide resolved
@trueNAHO trueNAHO changed the title Add flexible Qt theming with sensible defaults. qt: add flexible theming with sensible defaults Jan 23, 2025
@Mikilio Mikilio requested a review from trueNAHO January 24, 2025 21:54
@eblechschmidt
Copy link

I tested this PR and it works for me as well.

The only issue I ran into was the same as above with iconTheme.dark not being set. It would have been helpful to get a more useful error pointing me to setting that option in the HM module.

@Mistrustfully What behavior would you have expected here? I kind of intended it to work like that, but if it was perceived as bad UX I'd like to change it.

I also had to set targets.qt.platform = "qtct" because the default seems to be qt5ct for my system even though I don't set i t anywhere.

@Mikilio
Copy link
Contributor Author

Mikilio commented Jan 25, 2025

I also had to set targets.qt.platform = "qtct" because the default seems to be qt5ct for my system even though I don't set i t anywhere.

Thanks for pointing that out, it was a typo.

@Mikilio
Copy link
Contributor Author

Mikilio commented Jan 26, 2025

My last commit should fix the issue you encountered when not setting the icon theme. Mind that not setting iconTheme.dark or iconTheme.light will not provide you with an icon theme, but it will also not break the config anymore.

@eblechschmidt
Copy link

It now errors with the the following:

error:
       … while calling the 'head' builtin
         at /nix/store/v0g0bxsd5gw6k0jz2855f8h7l1218925-source/lib/attrsets.nix:1574:11:
         1573|         || pred here (elemAt values 1) (head values) then
         1574|           head values
             |           ^
         1575|         else

       … while evaluating the attribute 'value'
         at /nix/store/v0g0bxsd5gw6k0jz2855f8h7l1218925-source/lib/modules.nix:846:9:
          845|     in warnDeprecation opt //
          846|       { value = addErrorContext "while evaluating the option `${showOption loc}':" value;
             |         ^
          847|         inherit (res.defsFinal') highestPrio;

       … while evaluating the option `system.build.toplevel':

       … while evaluating definitions from `/nix/store/v0g0bxsd5gw6k0jz2855f8h7l1218925-source/nixos/modules/system/activation/top-level.nix':

       … while evaluating the option `warnings':

       … while evaluating definitions from `/nix/store/v0g0bxsd5gw6k0jz2855f8h7l1218925-source/nixos/modules/system/boot/systemd.nix':

       … while evaluating the option `systemd.services.home-manager-eike.serviceConfig':

       … while evaluating definitions from `/nix/store/v0g0bxsd5gw6k0jz2855f8h7l1218925-source/flake.nix':

       … while evaluating the option `home-manager.users.eike.home.file."/home/eike/.config/qt5ct/qt5ct.conf".source':

       … while evaluating definitions from `/nix/store/cb0iggncanz0ny51bv8mxidr8zl606vq-source/modules/files.nix':

       … while evaluating the option `home-manager.users.eike.home.file."/home/eike/.config/qt5ct/qt5ct.conf".text':

       … while evaluating definitions from `/nix/store/cb0iggncanz0ny51bv8mxidr8zl606vq-source/modules/misc/xdg.nix':

       … while evaluating the option `home-manager.users.eike.xdg.configFile."qt5ct/qt5ct.conf".text':

       … while evaluating definitions from `/nix/store/162043qziish2zn20aggx6am3s4q96h1-source/modules/qt/hm.nix':

       (stack trace truncated; use '--show-trace' to show the full, detailed trace)

       error: attribute 'optionString' missing
       at /nix/store/162043qziish2zn20aggx6am3s4q96h1-source/modules/qt/hm.nix:75:16:
           74|             ''
           75|             ++ lib.optionString (config.qt.style ? name) ''
             |                ^
           76|               style=${config.qt.style.name}
       Did you mean optionalString?

@Mikilio
Copy link
Contributor Author

Mikilio commented Jan 26, 2025

Oops, typo again.

@eblechschmidt
Copy link

Now I get this error:

error:
       … while calling the 'head' builtin
         at /nix/store/v0g0bxsd5gw6k0jz2855f8h7l1218925-source/lib/attrsets.nix:1574:11:
         1573|         || pred here (elemAt values 1) (head values) then
         1574|           head values
             |           ^
         1575|         else

       … while evaluating the attribute 'value'
         at /nix/store/v0g0bxsd5gw6k0jz2855f8h7l1218925-source/lib/modules.nix:846:9:
          845|     in warnDeprecation opt //
          846|       { value = addErrorContext "while evaluating the option `${showOption loc}':" value;
             |         ^
          847|         inherit (res.defsFinal') highestPrio;

       … while evaluating the option `system.build.toplevel':

       … while evaluating definitions from `/nix/store/v0g0bxsd5gw6k0jz2855f8h7l1218925-source/nixos/modules/system/activation/top-level.nix':

       … while evaluating the option `warnings':

       … while evaluating definitions from `/nix/store/v0g0bxsd5gw6k0jz2855f8h7l1218925-source/nixos/modules/system/boot/systemd.nix':

       … while evaluating the option `systemd.services.home-manager-eike.serviceConfig':

       … while evaluating definitions from `/nix/store/v0g0bxsd5gw6k0jz2855f8h7l1218925-source/flake.nix':

       … while evaluating the option `home-manager.users.eike.home.file."/home/eike/.config/qt5ct/qt5ct.conf".source':

       … while evaluating definitions from `/nix/store/cb0iggncanz0ny51bv8mxidr8zl606vq-source/modules/files.nix':

       … while evaluating the option `home-manager.users.eike.home.file."/home/eike/.config/qt5ct/qt5ct.conf".text':

       … while evaluating definitions from `/nix/store/cb0iggncanz0ny51bv8mxidr8zl606vq-source/modules/misc/xdg.nix':

       … while evaluating the option `home-manager.users.eike.xdg.configFile."qt5ct/qt5ct.conf".text':

       … while evaluating definitions from `/nix/store/3drfha8hnp71mvf1dwaazijll293rg4h-source/modules/qt/hm.nix':

       (stack trace truncated; use '--show-trace' to show the full, detailed trace)

       error: expected a list but found a string: "style=kvantum\n"

@eblechschmidt
Copy link

Ok, now it works again 👍

@Mikilio Mikilio force-pushed the pr-142 branch 2 times, most recently from 6ab23a1 to f29e5f3 Compare January 28, 2025 20:40
@Mikilio
Copy link
Contributor Author

Mikilio commented Jan 28, 2025

@trueNAHO only 1 convo left. If you agree with the solution you can just check it off.

Copy link
Collaborator

@trueNAHO trueNAHO left a comment

Choose a reason for hiding this comment

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

LGTM.

Thanks to everyone involved in this feature. Great to see this getting merged.

@trueNAHO trueNAHO merged commit b7f50a5 into danth:master Feb 3, 2025
49 checks passed
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Feb 3, 2025
With commit b7f50a5 ("qt: add flexible theming with sensible
defaults (danth#780)"), the Qt pinentry package is a reasonable default
value:

> NixOS stopped building gtk2 pinentry by default in
> NixOS/nixpkgs#270266 and there does not appear
> to be a reasonable other default.
>
> -- home-manager, "gpg-agent: don't set a default for pinentry",
>    nix-community/home-manager@4585445

Closes: danth#184
trueNAHO pushed a commit that referenced this pull request Feb 3, 2025
Link: #830
Fixes: b7f50a5 ("qt: add flexible theming with sensible defaults (#780)")

Reviewed-by: NAHO <[email protected]>
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Feb 9, 2025
Fixes: b7f50a5 ("qt: add flexible theming with sensible defaults (danth#780)")
danth pushed a commit that referenced this pull request Feb 10, 2025
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.

6 participants