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

kde: replace kdeglobals with Kvantum theme #142

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

bluskript
Copy link

@bluskript bluskript commented Aug 6, 2023

The current KDE theming system is pretty unreliable (dolphin and many other programs ignore the theme and a lot of the time the apps that do theme are completely unreadable), I always found kvantum to give a lot more pleasant themes. So, I based this off of Catppuccin's kvantum theme and it turned out to work pretty well.

It isn't perfect, and there's some weirdnesses in some colorschemes sometimes, but that's kind of how all of base16 is.

Here's Dolphin, OpenRGB, and Strawberry open all themed correctly (Strawberry is QT6 so that works too!)
image
image
image
image

@bluskript
Copy link
Author

One thing to note, I needed to use qt5ct to manually set the icon theme, before this dolphin showed no icons and it was very bad. I'm not sure, what should the default behavior be? Should be have some kind of icon setting feature?

@danth
Copy link
Owner

danth commented Sep 10, 2023

dolphin and many other programs ignore the theme

This is unusual because when I tested the original implementation, Dolphin followed the Stylix theme without any issues. Even the folder icons were coloured to match the theme. But I agree, Kvantum is probably more reliable if we can make it work.

I needed to use qt5ct to manually set the icon theme, before this dolphin showed no icons and it was very bad

This is something which we will need to be handle automatically. As a start, would it be possible to have it always set the default icon theme?

@danth danth linked an issue Sep 10, 2023 that may be closed by this pull request
@danth danth changed the title add kvantum theming support Replace kdeglobals with Kvantum theme Sep 10, 2023
@Kasper24
Copy link
Contributor

Kasper24 commented Oct 4, 2023

2023-10-04_03-40

I think rather than changing the text color here, it would make more sense to use a darker color for active menu item.
Stylix gtk in comprassion:
image

modules/kvantum/hm.nix Outdated Show resolved Hide resolved
@sofiedotcafe
Copy link

+1

@bluskript
Copy link
Author

This is something which we will need to be handle automatically. As a start, would it be possible to have it always set the default icon theme?

@danth I addressed this issue with my latest commit, with these changes I think this should be ready.

modules/kvantum/hm.nix Outdated Show resolved Hide resolved
modules/kvantum/kvconfig.mustache Outdated Show resolved Hide resolved
@nonetrix
Copy link

nonetrix commented Dec 14, 2023

Isn't applying for me at all oddly, but it does setup Kvantum, QT theming is rather annoying on NixOS in general could be improved but it's beside the point, was hoping this makes it a lot easier at least on my end. Hoping this gets merged in and fixed, looking forward to it just working without much of a hassle
image

modules/kvantum/hm.nix Outdated Show resolved Hide resolved
@trueNAHO trueNAHO changed the title Replace kdeglobals with Kvantum theme kde: replace kdeglobals with Kvantum theme Feb 9, 2024
@danth danth mentioned this pull request Feb 10, 2024
@Suyashtnt
Copy link

I'm having some weird theming issues within Anki right now. Would this change fix this?
image

@ghost
Copy link

ghost commented Feb 25, 2024

+1

@musjj
Copy link
Contributor

musjj commented Mar 24, 2024

Are there any blockers here? Would love to see this merged.

@nonetrix
Copy link

I'm having some weird theming issues within Anki right now. Would this change fix this? image

Anki is always kinda weird with theming, somewhat need to theme it itself with extension that I forgot the name of. But mostly because of how a lot of the UI is just QTWebEngine which is pretty much Chromium. Not sure if this is related though

@danth
Copy link
Owner

danth commented May 3, 2024

The previous implementation based on kdeglobals also themed desktop elements on KDE such as window decorations and panels (when it was working correctly). This does not appear to be possible using Kvantum.

@exzombie
Copy link

exzombie commented May 3, 2024

You're right. The machine where I applied the HM+stylix configuration was pretty fresh, no stylix nor any other previous configuration. Now, I tried applying the same configuration to another machine which was previously manually configured, and both Gwenview and Okular worked as they are supposed to. There was a residual kdeglobals file there, not present in the first machine. After removing the kdeglobals file, Okular and Gwenview looked broken the same way as on the first machine.

So, it looks like this PR will need to include kdeglobals to at least configure the colors, even if the majority of work is delegated to Kvantum.

@danth
Copy link
Owner

danth commented May 3, 2024

The issue with the original implementation may also be caused by a similar problem.

Using the testbed VM, the original implementation always themes KDE fully, once you have restarted Home Manager due to the fact that it can only apply the configuration while KDE is running.

Perhaps we should put more work into improving the way kdeglobals is managed before resorting to Kvantum?

KDE has a high number of settings across different files, any of which could be causing kdeglobals not to apply correctly on a system with existing state.

