Skip to content

add module for configuring window rules #199

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

Merged
merged 6 commits into from
Jun 22, 2024

Conversation

icewind1991
Copy link
Contributor

Allow configuring kwin window rules through plasma-manager.

For future work it would be nice to have the apply options be a fully typed submodule with all the available options and their correct type instead of the current, more freeform, attrsOf to remove some of the guesswork from writing configurations. But I was to lazy to do that for now.

Copy link
Collaborator

@magnouvean magnouvean left a comment

Choose a reason for hiding this comment

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

I did test the example config, and after running qdbus org.kde.KWin /KWin reconfigure manually it did work well, but this had to be done manually, it did not happen automatically. With that said though, we could just drop the qdbus reloading part as the rest of plasma-manager a.t.m. needs fully logging out/in for things to work. We can worry about this in a later PR (if plasma provides the necessary tools for this to work smoothly across for most/all modules).

Otherwise it looks good, though if you could format with nixpkgs-fmt that would be great. Nice work on this :)

@magnouvean
Copy link
Collaborator

I also see the checks fail, but that probably isn't due to this. I'll see if this is caused by something else.

@icewind1991
Copy link
Contributor Author

While using this for a while I ran into an issue where if the order of rules change or rules get removed, the news set of rules gets merged "randomly" with the old set of rules. Resulting in various unexpected window rules being create.

I've solved this locally by just setting kwinrulesrc to be deleted/overwritten.

Given that this currently already behaves as overwriting all "ui defined" window rules (because it has to set the list of configured rules). It might make sense to have it always delete kwinrulesrc regardless of whether overrideConfig is set. Though this could be considered unexpected behavior.

An alternative would be to extend the files module to allow specifying individual configuration blocks to be overwritten instead of merged.

@magnouvean
Copy link
Collaborator

I see. I'll try to improve the interface so that we could enable deletion of some files even without overrideConfig (this list could start off by just having kwinrulesrc) and when I figure out a proper way of doing this it can be implemented here.

@magnouvean
Copy link
Collaborator

OK so it should now be easy to make sure that kwinrulesrc is deleted upon activation even when not using overrideConfig (it should just be adding kwinrulesrc to https://github.com/pjones/plasma-manager/blob/trunk/modules/files.nix#L61). I would say that maybe this should only be added there in the case where window-rules is not empty (i.e. builtins.length of the list bigger than 0) as it then will not reset/remove this file in the case where the user doesn't configure window-rules from plasma-manager at all.

Fixing this and removing the qdbus part (I have never gotten much luck using qdbus in general for this project, especially for reloading kwin) should be enough before we can merge this.

@icewind1991
Copy link
Contributor Author

done

@magnouvean
Copy link
Collaborator

I'm happy with this now. Thanks for this, this is really cool :)

@magnouvean magnouvean merged commit 675d5fc into nix-community:trunk Jun 22, 2024
1 check passed
@icewind1991 icewind1991 deleted the window-rules branch June 22, 2024 18:57
Asqiir pushed a commit to Asqiir/plasma-manager that referenced this pull request Jun 26, 2024
@ada4a ada4a mentioned this pull request Jul 24, 2024
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.

2 participants