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

home.pointerCursor: remove backend specific enable options #3558

Closed
wants to merge 1 commit into from
Closed

home.pointerCursor: remove backend specific enable options #3558

wants to merge 1 commit into from

Conversation

polykernel
Copy link
Contributor

@polykernel polykernel commented Jan 3, 2023

This PR removes the home.pointerCursor.<backend>.enable options for each backend in favour of using global options to toggle backend specific cursor configurations. This change was motivated by #3545 and improves integration of home.pointerCursor with other home-manager modules.

Description

cc: @rycee @ncfavier

Checklist

  • Change is backwards compatible.

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all.

  • 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.

    • Added myself and the module files to .github/CODEOWNERS.

This PR removes the `home.pointerCursor.<backend>.enable` options for
each backend in favour of using global options to toggle backend
specific cursor configurations. This change was motivated by #3545 and improves integration of
`home.pointerCursor` with other home-manager modules.
@polykernel polykernel requested a review from league as a code owner January 3, 2023 05:10
@polykernel
Copy link
Contributor Author

I wasn't able to use mkRemovedOptionModule to add warnings for home.pointerCursor.x11.enable and home.pointerCursor.gtk.enable as per #3545 (comment) because mkRemovedOptionModule sets a pseudo option as an option tree via setAttrByPath but part of the attr path (i.e x11.enable and gtk.enable) is inside the submodule which causes a conflict definition error.

I am not really sure how to resolve this error, perhaps using getSubOptions to get an option tree representation to operate on?

@ncfavier
Copy link
Member

ncfavier commented Jan 3, 2023

The type of home.pointerCursor isn't submodule, it's null or submodule, so this check fails, so the option doesn't get merged into the submodule (even if it did, mkRemovedOptionModule would probably still not work correctly across submodule boundaries).

Ideally we could add imports = [ (mkRemovedOption ["x11" "enable"] "...") ]; to the submodule itself, but that won't work yet because we don't have submodule assertions (or NixOS/nixpkgs#207187). cc @infinisil @roberth

In the meantime, we'll probably have to use the same hack as in xdg-desktop-entries.nix: add an assertions option to the submodule and extract the assertions into the top-level assertions.

@polykernel
Copy link
Contributor Author

I am closing this PR in favor of the approach taken in #3545 which avoids introducing backward incompatibility and the mkRemovedOptionModule issue across submodule boundaries (see the linked PR for more details).

@polykernel polykernel closed this Jan 4, 2023
@ncfavier ncfavier mentioned this pull request Jan 11, 2023
5 tasks
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