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.

Sign up for GitHub

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-cursor: use mkRenamedOptionModule for xsession.pointerCursor #3545

Closed
wants to merge 1 commit into from
Closed

home-cursor: use mkRenamedOptionModule for xsession.pointerCursor #3545

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 12 additions & 17 deletions modules/config/home-cursor.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,10 @@ let
x11 = {
enable = mkEnableOption ''
x11 config generation for <option>home.pointerCursor</option>
'';
'' // {
default = config.xsession.enable;
defaultText = literalExpression "config.xsession.enable";
};

defaultCursor = mkOption {
type = types.str;
Expand All @@ -43,7 +46,10 @@ let
gtk = {
enable = mkEnableOption ''
gtk config generation for <option>home.pointerCursor</option>
'';
'' // {
default = config.gtk.enable;
defaultText = literalExpression "config.gtk.enable";
};
};
};
};
Expand All @@ -56,38 +62,27 @@ in {
meta.maintainers = [ maintainers.polykernel maintainers.league ];

imports = [
(mkAliasOptionModule [ "xsession" "pointerCursor" "package" ] [
(mkRenamedOptionModule [ "xsession" "pointerCursor" "package" ] [
"home"
"pointerCursor"
"package"
])
(mkAliasOptionModule [ "xsession" "pointerCursor" "name" ] [
(mkRenamedOptionModule [ "xsession" "pointerCursor" "name" ] [
"home"
"pointerCursor"
"name"
])
(mkAliasOptionModule [ "xsession" "pointerCursor" "size" ] [
(mkRenamedOptionModule [ "xsession" "pointerCursor" "size" ] [
"home"
"pointerCursor"
"size"
])
(mkAliasOptionModule [ "xsession" "pointerCursor" "defaultCursor" ] [
(mkRenamedOptionModule [ "xsession" "pointerCursor" "defaultCursor" ] [
"home"
"pointerCursor"
"x11"
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, in this case mkAliasOptionModule was used for a reason. If you look at the list entry below the fold in GitHub (click the ⬇) you'll notice that there is actually already a more helpful warning printed. The main difference is that it includes a reference to home.pointerCursor.x11.enable, which would be missing if we use mkRenamedOptionModule.

That said, I think we could get away by changing the warning text below slightly. Something simple like

To enable X11 specific cursor configurations, please enable 'home.pointerCursor.x11.enable'.

may be sufficient. The other options will get their own warnings through mkRenamedOptionModule.

Copy link
Member

Choose a reason for hiding this comment

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

That said², I'm not certain why the home.pointerCursor.x11.enable option is there at all. Presumably the user would like to use the cursor settings whenever config.xsession.enable is set? @polykernel Can you recall why that is?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did see that, that's why I said "at the cost of getting two warnings". I don't think it's too bad, although it might need a change in wording.

Probably a better solution would be to make mkRenamedOptionModule accept a "replacement instructions" message, just like mkRemovedOptionModule does.

But if we can just remove home.pointerCursor.x11.enable that's even simpler.

Copy link
Member Author

@ncfavier ncfavier Dec 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Or (assuming we do need home.pointerCursor.x11.enable) maybe we should indeed use mkRemovedOptionModule and fail instead of accepting a configuration that suddenly does not have X cursors anymore (although it's probably a bit late to be revisiting the migration path for this option).

Copy link
Contributor

@polykernel polykernel Jan 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

That said², I'm not certain why the home.pointerCursor.x11.enable option is there at all. Presumably the user would like to use the cursor settings whenever config.xsession.enable is set? @polykernel Can you recall why that is?

IIRC, the enable option was introduced as a toggle for each backend in case more options are added for the backend in the future.

After some thought, I think each home.pointerCursor.<backend> can also be a nullable submodule which achieves the same functionality as the enable option (e.g setting home.pointerCursor.x11 = {} would enable the x11 backend).

Copy link
Member Author

Choose a reason for hiding this comment

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

If you can submit a PR for that, that would be great! Similarly, I think the GTK backend can simply be enabled if gtk.enable is true. Then both enable options should get a mkRemovedOptionModule explaining why they've been removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

When drafting the PR, I noticed that it is not possible to maintain compatibility with the old xsession.pointerCursor module after the removal of x11.enable. Since the xsession.initExtra and xresources.properties settings become dependent on xsession.enable, this means it is not possible to enable the full X cursor configuration in isolation anymore which was supported by xsession.pointerCursor. With the gtk.enable option, the same type of issue arise because of coupled dependencies. For example, if a user wants to enable X cursor configurations via home.pointerCursor and has gtk.enable set to true only for gtk theme configuration, then the gtk cursor configuration also gets set by the home.pointerCursor unbeknownst to the user.

Is the incompatibility introduced acceptable for the module? In any case, I have submitted a draft PR as a work in progress.

Copy link
Member Author

@ncfavier ncfavier Jan 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe it would be much simpler to keep the options, but make x11.enable default to config.xsession.enable and gtk.enable default to config.gtk.enable. That way we don't lose backwards compatibility (although those use cases seem questionable to me) and we don't have to bother with the whole "mkRemovedOptionModule in submodules" situation, and we don't have to tell the user to set anything else.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. Would you be able to append the default value changes to this PR or should I make another PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

"defaultCursor"
])

({ ... }: {
warnings = optional (any (x:
getAttrFromPath
([ "xsession" "pointerCursor" ] ++ [ x ] ++ [ "isDefined" ])
options) [ "package" "name" "size" "defaultCursor" ]) ''
The option `xsession.pointerCursor` has been merged into `home.pointerCursor` and will be removed
in the future. Please change to set `home.pointerCursor` directly and enable `home.pointerCursor.x11.enable`
to generate x11 specific cursor configurations. You can refer to the documentation for more details.
'';
})
];

options = {
Expand Down