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.

Sign up for GitHub

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/redshift: rework to use config file #50979

Closed
wants to merge 1 commit into from
Closed

nixos/redshift: rework to use config file #50979

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
120 changes: 62 additions & 58 deletions nixos/modules/services/x11/redshift.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
{ config, lib, pkgs, ... }:
{ config, lib, pkgs, options, ... }:

with lib;

Expand All @@ -7,6 +7,10 @@ let
cfg = config.services.redshift;
lcfg = config.location;

target = if (cfg.settings.redshift.adjustment-method or null) == "drm" then "basic.target" else "graphical-session.target";

generatedConfig = pkgs.writeText "redshift-generated.conf" (generators.toINI {} cfg.settings);

in {

imports = [
Expand All @@ -23,7 +27,14 @@ in {
throw "services.redshift.longitude is set to null, you can remove this"
else builtins.fromJSON value))
(mkRenamedOptionModule [ "services" "redshift" "provider" ] [ "location" "provider" ])
];
(mkRemovedOptionModule [ "services" "redshift" "extraOptions" ] "All Redshift configuration is now available through services.redshift.settings instead.")
] ++
(map (option: mkRenamedOptionModule ([ "services" "redshift" ] ++ option.old) [ "services" "redshift" "settings" "redshift" option.new ]) [
{ old = [ "temperature" "day" ]; new = "temp-day"; }
{ old = [ "temperature" "night" ]; new = "temp-night"; }
{ old = [ "brightness" "day" ]; new = "brightness-day"; }
{ old = [ "brightness" "night" ]; new = "brightness-night"; }
]);

options.services.redshift = {
enable = mkOption {
Expand All @@ -35,42 +46,41 @@ in {
'';
};

temperature = {
day = mkOption {
type = types.int;
default = 5500;
description = ''
Colour temperature to use during the day, between
<literal>1000</literal> and <literal>25000</literal> K.
'';
};
night = mkOption {
type = types.int;
default = 3700;
description = ''
Colour temperature to use at night, between
<literal>1000</literal> and <literal>25000</literal> K.
'';
};
settings = mkOption {
type = with types; attrsOf (attrsOf (nullOr (oneOf [ str float bool int ])));
Copy link
Member

Choose a reason for hiding this comment

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

toINI doesn't seem to support floats yet. And you should either remove nullOr or make sure null results in nothing being generated.

default = {};
description = ''
The configuration to pass to redshift.
See <command>man redshift</command> under the section
Copy link
Member

Choose a reason for hiding this comment

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

This is the nasty syntax for man pages: <citerefentry><refentrytitle>redshift</refentrytitle><manvolnum>1</manvolnum></citerefentry>

CONFIGURATION FILE for options.
'';
example = literalExample ''{
redshift = {
dawn-time = "05:00-06:00";
dusk-time = "22:00-23:00";
temp-night = 3000;
};
};'';
apply = c:
if !(c ? redshift.dawn-time || c ? redshift.dusk-time) && !(c ? redshift.location-provider) && options.locations.provider.isDefined then
c // {
redshift.location-provider = lcfg.provider;
Copy link
Member

Choose a reason for hiding this comment

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

This clears all other redshift.* keys, you need to use recursiveUpdate instead

Copy link
Member

Choose a reason for hiding this comment

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

And location.provider is always defined because it has a default, also c ? dawn-time == c ? dusk-time as per assertion, so this can be

c: if ! c ? redshift.dawn-time && ! c ? redshift.location-provider
  then recursiveUpdate c {
    redshift.location-provider = lcfg.provider;
  } else c

}
else
c;
};

brightness = {
day = mkOption {
type = types.str;
default = "1";
description = ''
Screen brightness to apply during the day,
between <literal>0.1</literal> and <literal>1.0</literal>.
'';
};
night = mkOption {
Copy link
Member

Choose a reason for hiding this comment

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

You would have to introduce backwards compatible aliases for these removed options with mkAliasOptionModule

Copy link
Contributor Author

@hyperfekt hyperfekt Nov 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I've chosen mkRenamedOptionModule in order to get people to migrate to the new configuration method in case the config file options change in a way that doesn't allow an alias in the future.

type = types.str;
default = "1";
description = ''
Screen brightness to apply during the night,
between <literal>0.1</literal> and <literal>1.0</literal>.
'';
};
configFile = mkOption {
type = types.path;
example = "~/.config/redshift/redshift.conf";
description = ''
Configuration file for redshift. It is recommended to use the
<option>settings</option> option instead.
</para>
<para>
Setting this option will override any configuration file auto-generated
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be disallowed by assertion? Allowing a settings object that's going to have no effect since it's overridden adds potential for confusion but little value, doesn't it?

through the <option>settings</option> option.
'';
};

package = mkOption {
Expand All @@ -82,18 +92,21 @@ in {
'';
};

extraOptions = mkOption {
type = types.listOf types.str;
default = [];
example = [ "-v" "-m randr" ];
description = ''
Additional command-line arguments to pass to
<command>redshift</command>.
'';
};
};

config = mkIf cfg.enable {
services.redshift.configFile = mkDefault generatedConfig;

assertions = mkIf (cfg.configFile == generatedConfig) [ {
assertion = (cfg.settings ? redshift.dawn-time) == (cfg.settings ? redshift.dusk-time);
Copy link
Member

Choose a reason for hiding this comment

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

No parens needed

message = "Time of dawn and time of dusk must be provided together.";
Copy link
Member

Choose a reason for hiding this comment

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

The assertion message should also mention that this is about redshift

} ];

services.redshift.settings.manual = {
lat = mkIf options.location.latitude.isDefined lcfg.latitude;
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be something like

let
  needsLocationProvider = ! cfg.settings ? redshift.dawn-time;
in mkIf (needsLocationProvider && lcfg.provider == "manual") {
  lat = lcfg.latitude;
  lon = lcfg.longitude;
}

Setting lcfg == "manual" should force the user to set longitude and latitude if it's needed. isDefined doesn't do that.

lon = mkIf options.location.longitude.isDefined lcfg.longitude;
};

# needed so that .desktop files are installed, which geoclue cares about
environment.systemPackages = [ cfg.package ];

Expand All @@ -102,23 +115,14 @@ in {
isSystem = true;
};

systemd.user.services.redshift =
let
providerString = if lcfg.provider == "manual"
then "${toString lcfg.latitude}:${toString lcfg.longitude}"
else lcfg.provider;
in
{
systemd.user.services.redshift = {
description = "Redshift colour temperature adjuster";
wantedBy = [ "graphical-session.target" ];
partOf = [ "graphical-session.target" ];
wantedBy = [ target ];
partOf = [ target ];
serviceConfig = {
ExecStart = ''
${cfg.package}/bin/redshift \
-l ${providerString} \
-t ${toString cfg.temperature.day}:${toString cfg.temperature.night} \
-b ${toString cfg.brightness.day}:${toString cfg.brightness.night} \
${lib.strings.concatStringsSep " " cfg.extraOptions}
-c ${cfg.configFile}
'';
RestartSec = 3;
Restart = "always";
Expand Down