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

i3 module: refactor #20385

Merged
merged 1 commit into from
Nov 23, 2016
Merged

i3 module: refactor #20385

merged 1 commit into from
Nov 23, 2016

Conversation

ericsagnes
Copy link
Contributor

Motivation for this change

Make i3 module simpler.

cc @fmthoma

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
    • Linux
  • 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.

@mention-bot
Copy link

@ericsagnes, thanks for your PR! By analyzing the history of the files in this pull request, we identified @fmthoma, @obadz and @aszlig to be potential reviewers.

@fmthoma
Copy link
Contributor

fmthoma commented Nov 15, 2016

@ericsagnes Although this change simplifies the code a bit, it does not simplify things from a user perspective. The original intention for having a dedicated i3-gaps window manager was for people who know the fork by its name to be able to use it in NixOS without any additional documentation required. With your change, however, this is not true any longer. See also the original pull request introducing i3-gaps (#15917) where we had a similar discussion.

Is there any possibility to have your refactoring without changing the user interface?

@ericsagnes
Copy link
Contributor Author

The original intention for having a dedicated i3-gaps window manager was for people who know the fork by its name to be able to use it in NixOS without any additional documentation required.

As a personal preference, I think having extra documentation and using a package option that is consistent with other modules, is better than having a 2 modules in a single file with non-standard module code.

Non-standard module code is bad for maintainance, and two modules in a single file force the modules to share the same meta information and can lead to maintainership issues, so it should be avoided.

I understand the point of view of making things user friendly, but as i3-gaps is more a fork than a different window manager, it feels more natural to have it as option to the i3 module than as an entirely new window manager module.
If we start to add modules for every variation of window managers like i3-gaps or bspwm-unstable, things will quickly become hard to manage.

Adding some documentation, or an option such as windowManager.i3.enable-gaps to use the i3-gaps package feel like a good compromise between user-friendliness and modularity.

Is there any possibility to have your refactoring without changing the user interface?

It is possible to make alias options, or option redirections but I am not sure it is worth as a close term goal is to make window managers use extensible option types, so the main interface will change anyways.

Note: This was not clearly stated in the description, but this PR is a cleanup in preparation to use extensible option types for window managers. (replacing every windowManager.foo.enable in favor of a single enum option shared between all the window manager modules)

@fmthoma
Copy link
Contributor

fmthoma commented Nov 16, 2016

Non-standard module code is bad for maintainance, and two modules in a single file force the modules to share the same meta information and can lead to maintainership issues, so it should be avoided.

I agree with you here.

Note: This was not clearly stated in the description, but this PR is a cleanup in preparation to use extensible option types for window managers. (replacing every windowManager.foo.enable in favor of a single enum option shared between all the window manager modules)

Can you give an example of a typical configuration after this change?

@fmthoma
Copy link
Contributor

fmthoma commented Nov 16, 2016

I just had a look at #20384 where you do a similar thing for bspwm-unstable. With the windowManager.foo.package option being (or becoming) a »standard« option, this change makes sense to me.

I would wish for some documentation along the lines of »some window managers allow you to use a custom version of the package, like package = bspwm-unstable for bspwm or package = i3-gaps for i3«, e.g. here: https://nixos.org/nixos/manual/index.html#sec-x11.

By the way: The Wiki page (https://nixos.org/wiki/I3_Window_Manager) seems to be outdated; is there a place where this content is going to be (or already has been) migrated?

@ericsagnes
Copy link
Contributor Author

Can you give an example of a typical configuration after this change?

It is not yet merged, but there is a PR for display managers to use this. #20271
There is no consensus at the moment for what the option name should be, but for window managers this should become something like the following.

{
  services.xserver.windowManager.enable = [ "i3" "xmonad" ];
}

I just had a look at #20384 where you do a similar thing for bspwm-unstable. With the windowManager.foo.package option being (or becoming) a »standard« option, this change makes sense to me.

The package option is quite widely used through the modules, not a lot in window managers, but awesome is using it (services.xserver.windowManager.awesome.package).
hardware.pulseaudio.package, programs.ssh.package, services.mongodb.package are a few examples of other usages.

By the way: The Wiki page (https://nixos.org/wiki/I3_Window_Manager) seems to be outdated; is there a place where this content is going to be (or already has been) migrated?

There was a dedicated issue for this page (#13269), but it was decided that the content in that wiki page was outdated and not relevant anymore.
Depending on the documentation size, options description or the module meta.doc are the best place to document a module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants