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

Initial try to add support for immutable settings #94

Merged
merged 6 commits into from
Mar 27, 2024
Merged

Initial try to add support for immutable settings #94

merged 6 commits into from
Mar 27, 2024

Conversation

magnouvean
Copy link
Collaborator

This will change the files module quite a bit again in order to allow for additional options for the values. Where the values of a key in plasma-manager before was just a bool/int/float and so on, it now will have it's own custom type, which at the moment has two properties, namely "value" and "immutable". Another thing I had to do here was to revert the recursive attributes and go back to an improved version of the old format. While I think that the recursive options solution was good in comparison to what we had, I think it also makes things a lot harder to extend, and may even make some things impossible to distinguish between (you can probably work around this, but not without introducing weird quirks or hacky solutions), and when compared to the old system then becomes quite limiting for what we actually can achieve functionality-wise in plasma-manager. For example I would assume there is a reason why home-manager doesn't take much use of them even though they probably could in some places.

My proposal is not that we should go back to the old format though, which I think wasn't ideal. A new separator which I think makes a lot more sense is "/" as this isn't supported in kde config-file group-names, meaning there should not be any conficts here. I also added an option to escape this with "\\/" just as when we used periods (though I honestly don't think there should be much reason to use this).

A thing I like about this new format is that this will resemble the home-manager interface a lot closer, which I think is natural as this project heavily builds on the ideas of home-manager. If anyone has any good arguments against this (for example @MatthewCash) format feel free to post them here. This should close #89.

@magnouvean magnouvean marked this pull request as draft March 25, 2024 13:37
@magnouvean
Copy link
Collaborator Author

What do you think about this @toast003. I think it is important that we create an interface which allows for ease of extensibility, and while the project is still in the unstable period, now is the time we can allow for breaking changes for the long-term benefit.

@magnouvean
Copy link
Collaborator Author

I could also achieve the same results without having to add .value at the end of each key, hence most configs wouldn't break, but this doesn't really resemble the home-manager style very much, so I don't know if continuing this support is necessarily good.

@toast003
Copy link
Collaborator

I agree, I think having to set value would be the cleaner option, although it does break existing configs as you said

@toast003
Copy link
Collaborator

And since you mentioned ease of extensibility I think that after this pr gets merged we could implement [$e] (which would lets us use environment variables) pretty easily

@magnouvean
Copy link
Collaborator Author

I can do it right away I think. This draft still needs some tweaks with documentation I think so I'll have to fix that anyway and implementing this should be easy. There also is the $[ei] flag apparently, which expands variables without inserting in for the variable. Also I didn't even bother with the hotkeys module as that's been broken for a while (I'll take a look at the hotkeys pr after this is merged and make it work with this).

@magnouvean magnouvean marked this pull request as ready for review March 27, 2024 12:37
@magnouvean magnouvean merged commit 3399f8b into nix-community:trunk Mar 27, 2024
1 check passed
Asqiir pushed a commit to Asqiir/plasma-manager that referenced this pull request Mar 29, 2024
@quentinmit
Copy link
Contributor

I just updated my flake reference to pull this PR in and found it's a significant readability regression for my configs :(

I think there are two things that could drastically improve the UX:

  • Use types.coercedTo to support bare values (without the extra .value). Something like:
types.coercedTo basicSettingsType (value: { inherit value; }) advancedSettingsType

would allow both

      "kwinrc"."org.kde.kdecoration2"."ButtonsOnLeft" = "SF";
      "kwinrc"."Desktops"."Number" = {
        value = 8;
        # Forces kde to not change this value (even through the settings app).
        immutable = true;
      };

This would mean that the vast majority of definitions don't need an extra .value.

  • With that, I think you can support the recursive syntax again. I can't think of any weird quirks this would introduce?

@magnouvean
Copy link
Collaborator Author

The main reason why the old syntax had problems was that it caused overly complicated recursive syntax and implementations, but since we had to do away with the recursive syntax anyway we probably could quite easily re-add support for keys without explicitly adding .value at the end. Personally though I think the new syntax is fine as it more closely mimics that of home-manager which I think should be a goal.

@quentinmit
Copy link
Contributor

quentinmit commented Apr 22, 2024

The main reason why the old syntax had problems was that it caused overly complicated recursive syntax and implementations, but since we had to do away with the recursive syntax anyway we probably could quite easily re-add support for keys without explicitly adding .value at the end. Personally though I think the new syntax is fine as it more closely mimics that of home-manager which I think should be a goal.

WDYM by "mimics that of home-manager"? Do you mean something like home.file."foo/bar".text = "baz"; or the similar xdg.configFile, etc.? I think that's significantly different for two reasons:

  • Those are file paths, which already have a well-defined separator.
  • There are usually far fewer config files, so there isn't as much overhead caused by adding .text.

In places where you are defining nested config that isn't based on file paths, home-manager does allow recursive definitions without any extra fields. For example, programs.git.extraConfig.url."[email protected]".insteadOf = "https://github.com/"; or programs.neovim.coc.settings.languageserver.haskell.command = "haskell-language-server-wrapper";

To take a real example from my config, can you explain what the "overly complicated recursive syntax" is for

programs.plasma.configFile.kcminputrc.Libinput."2362"."628"."PIXA3854:00 093A:0274 Touchpad" = {
  ClickMethod = 2; # Two-finger click to right click
  TapToClick = true;
  TapDragLock = true;
};

versus (if I understand this PR correctly)

programs.plasma.configFile.kcminputrc."Libinput/2362/628/PIXA3854:00 093A:0274 Touchpad" = {
  ClickMethod.value = 2; # Two-finger click to right click
  TapToClick.value = true;
  TapDragLock.value = true;
};

?

@quentinmit
Copy link
Contributor

I sent #144 with the coercedTo usage, if you want to take a look.

@magnouvean
Copy link
Collaborator Author

Ok, I can take a look tomorrow :)

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.

Allow immutable properties
3 participants