danth / stylix

System-wide colorscheming and typography for NixOS
https://stylix.danth.me/
MIT License
904 stars 105 forks source link

hyprpaper: init #377

Closed edrobertsrayne closed 2 weeks ago

edrobertsrayne commented 1 month ago

Add hyprpaper styling using stylix.image as wallpaper.

danth commented 1 month ago

IMHO, it would be more structured to add this functionality under an options.stylix.targets.hyprland.wallpaper option, although we might change the wallpaper option name.

I agree, this should be combined with the hyprland module.

However, having a separate option for it would make it different to all other modules, which just use the stylix.image value directly.

edrobertsrayne commented 1 month ago

I agree, this should be combined with the hyprland module.

Something like this?

in {
  options.stylix.targets = {
    hyprland.enable = config.lib.stylix.mkEnableTarget "Hyprland" true;
    hyprpaper.enable = config.lib.stylix.mkEnableTarget "Hyprpaper" true;
  };

  config.wayland.windowManager.hyprland.settings =
    lib.mkIf config.stylix.targets.hyprland.enable settings;

  config.services.hyprpaper.settings = lib.mkIf config.stylix.targets.hyprpaper.enable {
    preload = ["${config.stylix.image}"];
    wallpaper = [",${config.stylix.image}"];
  };
}
danth commented 1 month ago

IMO, hyprpaper is less of a target itself, and more just the means of theming hyprland. So ideally they should use the same option to enable both.

edrobertsrayne commented 1 month ago

Your seemingly unrelated flake.lock changes from eb44e01 are causing build errors in my Home Manager setup, and have been reverted in e722720 to resolve this issues.

Thanks, I inadvertently added these when trying to get a local version of the repo to work in my NixOS flake.

7887ca8 removes the preload statement in attempt to reduce the running memory usage. Take a look at the commit message for more details. However, this causes Hyprpaper not to set the wallpaper anymore, as it is not loaded into memory before setting it. I assume this requires patching an unload option to Home Manager or Hyprpaper such that wallpapers are unloaded right after setting them.

@edrobertsrayne could you resolve this preload issue?

As far as I can tell, this is the expected way that hyprpaper works. You need to manually load the image into memory and then apply it. You can't set a wallpaper without using preload unless I've missed something. See hyprpaper wiki

trueNAHO commented 1 month ago

7887ca8 removes the preload statement in attempt to reduce the running memory usage. Take a look at the commit message for more details. However, this causes Hyprpaper not to set the wallpaper anymore, as it is not loaded into memory before setting it. I assume this requires patching an unload option to Home Manager or Hyprpaper such that wallpapers are unloaded right after setting them. @edrobertsrayne could you resolve this preload issue?

As far as I can tell, this is the expected way that hyprpaper works. You need to manually load the image into memory and then apply it. You can't set a wallpaper without using preload unless I've missed something. See hyprpaper wiki

Indeed. However, I was thinking about unloading the image after setting it, as it will not be set again. This would reduce the running memory usage.

edrobertsrayne commented 1 month ago

7887ca8 removes the preload statement in attempt to reduce the running memory usage. Take a look at the commit message for more details. However, this causes Hyprpaper not to set the wallpaper anymore, as it is not loaded into memory before setting it. I assume this requires patching an unload option to Home Manager or Hyprpaper such that wallpapers are unloaded right after setting them. @edrobertsrayne could you resolve this preload issue?

As far as I can tell, this is the expected way that hyprpaper works. You need to manually load the image into memory and then apply it. You can't set a wallpaper without using preload unless I've missed something. See hyprpaper wiki

Indeed. However, I was thinking about unloading the image after setting it, as it will not be set again. This would reduce the running memory usage.

I'm not sure you can do that, or at least should. The image has got to be in memory somewhere to be displayed and looking through the wiki it seems like the important option is unload unused. The unload command is a runtime command rather than a configuration option.

trueNAHO commented 1 month ago

7887ca8 removes the preload statement in attempt to reduce the running memory usage. Take a look at the commit message for more details. However, this causes Hyprpaper not to set the wallpaper anymore, as it is not loaded into memory before setting it. I assume this requires patching an unload option to Home Manager or Hyprpaper such that wallpapers are unloaded right after setting them.

I'm not sure you can do that, or at least should. The image has got to be in memory somewhere to be displayed and looking through the wiki it seems like the important option is unload unused. The unload command is a runtime command rather than a configuration option.

This should be addressed in a follow-up PR, if at all. Feel free to revert 7887ca8, so this feature can be merged.

I opened https://github.com/danth/stylix/issues/408, to investigate this issue further once I have more free time.

diniamo commented 4 weeks ago

IMO, hyprpaper is less of a target itself, and more just the means of theming hyprland. So ideally they should use the same option to enable both.

This is wrong, hyprpaper works perfectly well as a standalone service for all wlroots-based compositors. It should be a separate target.

danth commented 3 weeks ago

This is wrong, hyprpaper works perfectly well as a standalone service for all wlroots-based compositors. It should be a separate target.

Depends whether we're only setting it up when it's already been enabled, or if we're also installing it by default. If we removed the services.hyprpaper.enable = true line then I would support it being separate.

On the other hand, enabling the service automatically is helpful for Hyprland users who expect the wallpaper to "just work".

diniamo commented 3 weeks ago

Users who except things to just work will not be using nix and a compositor IMO.

Either way, at least have it as a separate target, and enable that in the Hyprland one.

diniamo commented 3 weeks ago

Me? This isn't my PR.

trueNAHO commented 3 weeks ago

Me? This isn't my PR.

My bad. I pinged the wrong person.