@e-tho
Copy link

e-tho commented May 8, 2024

Perhaps we should put more work into improving the way kdeglobals is managed before resorting to Kvantum?

Isn't kdeglobals a different problem from the Kvantum module?

The theming of Qt applications should be DE agnostic, just like the Gtk module.
Different desktop environments could then use their own Qt implementation by overwriting the QT_QPA_PLATFORMTHEME variable.
For example: plasma-integrations for KDE, qgnomeplatform, for Gnome and qtct for WMs/Compositors.

This would unblock the situation regarding this PR and shift the responsibility for integration to the desktop environment in their respective modules.

@danth
Copy link
Owner

danth commented May 10, 2024

Isn't kdeglobals a different problem from the Kvantum module?

Correct. I just noticed that, contrary to the title of the PR, the original KDE module is no longer removed. This was originally the case given 20b08d3, but at some point it must have been restored.

Different desktop environments could then use their own Qt implementation by overwriting the QT_QPA_PLATFORMTHEME variable.

Absolutely. See #203 regarding this.

@danth
Copy link
Owner

danth commented May 10, 2024

The following screenshots were taken after restarting a few services to get kdeglobals to apply fully. The need to restart is not relevant to whether we merge this PR, since it occurs on the master branch too.

Original appearance:

image

Appearance with this PR:

image

The only major blocker now is that the icons are dark on a dark background, which makes them very difficult to see.

The Kvantum theme does look worse than Breeze IMHO, however since kdeglobals is still configured we can presumably still access Breeze when using KDE by disabling this new target. In the future, we can add a proper option to switch between Breeze/Adwaita/other designs (see the issue linked in my previous comment).

Looking forward to getting this merged so we can have proper Qt support across all desktop environments :)

@Jackaed
Copy link
Contributor

Jackaed commented May 11, 2024

Since you can configure qtct to use Breeze, is there not a way to configure a Breeze theme without using kdeglobals? That way we can just use breeze and get consistent aesthetics across different desktops, rather than having to "settle" for Kvantum on WMs.

I also think it might be worth re-evaluating how icons are handled in general - would a global icon theme setting be a good idea/worth pursuing?

@NovaViper
Copy link

