Add NixOS module: xidlehook #76063
Add NixOS module: xidlehook #76063
Conversation
There was a problem hiding this comment.
Thanks! Haven't tested it yet (I don't use xautolock anymore since I switched to Wayland a while ago), but may be able to do this next week unless somebody is faster. Apart from the two, minor comments, some additional thoughts about this:
- I'd recommend to add an entry to the release-notes (those can be found in
nixos/doc/manual/release-notes/rl-2003.xml) to encourage users to switch. - This module needs to be included by adding it to
nixos/modules/module-list.nix. - There's a fairly simple test for
xautolock. Could you please adapt this one forxidlehook? - I don't think that we want to have the drop-in options forever. I haven't found a simple way to mark options as deprecated, but I'd mark those as deprecated (at least by noting this in the description).
chkno
force-pushed
the
nixos-module-xidlehook
branch
from
eb96a35
to
bb41cc6
Compare
chkno
force-pushed
the
nixos-module-xidlehook
branch
3 times, most recently
from
eb96a35
to
e2648a3
Compare
|
Release note: Done.
I don't want the xautolock-compatibility options to ever be more-deprecated than xautolock itself is; I want the path from xautolock -> xidlehook-with-compat-options -> xidlehook-trigger-list to be monotonically encouraging. Also, separately, I have a lot of sympathy for people being busy & not wanting to mess with config that is working for them, & thus I wouldn't mind the xautolock-compatibility options sticking around for many years. It's a bit weird that xautolock would leave a legacy imprint on xidlehook configuration even after it is long gone, but it doesn't seem terrible. ... Other than the seconds vs. minutes inconsistency in the threshold options. That part is terrible. But I feel so excited about making the path from xautolock to xidlehook as smooth as possible that I'm reluctant to introduce even the modest extra step of "Oh, now convert your notify threshold from seconds to fractional minutes (or convert your lock and kill times to seconds) so that they're all in consistent units." It makes sense to me to defer that churn to later -- maybe along with the removal of the xautolock module after its deprecation. (Responses to review comments are visible as separate commits here: https://github.com/chkno/nixpkgs/commits/nixos-module-xidlehook-review-responses ) Thanks! |
| assertion = builtins.substring 0 1 timer.command == "/"; | ||
| message = "Please specify canonical paths for `services.xserver.xidlehook.timers` commands"; | ||
| }) cfg.timers) ++ (map (timer: { | ||
| assertion = (timer.canceller or "") != "" -> builtins.substring 0 1 timer.command == "/"; |
There was a problem hiding this comment.
Shouldn't this be timer.canceller?
And this shouldn't be necessary if you set the type of canceller to nullOr path. Same for the other options, specifying the path type automatically ensures an initial /
There was a problem hiding this comment.
Shouldn't this be
timer.canceller?
Yes! Fixed. Thanks!
And this shouldn't be necessary if you set the type of
cancellertonullOr path. Same for the other options, specifying thepathtype automatically ensures an initial/
It's not a path, though. It's a command -- it's a path up to the first space, and then not. In addition to being misleading as documentation, this would break if the path type later gained additional validation like checking path name component length (chars between /s) or normalizations like replacing /./ with /
|
|
||
| killer = mkOption { | ||
| default = null; # default according to `man xautolock` is none | ||
| example = "${pkgs.systemd}/bin/systemctl suspend"; |
There was a problem hiding this comment.
Needs exampleText = "\${pkgs.systemd}/bin/systemctl suspend", same for other default's that have ${} in them
There was a problem hiding this comment.
What is exampleText?
I tried adding \ to the $s in these examples. They still didn't render well in the manual:
-Example: ''''${pkgs.libnotify}/bin/notify-send "Locking in 10 seconds"''
+Example: ''''${pkgs.libnotify}/bin/notify-send "Locking in ''${toString cfg.notify} seconds"''
So I switched them all to literalExample. Thanks!
There was a problem hiding this comment.
Ah sorry, I mixed it up with defaultText. Looking good now
| command = cfg.killer; | ||
| })); | ||
|
|
||
| environment.systemPackages = optional cfg.enable pkgs.xidlehook; |
There was a problem hiding this comment.
Is this needed in the system environment? E.g. do users need to call the binary themselves for some reason? If not this can be removed
There was a problem hiding this comment.
This is necessary to get xidlehook-client in $PATH, which is the equivalent of xscreensaver-command & is used for things like lock-now keyboard shortcuts.
| # be brought to the attention of the user. Just appending one to the other | ||
| # usually would not make sense and definitely should not be done implicitly and | ||
| # silently. | ||
| type = types.uniq (types.listOf (types.submodule { |
There was a problem hiding this comment.
In general, the list type should be avoided if possible. And in this case this can work well by just having to name the timers such that this can be attrsOf submodule. This then also won't clear timers specified with the compatibility options if users set them.
There was a problem hiding this comment.
It doesn't make sense to me to ever merge timer lists because the durations are relative -- inserting a timer entry delays all the timers after it. For example, if you wish screens to lock after 10 minutes of inactivity and then wish to insert something before this entry that happens after 3 minutes of inactivity, you must modify the screen-lock timer's duration from 10 minutes to 7 minutes, because it is specified relative to the previous timer.
Contemplating a hypothetical for just a moment: We could make the Nix configuration use absolute timer durations and convert these to relative durations to pass them to xidlehook. But this diverges from how xidlehook is normally used & would confuse users trying to move existing xidlehook configuration to NixOS. Also, it opens up the question of how to merge cancellers. If the Nth timer's action both undoes the previous timer's action and performs its own (as in upstream's xrandr-dimmer example) what happens when another timer is inserted between them? One could imagine providing a merge-friendly interface that does stack unwinding, automatically adding each timer's canceller onto the next timer's action such that timers could be inserted at any point and still have everything chained correctly. This is now mergable, but it is very different than the normal semantics of xidlehook.
Instead, I think it makes sense to treat one timer-list as one opaque value. If two timer-lists are specified, this should raise an error that can be resolved by choosing one of them with mkForce.
xautolock has been unmaintained since 2007. (xss-lock, the other contender, has been unmaintained since 2014.) xidlehook is the new deal. It oughta have at least as much support in NixOS as xautolock has. In the long run, it might make sense to deprecate the xautolock module in favor of this one.
Omitted: `nowlocker`, which has no counterpart in xidlehook, and was usually just set to the same value as `locker`.
chkno
force-pushed
the
nixos-module-xidlehook
branch
from
e2648a3
to
d7a0d87
Compare
| machine.start() | ||
| machine.wait_for_x() | ||
| machine.fail("pgrep xlock") | ||
| machine.sleep(120) |
There was a problem hiding this comment.
is it possible to reduce this time? (just to be sure Hydra won't timeout on this test in future)
|
Thank you for your contributions. This has been automatically marked as stale because it has had no activity for 180 days. If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity. Here are suggestions that might help resolve this more quickly:
|
stale
bot
added
the
2.status: stale
stale
bot
removed
the
2.status: stale
Motivation for this change
xautolock has been unmaintained since 2007 and has bugs. xidlehook is the new hotness. This module is intended to be a drop-in replacement for xautolock -- users can just replace all occurrences of
xautolockwithxidlehookin their configs & be switched over†.† (And delete
nowlocker, which has no counterpart, but was typically just the same thing aslocker.)Things done
sandboxinnix.confon non-NixOS linux)nix-shell -p nix-review --run "nix-review wip"./result/bin/)nix path-info -Sbefore and after)Notify maintainers
cc @WilliButz @Ma27