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

nixos/sway: set default wallpaper #122992

Closed
wants to merge 1 commit into from

Conversation

Synthetica9
Copy link
Member

@Synthetica9 Synthetica9 commented May 14, 2021

Motivation for this change

cc @primeos

See also: https://github.com/Synthetica9/nixpkgs/pull/new/sway-default-wallpaper

The wallpaper shows up in the test, but I don't know if this is what you had in mind.

sway_exit

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels May 14, 2021
@primeos
Copy link
Member

primeos commented May 14, 2021

The wallpaper shows up in the test, but I don't know if this is what you had in mind.

Yeah, unfortunately not quite, sorry about that... I've drafted #122995 with what I had in mind.

@Synthetica9 Synthetica9 force-pushed the sway-default-wallpaper branch from b77309a to 22e6693 Compare May 14, 2021 20:05
@Synthetica9 Synthetica9 force-pushed the sway-default-wallpaper branch from 22e6693 to 45978fa Compare May 14, 2021 20:36
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels May 14, 2021
@primeos primeos mentioned this pull request May 27, 2021
22 tasks
@stale
Copy link

stale bot commented Nov 16, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Nov 16, 2021
@Synthetica9
Copy link
Member Author

Still relevant I'd say

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Nov 16, 2021
Copy link
Member

@primeos primeos left a comment

Choose a reason for hiding this comment

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

We still have to decide if we want to go with this PR or #122995 (which was a bit of an unfortunate situation). IIRC I prefer my approach (duh) but that is certainly open for discussion (plus I have to take another look at it). However, I'd be nice if we could wait until the weekend (for my replies/opinion, the discussion can of course already continue).

@Synthetica9 I assume you prefer your approach, right?

@Synthetica9
Copy link
Member Author

Might be nice to install the Nix-themed or sway-themed wallpaper depending on whether we're running on NixOS or not, but I have no strong preference. Feel free to merge whichever one and close the other.

@primeos
Copy link
Member

primeos commented Nov 21, 2021

Might be nice to install the Nix-themed or sway-themed wallpaper depending on whether we're running on NixOS or not, but I have no strong preference.

Yeah, by default I'd prefer the upstream default from Sway though as I don't really like downstream patches/changes when avoidable. However, I don't know how other community members feel about this and unfortunately we don't have any formal guidelines (at least AFAIK).

Anyway, the implementation is fine but I don't really like this approach for the following reasons:

  • It adds an additional option to the NixOS module and IMO defaultWallpaper doesn't seem relevant enough (but who knows, maybe many users would like such an option to more easily refer to dynamic Nix store paths)
  • It hardcodes fill and always applies the wallpaper to all outputs (e.g. I use different wallpapers per monitor but that might be an uncommon approach - then again it e.g. makes a lot of sense if the monitors have different resolutions).
    • Note: This isn't an issue for the default but it makes the option less flexible/useful.
  • It links to the wallpaper via /etc/sway/wallpaper which doesn't feel right to me.
  • It might unexpectedly override user-defined wallpapers if they include /etc/sway/config.d/* (it's of course fixable by setting defaultWallpaper to null but I'd like to avoid that).

Feel free to merge whichever one and close the other.

@Synthetica9 so if you're fine with it I'd prefer to go with #122995. It's not perfect either but it seems good enough to me for now.

@primeos primeos closed this Nov 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants