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

sway module: add configFile option #55931

Closed
wants to merge 3 commits into from
Closed

sway module: add configFile option #55931

wants to merge 3 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Feb 17, 2019

Motivation for this change
  1. Port configFile option from i3 module to sway and sway-beta modules
  2. If sway-beta with buildDocs = false don't pass -Dman-pages=enabled cmake flag

cc @primeos

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option 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/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@primeos
Copy link
Member

primeos commented Feb 17, 2019

To be honest I'm not sure if we really want the configFile option.

I definitely see some advantages but I'm not sure how many users would actually prefer to manage their Sway configuration via Nix (as root) instead of in their $HOME (and most users probably use $HOME ATM). And AFAIK some community members are currently trying to avoid unnecessary options.

But another problem I see is the implementation. All relevant parameters (e.g. --version and --help) still work, which is good, but (at least in theory) a user looses the ability to change/override his configuration. But on the other hand this is only a problem if someone actually uses configFile and not by default.

man sway currently states the following:

CONFIGURATION
       sway searches for a config file in the following locations, in this order:

       1.   ~/.sway/config
       2.   $XDG_CONFIG_HOME/sway/config (suggested location)
       3.   ~/.i3/config
       4.   $XDG_CONFIG_HOME/i3/config
       5.   /etc/sway/config
       6.   /etc/i3/config

       If unset, $XDG_CONFIG_HOME defaults to ~/.config.

       An error is raised when no config file is found. The recommended default configuration is usually installed to /etc/sway/config; you are encouraged to copy this to ~/.config/sway/config and edit it
       from there.

IMO it would be better if users that want to manage their configuration via Nix would simply use something like environment.etc."sway/config".text. But that's just my personal opinion.

I'm not necessarily against merging this but I'd like to get some feedback from other community members (e.g. Infinisil or Mic92) first, if you still want this merged.

The first commit is fine :)
The other two commits should use the nixos/<module> format (see CONTRIBUTING.md#submitting-changes) and I'd prefer pointing to something like $HOME/.config/sway/config (I don't like having a lot of configuration file in $HOME) but technically this isn't 100 % correct either (maybe we could just refer to $HOME or man sway).

@ghost
Copy link
Author

ghost commented Feb 17, 2019

@primeos good point, thanks! Could you please take only first commit then?

@primeos
Copy link
Member

primeos commented Feb 17, 2019

Okay, I've pushed it in e5a937b. Thanks :)

@primeos primeos closed this Feb 17, 2019
@ghost ghost deleted the sway branch February 17, 2019 18:15
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

2 participants