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

Jump to bottom

nixos/rootston: Use an attribute set and the new keyboard configuration format #38401

Closed
wants to merge 2 commits into from
Closed

nixos/rootston: Use an attribute set and the new keyboard configuration format #38401

wants to merge 2 commits into from

Conversation

primeos
Copy link
Member

@primeos primeos commented Apr 3, 2018 •
edited

Motivation for this change

Rootston has a new preferred configuration format for the keyboard. E.g. with sway it works with environment variables like this:

export XKB_DEFAULT_LAYOUT=de,us
export XKB_DEFAULT_VARIANT=nodeadkeys
export XKB_DEFAULT_OPTIONS=grp:alt_shift_toggle,caps:escape

But the new preferred way is via the INI configuration file:

[keyboard]
layout=de,us
variant=nodeadkeys
options=grp:alt_shift_toggle,caps:escape

Because the keyboard section is already specified in the default configuration:

[keyboard]
meta-key=Logo

It probably makes sense to use an attrset instead. As a result config and extraConfig can be merged automatically and converted to the INI configuration format.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@gnidorah: Do you have an opinion on this approach? It makes it possible to extend and override the default configuration with extraConfig but it is probably a bit confusing (attrset vs INI), doesn't support comments (only within the Nix code), and doesn't look as nice in the configuration.nix man page (lib.generators.toPretty doesn't really help either).

@primeos
nixos/rootston: Use an attrset for merging of configuration options
f3e5375 
This change is not backwards compatible, but rootston isn't intended to
be used as a "real" window manager (i.e. it shouldn't break anyone's
setup).
@primeos primeos self-assigned this Apr 3, 2018
@primeos
Copy link
Member Author

primeos commented Apr 3, 2018 •
edited

cc @Profpatsch In case you're interested (but it's not really an important module) / want to give me some feedback.

ghost
ghost approved these changes Apr 3, 2018
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Thanks! Yes I see it's non ideal, but overall is neater and also a new stuff to learn for me :D

Profpatsch
Profpatsch reviewed Apr 4, 2018
View reviewed changes
nixos/modules/programs/rootston.nix
description = ''
Default configuration for rootston (used when called without any
parameters).
'';
};

extraConfig = mkOption {
type = types.attrs;
Copy link
Member

Choose a reason for hiding this comment

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

toINI only accepts attrsets of attrsets of any, anything else will make it crash iirc. So this should really be attrsOf (attrsOf undefined).

Copy link
Member

Choose a reason for hiding this comment

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

Or even better would be an ini type that only accepts stuff toINI also accepts, but I haven’t thought that through.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Profpatsch Thank you very much for your review and apologies that I had to close this PR.

@primeos
Copy link
Member Author

primeos commented Apr 6, 2018

Closing this PR due to 9db699e and the linked issue. It was probably a bad idea from me to add this module in the first place (I thought it would be useful, but upstream stated that rootston isn't supposed to be packaged and we don't even have a module for Weston - I'll eventually remove this module). - Apologies

@primeos primeos closed this Apr 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Profpatsch Profpatsch Profpatsch left review comments

Assignees

@primeos primeos

Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@primeos @Profpatsch @GrahamcOfBorg