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

treewide: optionalize wallpaper functionality #442

Open
4 tasks
trueNAHO opened this issue Jun 19, 2024 · 5 comments · May be fixed by #717
Open
4 tasks

treewide: optionalize wallpaper functionality #442

trueNAHO opened this issue Jun 19, 2024 · 5 comments · May be fixed by #717
Labels
C-tracking-issue Category: A tracking issue feature A new feature or a feature request good first issue Good for newcomers

Comments

@trueNAHO
Copy link
Collaborator

trueNAHO commented Jun 19, 2024

About

This is a tracking issue for optionalizing wallpaper functionality, when supported.

Steps

Unresolved Questions

  • How to optionalize the wallpaper functionality?
@trueNAHO trueNAHO added feature A new feature or a feature request good first issue Good for newcomers C-tracking-issue Category: A tracking issue labels Jun 19, 2024
@danth
Copy link
Owner

danth commented Jun 19, 2024

So, an option like enableWallpaper under each target?

@trueNAHO
Copy link
Collaborator Author

trueNAHO commented Jun 19, 2024

So, an option like enableWallpaper under each target?

Yes, I had the same idea. However, I would simplify the option name to wallpaper.

For consistency, we could implement and use a helper function, like mkEnableWallpaper, with an appropriate description, similar to mkEnableTarget:

options.stylix.targets.hyprland = let
  target = "Hyprland";
in {
  enable = lib.stylix.mkEnableTarget target true;
  wallpaper = lib.stylix.mkEnableWallpaper target false;
}

@NovaViper
Copy link

Hey @trueNAHO would this be an option that can be set globally for stylix? I actually want to disable stylix's wallpaper functionality in favor of plasma-manager's wallpaper module as that module lets me set multiple wallpapers for KDE Plasma.

@compactcode
Copy link
Contributor

compactcode commented Jun 20, 2024

This is my current thinking which depends on #200

  • If sylix.image is set (currently mandatory) then we configure all enabled wallpaper services.
    • It shouldn't be the responsibility of sylix to decide (and maintain code for) which wallpaper service should be used.
  • If sylix.image is not set then we do not configure any wallpaper services.
    • If the user doesn't want wallpaper then this is the way.
    • If the user wants to do something custom (e.g. a folder with rotating images) they now have unobstructed control to configure their chosen service.

@trueNAHO
Copy link
Collaborator Author

trueNAHO commented Jun 23, 2024

would this be an option that can be set globally for stylix?

The options.stylix.targets.<TARGET>.wallpaper options would be <TARGET> specific, similar to options.stylix.targets.<TARGET>.enable.

Based on the following use cases, it makes sense to add a stylix.enableWallpaper option complementary to stylix.autoEnable:

  • Get color scheme from stylix.image without setting stylix.image as wallpaper: stylix = { enableWallpaper = false; image = image; }.

  • Get color scheme from stylix.image and set stylix.image as wallpaper: stylix = { enableWallpaper = true; image = image; }.

  • Manually declare color scheme (stylix.base16Scheme) and entirely ignore stylix.image: stylix = { base16Scheme = base16Scheme; enableWallpaper = false; image = null; }.

For reference, it has been considered to better rename the stylix.autoEnable option. Additionally, these use cases somewhat correspond to the slideshow feature suggestion.

I actually want to disable stylix's wallpaper functionality in favor of plasma-manager's wallpaper module as that module lets me set multiple wallpapers for KDE Plasma.

My previously mentioned use cases and an additional options.stylix.targets.kde.wallpaper would resolve this scenario. For reference, I added the kde module to the Steps required to resolve this tracking issue.

This is my current thinking which depends on #200

  • If sylix.image is set (currently mandatory) then we configure all enabled wallpaper services.
    • It shouldn't be the responsibility of sylix to decide (and maintain code for) which wallpaper service should be used.
  • If sylix.image is not set then we do not configure any wallpaper services.
    • If the user doesn't want wallpaper then this is the way.
    • If the user wants to do something custom (e.g. a folder with rotating images) they now have unobstructed control to configure their chosen service.

This corresponds to my previously mentioned use cases, except that stylix.enableWallpaper provides a default implementation for setting wallpapers:

  • It shouldn't be the responsibility of sylix to decide (and maintain code for) which wallpaper service should be used.

Regarding the implementation of stylix.enableWallpaper, we could add additional guards, similar to lib.stylix.mkEnableTarget:

default = cfg.enable && cfg.autoEnable && autoEnable;

Considering that the scope of wallpaper optionality is larger than initially anticipated, it might be better to extend the scope of this tracking issue. Consequently, #200 is included and the following statement is removed from this issue's top-level description:

Unlike #200, this does not optionalize the stylix.image declaration. Instead, it should be possible to disable wallpapers being set, when supported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: A tracking issue feature A new feature or a feature request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants