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

hyprland: add hyprpaper.enable option and improve code #501

Merged

Conversation

trueNAHO
Copy link
Collaborator

@trueNAHO trueNAHO commented Aug 10, 2024

This patchset contains the following commits:

  • hyprland: add hyprpaper.enable option
  • hyprland: structurally separate unconditional and conditional settings
  • hyprland: inline single-use variable
  • hyprland: reduce local variable scopes
  • hyprland: remove with statement

Closes: #499


Note that I have not actually tested this PR, and as such it may contain silly errors.

This PR has been tested:

Yup, just tested on your branch and everything hyprpaper was removed upon rebuilding once it was set to false. And once adding hyprpaper manually, my wallpaper script is working perfect so far. So seems to be working to me.

-- #499 (comment)

@trueNAHO
Copy link
Collaborator Author

It would be nice to have this merged before #519.

Copy link
Owner

@danth danth left a comment

Choose a reason for hiding this comment

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

Couldn't we just use lib.mkDefault when setting services.hyprpaper.enable, then users can override it by setting that to false directly?

@trueNAHO
Copy link
Collaborator Author

trueNAHO commented Sep 2, 2024

Couldn't we just use lib.mkDefault when setting services.hyprpaper.enable, then users can override it by setting that to false directly?

I don't think we have any consistency throughout the project in that regard yet. However, providing a Stylix option in this case has two main benefits:

  1. Additional functionality is more explicit to end-users without looking at the implementation.

  2. Added functionality may be arbitrarily large (more than just one <OPTION>.enable = stylix.targets.<TARGET>.enable; declaration) and can be toggled with one interface option. For example, the hyprpaper.enable option would toggle the following code block:

    lib.mkIf cfg.hyprpaper.enable {
    services.hyprpaper.enable = true;
    stylix.targets.hyprpaper.enable = true;
    }

@drishal
Copy link

drishal commented Sep 14, 2024

Couldn't we just use lib.mkDefault when setting services.hyprpaper.enable, then users can override it by setting that to false directly?

I don't think we have any consistency throughout the project in that regard yet. However, providing a Stylix option in this case has two main benefits:

1. Additional functionality is more explicit to end-users without looking at the implementation.

2. Added functionality may be arbitrarily large (more than just one `<OPTION>.enable = stylix.targets.<TARGET>.enable;` declaration) and can be toggled with one interface option. For example, the `hyprpaper.enable` option would toggle the following code block:
   https://github.com/danth/stylix/blob/c985b64469b0408eea990fe3c655cf7c010cf206/modules/hyprland/hm.nix#L42-L45

I think having services.hyprpaper.enable = true; might be a bad idea, since I would not want that to always autostart if I have other window managers installed

@trueNAHO
Copy link
Collaborator Author

I think having services.hyprpaper.enable = true; might be a bad idea, since I would not want that to always autostart if I have other window managers installed

This PR resolves this exact issue by providing an interface option to disable all associated hyprpaper behaviour. IIRC, this PR also closes #499.

@trueNAHO trueNAHO requested a review from danth September 23, 2024 11:46
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Oct 8, 2024
@trueNAHO trueNAHO force-pushed the hyprland-add-hyprpaper-enable-option branch from c985b64 to 85fc687 Compare October 8, 2024 13:53
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Oct 8, 2024
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Oct 8, 2024
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Oct 8, 2024
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Oct 8, 2024
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Oct 8, 2024
@trueNAHO trueNAHO force-pushed the hyprland-add-hyprpaper-enable-option branch from 85fc687 to 018db00 Compare October 8, 2024 13:57
@trueNAHO
Copy link
Collaborator Author

trueNAHO commented Oct 8, 2024

Changelog

v2: 40bc8db

  • Add Link: https://github.com/danth/stylix/pull/501 tag to every commit
  • Rebase on top of commit 63426a5 ("forge: init (forge: init #573)")

v1

  • I don't remember the specific changes made between v0 and v2. But they should be very minor.

v0: 2aa427c

trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Oct 8, 2024
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Oct 8, 2024
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Oct 8, 2024
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Oct 8, 2024
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Oct 8, 2024
@trueNAHO trueNAHO force-pushed the hyprland-add-hyprpaper-enable-option branch from 018db00 to 40bc8db Compare October 8, 2024 14:00
@trueNAHO
Copy link
Collaborator Author

@danth, IIRC, this PR is ready to merge.

trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Nov 7, 2024
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Nov 7, 2024
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Nov 7, 2024
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Nov 7, 2024
trueNAHO added a commit to trueNAHO/stylix that referenced this pull request Nov 7, 2024
@trueNAHO trueNAHO force-pushed the hyprland-add-hyprpaper-enable-option branch from 40bc8db to 7b5d7cc Compare November 7, 2024 16:20
@trueNAHO
Copy link
Collaborator Author

trueNAHO commented Nov 7, 2024

Changelog

v3: 7b5d7cc

v2: 40bc8db

  • Add Link: https://github.com/danth/stylix/pull/501 tag to every commit
  • Rebase on top of commit 63426a5 ("forge: init (forge: init #573)")

v1

  • I don't remember the specific changes made between v0 and v2. But they should be very minor.

v0: 2aa427c

@trueNAHO
Copy link
Collaborator Author

trueNAHO commented Nov 7, 2024

Merging v3 on top of #608 resolves the merge conflicts.

@trueNAHO
Copy link
Collaborator Author

trueNAHO commented Nov 7, 2024

Merging v3 on top of #608 resolves the merge conflicts.

#608 has been merged, making this PR merge-ready again.

Copy link
Owner

@danth danth left a comment

Choose a reason for hiding this comment

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

Looks good to me. Sorry for the delay, this got buried at the bottom of my notifications.

@trueNAHO trueNAHO force-pushed the hyprland-add-hyprpaper-enable-option branch from 7b5d7cc to c6547e2 Compare December 6, 2024 18:34
@trueNAHO
Copy link
Collaborator Author

trueNAHO commented Dec 6, 2024

Changelog

v4: c6547e2

v3: 7b5d7cc

v2: 40bc8db

  • Add Link: https://github.com/danth/stylix/pull/501 tag to every commit
  • Rebase on top of commit 63426a5 ("forge: init (forge: init #573)")

v1

  • I don't remember the specific changes made between v0 and v2. But they should be very minor.

v0: 2aa427c

@trueNAHO trueNAHO changed the title hyprland: add hyprpaper.enable option hyprland: add hyprpaper.enable option and improve code Dec 6, 2024
@trueNAHO trueNAHO merged commit e309d64 into danth:master Dec 6, 2024
12 checks passed
@trueNAHO
Copy link
Collaborator Author

trueNAHO commented Dec 6, 2024

@danth, should we merge future patchsets (a series of commits) as a merge commit, as in commit e309d64, or as fast-forward merges? FYI, I do not have permission to do fast-forward merges ("Rebase and merge") through the GitHub UI:

image

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.

Ability to stop Stylix from starting Hyprpaper service automatically
3 participants