🤔 Maybe we can look at how the HM module, Plasma-Manager handles applying themes (icons, lookAndFeel, etc.) for KDE Plasma? This module in particular seems to do a good job in applying themes for Plasma 6 (which stylix currently has trouble applying, related to #350)

@Jackaed
Copy link
Contributor

Jackaed commented May 11, 2024

Okay so I've done some more testing, it is possible to get it working with kdeglobals if you install libsForQt5.plasma-workspace, set style to Breeze and set your theme to default, and run plasma-apply-lookandfeel --apply stylix. Proof of this working on Hyprland:

image

This is a bit of a hack and I don't think it's how we should choose to do things - instead, I think the best way would be to use qt5ct's "custom" color settings as a target, which means we don't depend on Kvantum. I also imagine the settings for theming through qt5ct are similar to doing it through kdeglobals. It would also allow us to theme qt6 consistently through qt6ct.

@Jackaed
Copy link
Contributor

Jackaed commented May 11, 2024

Okay more testing with qt5ct makes me realize that certain applications seem to get their colors from the global theme regardless of your choice of colors for qt5ct. It might be that my hacky way of doing things isn't as bad as I initially thought.

@Jackaed
Copy link
Contributor

Jackaed commented May 11, 2024

This is such a mess. Firstly, I can only get plasma-apply-lookandfeel --apply stylix to work if my current directory is .nix-profile. Won't even allow an objective path.

If I try and get it working without qt5ct, I get something like this when using the command QT_QPA_PLATFORMTHEME=kde QT_STYLE_OVERRIDE=Breeze dolphin:

image

I can get it to work with qt5ct, and get the screenshot in my above post, but I'm not configuring my colors through qt5ct in that instance, I'm just telling it to use the system defaults, but it seems to just work.

Make it make sense, I'm losing my mind over this.

The other weird thing is that for whatever reason, the moment you use qt5ct, the bottom right storage space indicator becomes incredibly ugly (again see screenshot in above post) and I have no clue why that is. Hopefully there's some environment variable that I'm missing or something because this is driving me insane.

Edit:

So my best guess at the moment is that all qtct does when you set colour scheme to default is convert whatever is in kdeglobals into its own theming system and works from there. The reason for the questionable dolphin theming then comes from dolphin also independently reading kdeglobals when setting the background colour for the window.

I think the best bet is to use qtct and write directly to kdeglobals for the theme. The only inconsistency this causes is the weird drive usage thing not looking like it should on Breeze, as well as Breeze icons not being able to theme themselves according to the system theme. I'll see if there's anything you can do about it within qtct.

@Jackaed
Copy link
Contributor

Jackaed commented May 12, 2024

OKAY so I've finally gotten it working in the same way that it works on KDE on a non-kde system. The thing I was missing is that in order for QT_QPA_PLATFORMTHEME=kde to work, you need to have libsForQt5.plasma-integrations installed.

Home-manager will install this for you if you set qt.platformTheme="kde", but also installed libsForQt5.systemsettings which I think is undesirable for most people not on KDE.

When you do so, the theming information is then read in from kdeglobals. Therefore, in order to get stylix working as intended on other systems, we just need to write what we've been writing to .nix-profile/share/color-schemes/<theme-name>.colors to .config/kdeglobals and everything seems to work more or less as intended.

You obviously also need to set QT_STYLE_OVERRIDE=Breeze and have Breeze installed for this to work, but then you get pretty much identical functionality to what you get on KDE, including proper coloring of icons.

Apologies if none of this is new information but I've gone down the rabbit hole at this point, so I figured I may as well write it up.

@danth
Copy link
Owner

danth commented May 15, 2024

@Jackaed That sounds great! If you have time, would you be able to put it together as a separate PR?

@Jackaed
Copy link
Contributor

Jackaed commented May 15, 2024

@danth Sure, I'll have a look and see what I can do. Reading through the code, there's a few things that I'm not sure I fully understand - not sure that github issues is the right place to discuss that, can we speak on matrix?

@danth
Copy link
Owner

danth commented May 15, 2024

Absolutely, feel free to use either the public chat #stylix:danth.me or message me directly @danth:danth.me

@donovanglover
Copy link
Contributor

Now that 24.05 is stable, we can change qt.platformTheme back to qt.platformTheme.name to remove the build warning.

modules/qt/hm.nix Outdated Show resolved Hide resolved
@MatthewCroughan
Copy link

What is blocking the merge of this generally?

@trueNAHO
Copy link
Collaborator

trueNAHO commented Nov 7, 2024

What is blocking the merge of this generally?

Is it possible to rebase this PR on top of master to simplify the manual testing process? I am encountering some errors when trying to replace my Stylix input from 04afcfc with e53d92d. However, this problem could just be a result of my setup.

@donovanglover
Copy link
Contributor

Feel free to try my fork or use curl https://github.com/donovanglover/stylix/commit/87612ee1cb96cc820abfdeba3724877141fcc4fa.patch | git am with your own repo

@trueNAHO
Copy link
Collaborator

trueNAHO commented Nov 7, 2024

Feel free to try my fork or use curl https://github.com/donovanglover/stylix/commit/87612ee1cb96cc820abfdeba3724877141fcc4fa.patch | git am with your own repo

This PR successfully builds on my setup after setting programs.kitty.enable = false;, as I require commit f95022b ("stylix: downgrade and lock tinted-kitty input (#589)"). However, I have not yet tested if the fork works as expected beyond building.

Unless the original author, @bluskript, is willing to take over this PR again, it might be better to supersede it with a new one that is properly rebased on top of danth/master.

Does @donovanglover's fork resolve the issue mentioned in #142 (comment)?

Please give proper credit in the squashed commit message of this rather old PR by giving Blusk <[email protected]> honorable commit authorship and crediting the following work at the end of the commit body:

Co-authored-by: Jackaed <[email protected]>
Reviewed-by: Daniel Thwaites <[email protected]>
Reviewed-by: NAHO <[email protected]>
Tested-by: NAHO <[email protected]>

It might be good to re-confirm Daniel (@danth) and NAHO's (@trueNAHO) credit tags before merging into danth/master.

Please send a comment if I missed any other up-to-date mention worthy contributions.


@donovanglover, do you want to take over this PR?

@donovanglover
Copy link
Contributor

I'm quite busy at the moment but anyone is free to continue working on this

@bluskript
Copy link
Author

bluskript commented Nov 9, 2024

Hi, I don't really use NixOS or Github much anymore so I can't really test the features, but I rebased this branch onto master, although it still needs someone to actually test if it still works. If further changes are needed someone else taking the PR would make sense I think.

The main blocker for this PR iirc is that in some places it actually causes theming regressions like in Anki but I haven't had the time to investigate why, and there was discussion that this could be done without kvantum, but again this is a lot of work to figure out. QT theming is just really confusing.

@MatthewCroughan
Copy link

I've been testing it a bit @bluskript, thanks for the rebase! qbittorrent-qt5 formerly worked fine with this PR, but now that Nixpkgs has dumped qbittorrent-qt5 in favour of qt6, it renders incorrectly with this PR as can be seen in the image. Qt5 applications like paraview look fine though.

image

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.

kde: theme is partially applied