Don't set background to black if ~/.background-image not present #78346

infinisil · 2020-01-23T02:32:14Z

Motivation for this change

In #49492 @mkaito mentions their usecase of running nitrogen --restore to set the background image, which could be implemented with e.g. services.xserver.windowManager.i3.extraSessionCommands, but only if the code for ~/.background-image wouldn't run after that, which sets a black background, which was done because of #25101.

This PR gets rid of setting the background to black again, which I think was the wrong solution to that problem.

This can be tested with a configuration.nix like

{ config, pkgs, ... }: {
  services.xserver = {
    enable = true;
    displayManager.auto = {
      enable = true;
      user = "demo";
    };
    windowManager.i3 = {
      enable = true;
      extraSessionCommands = ''
        ${pkgs.feh}/bin/feh --bg-max ${config.boot.loader.grub.splashImage}
      '';
    };
  };

  users.users.demo = {
    initialPassword = "";
    isNormalUser = true;
  };
}

which when ran with

$ nix-build nixos --arg configuration ./configuration.nix -A vm
$ result/bin/run-nixos-vm

results in the background image being set:

which wasn't possible like this previously

Ping @mkaito @worldofpeace @jtojnar @hedning

Note: The first commit is just a cleanup

This value isn't used since 71a8dbb

worldofpeace · 2020-01-23T02:51:26Z

nixos/modules/services/x11/desktop-managers/default.nix

+              ${pkgs.feh}/bin/feh --bg-${cfg.wallpaper.mode} ${optionalString cfg.wallpaper.combineScreens "--no-xinerama"} $HOME/.background-image
+            else
+              # Use a solid black background as fallback
+              ${pkgs.xorg.xsetroot}/bin/xsetroot -solid black


I don't think this should have a fallback code path, in other desktop environments I noticed it being a solid black wallpaper when it should be the texture used from the window manager. Feels extra.

Yeah probably just removing that is a better solution. I'm mainly wondering then why it's there in the first place.

git blame gave me the answer: #25101

I'm actually wondering why I ran into that in the first place.
It happened with desktopManagers that didn't use services.xserver.desktopManager.session to have the bgSupport attribute.

So yeah, I think that PR was the wrong solution to that problem. And I can't reproduce the issue from that PR. Using the current version of the PR without the fallback branch and the following config:

{ services.xserver = { enable = true; displayManager.sddm.enable = true; windowManager.i3.enable = true; }; users.users.demo = { initialPassword = ""; isNormalUser = true; }; }

I get the following after logging in:

Hmm, maybe i3 started resetting the root window but I am pretty sure I encountered this issue in some DM/WM combination when testing #53843

It was LightDM & XMonad: #53843 (comment)

Previously if ~/.background-image wasn't present, the background would be set to black, which would override what the user could set in e.g. services.xserver.windowManager.i3.extraSessionCommands

infinisil · 2020-01-26T19:55:17Z

@primeos Could you test this PR to see if the issue you had in #25101 still occurs with it?

primeos · 2020-01-27T13:00:33Z

@infinisil unfortunately not, I switched to Sway and don't have my previous SDDM + i3 configuration anymore (I could create a new test setup of course, but since you've already tested it with SDDM + i3 it shouldn't add any value / give different results).

I also assume that i3 started resetting the root window in the meantime, like @jtojnar mentioned, which sounds like a good idea in general.

And I agree that defaulting to a black background was just another hack / incomplete solution. However, I'm also not sure if extraSessionCommands is really the right place to configure the background (IIRC these commands are executed before the WM). In general I also don't like the extra logic for ~/.background-image in general (IIRC undocumented, not very flexible, hardcoded path, etc), but that's another discussion.

Anyway, I don't have any objections and this PR LGTM (it might still cause a few regressions that result in something similar to #25101, but a black background is also not always optimal).

infinisil · 2020-01-27T13:56:02Z

Cool, thanks. I think the most important thing here is that the default behavior is to do nothing. So if the user doesn't have a .background-image, NixOS doesn't do anything special.

mkaito

Looks good!

…S#78346) Don't set background to black if ~/.background-image not present

nixos-discourse · 2020-02-05T17:07:24Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/january-2020-in-nixos/5771/1

jluttine · 2020-02-07T12:30:04Z

What is the correct way to set the background to black now? (Using i3 with SDDM, but it's my own i3 desktop environment definition.)

EDIT: What happens now is that the background is the SDDM login view how it was left when I logged in...

jluttine · 2020-02-07T12:36:54Z

I suppose running this explicitly?

${pkgs.xorg.xsetroot}/bin/xsetroot -solid black

primeos · 2020-02-07T13:04:59Z

@jluttine do you still see the behaviour described in #25101 (SDDM login screen as background) or is there a different issue now?

Regarding the fix: Yes, running xsetroot -solid black should be the best solution IMO (man i3 seems to suggest setting this in ~/.xsession - therefore setting it in extraSessionCommands should be fine as well (and as exec in the i3 config file it should of course also be fine)).

jluttine · 2020-02-07T13:20:18Z

@primeos yeah, that looks like the issue I had after this PR was merged. But indeed using xsetroot explicitly fixes that. 👍

primeos · 2020-02-07T13:37:30Z

Ok, thanks for the update :) Then this behaviour might be hardware/driver dependant :o

