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

lxqt.pcmanfm-qt: fix default wallpaper #87623

Merged
merged 1 commit into from Jun 21, 2020

Conversation

wamserma
Copy link
Member

Motivation for this change

#70780

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.

@romildo
Copy link
Contributor

romildo commented May 13, 2020

This change sets the wallpaper to one with a NixOS brand.

What other distributions have done is make a new branding theme and set it as the default. Maybe we should follow them as soon as the panel/theme issue is fixed.

I am not sure about committing this change and would like to see other opinions.
cc @worldofpeace @jtojnar

@wamserma
Copy link
Member Author

Yes, setting a branded wallpaper is an opinionated decision.
The current default points at a non-existent wallpaper, sets a black background. Together with the default panel background, this is a bad UX.
Fixing the wallpaper is easy and a quick win, while the panel/theme issue linger around for some more time, I suspect.
Regarding the branded wallpaper, I was inspired by #86163 but I favour the blue wallpaper.

@wamserma
Copy link
Member Author

ping @worldofpeace @jtojnar

@jtojnar
Copy link
Contributor

jtojnar commented Jun 15, 2020

I am not sure apps should depend on system theme. Is there a way to set the background in a NixOS module rather than pcmanfm?

@wamserma
Copy link
Member Author

I updated the PR to just fix the path instead of using NixOS-artwork.
The main problem was that the template config is copied to user's home at first login, and without this change the Wallpaper would be set to a file in the Nix Store that is eventually GC'd (due to the hardcoded path). Now using the proper symlink from /run/current-system/sw....

@jtojnar Ok for you?

@wamserma
Copy link
Member Author

ping @romildo @jtojnar @worldofpeace

@jtojnar
Copy link
Contributor

jtojnar commented Jun 19, 2020

Looks like that should work, at least in LxQt.

The path is linked:

# Link some extra directories in /run/current-system/software/share
environment.pathsToLink = [ "/share" ];

And the package is installed unconditionally

I am not a fan of using this nexus but I guess it is fine until #47173 is resolved.

@wamserma
Copy link
Member Author

This is currently imho the best (and only without further indirection and hackery) way to have a stable path that always points to the currently active lxqt-themes package.

Also fixed a missing slash in this path.

@wamserma wamserma requested a review from romildo June 20, 2020 12:37
Copy link
Contributor

@romildo romildo left a comment

Choose a reason for hiding this comment

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

There are 3 commits. Reduce them to just one. Then I think it will be ready for merging.

@wamserma
Copy link
Member Author

There are 3 commits. Reduce them to just one. Then I think it will be ready for merging.

Yes. That was meant for easier review. Should be good for merging now.

@wamserma wamserma requested a review from romildo June 20, 2020 18:01
@romildo romildo merged commit 6a12c20 into NixOS:master Jun 21, 2020
@wamserma wamserma deleted the lxqt-fix-wallpaper branch June 21, 2020 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants