home-cursor: modernize

[isabelroses]

Description

Added the .enable pattern to home.pointerCursor for the selfish reason that is https://github.com/catppuccin/nix/blob/24dac16e6babd41961d985eef3dce6a30dab2e91/modules/home-manager/cursors.nix#L26-L27.

And made the .icons directory "toggleable" via home.pointerCursor.doticons.enable. Which in my opinion is easier than writing:

{
  lib,
  config,
  ...
}:
{
  home.file = lib.mkIf (config.home.pointerCursor != null) {
    ".icons/default/index.theme".enable = false;
    ".icons/${config.home.pointerCursor.name}".enable = false;
  };
}

And removing the top-level with lib

Checklist

• Change is backwards compatible.

• Code formatted with ./format.

• Code tested through nix-shell --pure tests -A run.all
  or nix build --reference-lock-file flake.lock ./tests#test-all using Flakes.

• Test cases updated/added. See example.

• Commit messages are formatted like

  {component}: {description}
  
  {long description}
  

  See CONTRIBUTING for more information and recent commit messages for examples.

• If this PR adds a new module

  • Added myself as module maintainer. See example.

Maintainer CC

[rycee]

See 797fbbf for a prior example of introducing an enable option. That commit misses to include the change in the release notes, however. Since it is a state version change, it should be included in the 25.05 release notes.

[rycee]

Suggested change

	doticons = {
	dotIcons = {

[rycee]

Suggested change

	doticons config generation for {option}`home.pointerCursor`
	`.icons` config generation for {option}`home.pointerCursor`

[khaneliman]

Sorry, I've been slow to respond because I've been thinking through the silent stateVersion changes you've been doing vs the louder warnings to inform about the deprecations I would have expected. Originally, I expected us to add test.assert.warnings.expected to the legacy test and not need them in the modern test case.

But, maybe this is a good approach to not confuse / burden users with internal details and only new configurations have to deal with the new schema... just worried about tech debt longterm if we have to gate every change and keep it backwards compatible like that.

[isabelroses]

just worried about tech debt longterm if we have to gate every change and keep it backwards compatible like that.

The tech debt is great in my opinion too. See https://github.com/catppuccin/nix/blob/b1ff2a638afa827f1473498190a2c1cae1cf41cf/modules/home-manager/swaylock.nix#L13-L26. I see the point of stateVersion but I am not a fan and would prefer to break configs, but I also see not many people have time to fix these things. I would prefer a middle ground but submodules make it very difficult. Perhaps we can remove the submodule somehow.

[ambroisie]

Originally, I expected us to add test.assert.warnings.expected to the legacy test and not need them in the modern test case

👍 this would be my expectation, and the easiest way to on-board users onto the new options.

---

just worried about tech debt longterm if we have to gate every change and keep it backwards compatible like that.

The tech debt is great in my opinion too

[I] would prefer to break configs

You can (and you should), much the same NixOS does it: support the legacy option for one release, while deprecating it loudly, and removing it at the next branch-off. Given that home manager follows NixOS' release schedule and stateVersions, it should be very familiar to users.

[khaneliman]

Yes, sorry if I wasn't clear. I meant I'd like a loud deprecation with a stable release support. But, not gate everything silently forever with stateVersion checks, making the code base full of crazy conditions that we can't properly remove. We don't need to break a config, if we do it correctly.

I think the main thing we want here is a warning when we detect an option default priority of null for the enable option while they have a configuration that isn't empty. That way, users who have the current behavior of enabling the module via configuration would be notified to either enable or disable explicitly.

Something like this? Not sure if we need to even check that closely or we can just check for null.

options.enable.highestPrio == (lib.mkOptionDefault null).priority

[isabelroses]

Something like this? Not sure if we need to even check that closely or we can just check for null.

options.enable.highestPrio == (lib.mkOptionDefault null).priority

Since its a submodule you cannot access outside of the submodule the options.enable or at least to my knowledge.

[khaneliman]

This was supposed to be a test that expects the warning with a new one created that doesn't throw the warning when disabled properly.

[khaneliman]

this was supposed to be the test that works with setting a config without enable and threw a warning that the behavior was deprecated