Not sure what we should do in this case / how many users are affected.
i3 can probably do nothing about this (as that should break ~/.xsession scripts and our extraSessionCommands without extra hacks) and the only thing we can do is probably to add ${pkgs.xorg.xsetroot}/bin/xsetroot -solid black to nixos/modules/services/x11/window-managers/i3.nix right before ${cfg.extraSessionCommands} (but this would only cover i3 and kinda restore the previous behaviour with the exception that it now runs before extraSessionCommands but it might still run after ~/.xsession (haven't looked into this)).

Getting the background color/image correctly configured has been a bit tricky due to the interactions of the display manager, compton, and NixOS. NixOS is doing the following. If ~/.background-image exists, it uses feh to set that as the background, otherwise, it uses xsetroot to set the root X window's background to black: xsetroot -solid black This happens _after_ displayManager.sessionCommands have been run, clobbering any color that was previously set. A fix for this has been merged upstream: NixOS/nixpkgs#78346 Also relevant is a newly introduced option: services.xserver.windowManager.i3.extraSessionCommands NixOS/nixpkgs#49492 (comment) In the meantime, my workaround has been to create an image with the color I want and copy it to ~/.background-image. While trying to figure out how to get conky to work in i3, I learned that compton doesn't play nice with xsetroot. I haven't dug into too much detail, but it behaves as if compton introduces another layer of background that hides the layer xsetroot touches. Instead, using hsetroot has the desired affect.

nixos/desktop-managers: Remove needBGPackages thing

7a7b8d9

This value isn't used since 71a8dbb

infinisil mentioned this pull request Jan 23, 2020

display-manager: allow executable background #49492

Closed

9 tasks

ofborg bot added 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 labels Jan 23, 2020

worldofpeace reviewed Jan 23, 2020

View reviewed changes

nixos/desktop-manager: Don't fall back to black background

78d8365

Previously if ~/.background-image wasn't present, the background would be set to black, which would override what the user could set in e.g. services.xserver.windowManager.i3.extraSessionCommands

infinisil force-pushed the background-image branch from 1cbaf4d to 78d8365 Compare January 23, 2020 04:30

ofborg bot added 10.rebuild-linux: 0 and removed 10.rebuild-linux: 1-10 labels Jan 23, 2020

infinisil changed the title ~~Allow turning off ~/.background-image~~ Don't set background to black if ~/.background-image not present Jan 26, 2020

infinisil requested a review from mkaito January 27, 2020 23:14

mkaito approved these changes Jan 28, 2020

View reviewed changes

infinisil merged commit 766b788 into NixOS:master Jan 28, 2020

infinisil deleted the background-image branch January 28, 2020 15:42

anna328p pushed a commit to anna328p/nixpkgs that referenced this pull request Feb 2, 2020

Don't set background to black if ~/.background-image not prese… (NixO…

82365e8

…S#78346) Don't set background to black if ~/.background-image not present

primeos mentioned this pull request Feb 13, 2020

lightdm-gtk-greeter background stays in i3 session #80040

Open

jtojnar mentioned this pull request May 9, 2020

After upgrade to NixOS 20.03, desktop session "none+xmonad" does not reset root window #86338

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't set background to black if ~/.background-image not present #78346

Don't set background to black if ~/.background-image not present #78346

infinisil commented Jan 23, 2020 •

edited

Loading

worldofpeace Jan 23, 2020

infinisil Jan 23, 2020

infinisil Jan 23, 2020

worldofpeace Jan 23, 2020

infinisil Jan 23, 2020

jtojnar Jan 23, 2020

jtojnar Jan 23, 2020

infinisil commented Jan 26, 2020

primeos commented Jan 27, 2020

infinisil commented Jan 27, 2020

mkaito left a comment

nixos-discourse commented Feb 5, 2020

jluttine commented Feb 7, 2020 •

edited

Loading

jluttine commented Feb 7, 2020

primeos commented Feb 7, 2020

jluttine commented Feb 7, 2020

primeos commented Feb 7, 2020

Don't set background to black if ~/.background-image not present #78346

Don't set background to black if ~/.background-image not present #78346

Conversation

infinisil commented Jan 23, 2020 • edited Loading

Motivation for this change

worldofpeace Jan 23, 2020

Choose a reason for hiding this comment

infinisil Jan 23, 2020

Choose a reason for hiding this comment

infinisil Jan 23, 2020

Choose a reason for hiding this comment

worldofpeace Jan 23, 2020

Choose a reason for hiding this comment

infinisil Jan 23, 2020

Choose a reason for hiding this comment

jtojnar Jan 23, 2020

Choose a reason for hiding this comment

jtojnar Jan 23, 2020

Choose a reason for hiding this comment

infinisil commented Jan 26, 2020

primeos commented Jan 27, 2020

infinisil commented Jan 27, 2020

mkaito left a comment

Choose a reason for hiding this comment

nixos-discourse commented Feb 5, 2020

jluttine commented Feb 7, 2020 • edited Loading

jluttine commented Feb 7, 2020

primeos commented Feb 7, 2020

jluttine commented Feb 7, 2020

primeos commented Feb 7, 2020

infinisil commented Jan 23, 2020 •

edited

Loading

jluttine commented Feb 7, 2020 •

edited

Loading