i3: add module #79

uvNikita · 2017-09-22T22:05:19Z

~~Note: this is work in progress PR, created merely to identify that the new module is being developed, and to prevent duplicating work.~~
~~While the code is currently in active development, comments, ideas, and feedback on the module API are highly appreciated. Be aware that commits might be amended and rebased in the process.~~

Progress:

uvNikita · 2017-10-18T20:15:51Z

I think it's pretty much ready for merge.

One of my goals was to make it possible to enable module via xsession.windowManagers.i3.enable = true to get working i3 with the default parameters. I also wanted to give people possibility to set i3.config = null and to configure module with existing text configuration using i3.configExtra parameter.

My personal configuration that uses almost all options: https://pastebin.com/QTUnEuBX.

Default values were taken from here: https://github.com/i3/i3/blob/next/etc/config.
i3 config documentation: https://i3wm.org/docs/userguide.html

@rycee if I understand correctly you are using xmonad, but maybe you look at this PR from a general perspective? I would appreciate any feedback. For now, I put the code in a separate xsession.wm.i3 submodule since I didn't want to pollute xsession module with extra stuff. Maybe we could switch to a new way of supporting window managers before merging this pull request (and I will switch form xsession.wm to xsession.windowManager)? It's almost a month has passed since we deprecated xsession.windowManager parameter which should have given users enough time to switch. What do you think?

rycee · 2017-10-18T22:26:01Z

modules/services/window-managers/i3.nix

+            titlebar = mkOption {
+              type = types.bool;
+              default = cfg.package != pkgs.i3-gaps;
+              defaultText = "cfg.package != pkgs.i3-gaps (titlebar should be disabled for i3-gaps)";


The "cfg" here isn't very understandable for a user reading the documentation. Unfortunately will need something more extensive like "xsession.wm.i3.package != pkgs.i3-gaps" (maybe even change "pkgs" to "nixpkgs"?).

Good point!

rycee · 2017-10-18T22:40:55Z

Wow! This is quite the module! 😄

I just had a very quick look right now. I'll try to look it over a bit more carefully tomorrow, although as you pointed out I'm an xmonad user so I'll avoid looking at the particulars of i3.

I'll then also add a news entry saying that the xsession.windowManager option as a string will be removed completely on October 31 (~1 month after the deprecation merge) . Is it OK for you if we delay the merge of this module until then?

uvNikita · 2017-10-19T06:55:01Z

Thank you!

Yes, I think we can wait until October 31, I think more people will likely to use home-manager if we will not break anything unexpectedly :)

Hopefully, some i3 users will also show-up to test this module in the meantime :)

pmiddend · 2017-10-19T08:22:28Z

I tested the module and was delighted to see that I was able to migrate 99% of my config to home-manager. Just one thing: I didn't find the "focus_follows_mouse" option in the i3 module. Is there a way to set this option (its on by default - I like it off).

Also, it (auto-)generates font pango:pango:monospace 8, isn't that one pango too many? ;)

uvNikita · 2017-10-19T09:02:07Z

Thank you for testing it out @pmiddend ! Let me know if you think some option names or submodules structure is confusing, we can still discuss it and change while it's not merged yet.

Good point about the default font; fixed that. I also added i3.focus submodule where you can set followMouse, forceWrapping and newWindow parameters.

pmiddend · 2017-10-19T09:45:35Z

Works like a charm, thanks!

rycee · 2017-10-21T13:52:42Z

modules/services/window-managers/i3.nix

+              defaultText = "xsession.wm.i3.package != nixpkgs.i3-gaps (titlebar should be disabled for i3-gaps)";
+              description = "Whether to show window titlebars.";
+            };
+            border = mkOption {


Generally I think it's nice to have an empty line between options. You have such separators above but not below.

rycee · 2017-10-21T15:12:10Z

I think the module is just fine. The single concern I may raise is that the number of options is so large that it might make the module cumbersome to maintain and the documentation cluttered. But if you are willing to maintain it then I think it is OK to merge :-)

uvNikita · 2017-10-22T10:42:10Z

I see your point about a number of options and cluttered documentation. The reason I wrote the module in this way is that for my use-case it is very convenient to have this level of flexibility. At the same time, I was also trying to make this module useful for simpler cases. Since I use i3 on daily bases, I don't mind to maintain this module.

One way I see to simplify the module and significantly reduce the documentation size (by 30%) is to change i3.config.colors module to this:

colors = mkOption {
  type = types.submodule {
    options = {
      background = mkOption {
        type = types.string;
        default = "#ffffff";
        description = ''
          Background color of the window.
          Only applications which do not cover the whole area expose the color.
        '';
      };

      window = mkOption {
        type = types.attrsOf colorSetModule;
        default = {
          focused = { ... };
          focusedInactive = { ... };
          unfocused = { ... };
          urgent = { ... };
          placeholder = { ... };
        };
        description = "Color settings for different states of windows";
      };
    };
  };
  default = {};
  description = "Colors settings.";
};

I see two downsides to this approach:

We will lose a compile-time check of the attribute names (like focused, urgent etc).
And possibility for users to specify only the groups that they want to change (like in my config above) and get other values set to defaults. I'm not sure if there is any way to preserve this behavior though. E.g. if we could somehow merge default values with the once specified by users.

I'm personally fine with both of the downsides if you think this would improve the module @rycee.

rycee · 2017-10-22T13:06:13Z

About the colors, maybe could do something similar to the vim module? I.e., set visible = false on the options in colorSetModule and describe the color options in, e.g., colors.window.focused and in the other options say something like "See colors.window.focused for configuration options".

uvNikita · 2017-10-22T18:31:29Z

I made colorSetModule fields invisible as you suggested and added more documentation to the color submodule describing how to use it. Thank you for the input!

uvNikita · 2017-10-31T12:44:52Z

I squashed commits to prepare for the rebase. Note that the module still using xsession.wm path. Once xmonad will be moved to its own submodule and xsession.windowManager will be removed, I will modify this PR. I could create PR for the xmonad migration, but since I don't use it, I can't test if it will work as should after changes.

rycee · 2017-10-31T12:46:51Z

No worries. I'll switch over the windowManager option shortly and will take care of xmonad at the same time. I'll ping in this PR once it's done :-)

uvNikita · 2017-10-31T12:50:43Z

Thank you! :-)

rycee · 2017-10-31T13:12:23Z

@uvNikita Ok, now it should be sorted.

uvNikita · 2017-10-31T14:23:35Z

Great! I now modified the commit to use xsession.windowManager path.

rycee · 2017-10-31T14:30:43Z

Looks good. Feel free to merge whenever you are happy with it.

uvNikita · 2017-10-31T15:12:14Z

Rebased into master 467b774. Thanks to all who participated in the PR!

rycee · 2017-10-31T16:15:54Z

Wonderful work! Thanks for the patience in getting this in 👍

GaetanLepage · 2023-01-06T09:14:09Z

Hello @uvNikita
What is the reason for the initialization of default_border and default_floating_border ?
Why shouldn't it be normal when using i3-gaps ?

I ask this because I am currently adapting the xsession.windowManager.i3 module to the recent merge of i3-gaps and i3.
Because now there is no more package distinction, what should be the default value ?

uvNikita · 2023-01-06T09:31:13Z

@GaetanLepage It's been awhile, I don't really remember why titlebar had to be disabled for i3-gaps...
I vaguely remember that, at the time of writing the module, title bars didn't play well with gaps.
If it has been fixed, it should probably be set to the default value of i3 itself.

uvNikita · 2023-01-06T09:36:52Z

OK, I found it, see comment here: #3563 (comment)

GaetanLepage · 2023-01-06T09:37:25Z

All right ! Thank you for the quick answer :)
I realize that with the current way the module is written, it is not possible to set the default_[foating_]border options to all possible settings described in i3 documentation.
The value can be either normal or pixel but not none or pixel 3 or normal 42.

uvNikita · 2023-01-06T09:40:29Z

I think pixel 3 should be possible to achieve with

window = {
  titlebar = false;
  border = 3;
}

And normal 42 with

window = {
  titlebar = true;
  border = 42;
}

GaetanLepage · 2023-01-06T09:44:21Z

You are right, I had missed those :)

uvNikita · 2023-01-06T09:45:35Z

As for the none, according to the documentation:

# The same as default_border none
default_border pixel 0

So I think that's the reason of not including none into options.

uvNikita mentioned this pull request Sep 24, 2017

Add more documentation about xsession #76

Closed

uvNikita force-pushed the add/i3 branch 11 times, most recently from c32091d to 1a63dae Compare October 18, 2017 18:48

uvNikita changed the title ~~[WIP] i3: add module~~ i3: add module Oct 18, 2017

uvNikita force-pushed the add/i3 branch 5 times, most recently from 51aae58 to cfd631b Compare October 18, 2017 20:11

rycee reviewed Oct 18, 2017

View reviewed changes

uvNikita force-pushed the add/i3 branch from cfd631b to 6d012d4 Compare October 19, 2017 06:48

rycee reviewed Oct 21, 2017

View reviewed changes

uvNikita force-pushed the add/i3 branch from 32c1a1a to e611e95 Compare October 31, 2017 12:38

uvNikita force-pushed the add/i3 branch from e611e95 to 7f66fab Compare October 31, 2017 14:20

i3: add module

7e8c43b

uvNikita force-pushed the add/i3 branch from 7f66fab to 7e8c43b Compare October 31, 2017 14:23

uvNikita closed this Oct 31, 2017

uvNikita deleted the add/i3 branch October 31, 2017 15:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

i3: add module #79

i3: add module #79

uvNikita commented Sep 22, 2017 •

edited

Loading

uvNikita commented Oct 18, 2017

rycee Oct 18, 2017

uvNikita Oct 19, 2017

rycee commented Oct 18, 2017 •

edited

Loading

uvNikita commented Oct 19, 2017

pmiddend commented Oct 19, 2017 •

edited

Loading

uvNikita commented Oct 19, 2017

pmiddend commented Oct 19, 2017

rycee Oct 21, 2017

rycee commented Oct 21, 2017

uvNikita commented Oct 22, 2017

rycee commented Oct 22, 2017

uvNikita commented Oct 22, 2017

uvNikita commented Oct 31, 2017

rycee commented Oct 31, 2017

uvNikita commented Oct 31, 2017

rycee commented Oct 31, 2017

uvNikita commented Oct 31, 2017

rycee commented Oct 31, 2017

uvNikita commented Oct 31, 2017

rycee commented Oct 31, 2017

GaetanLepage commented Jan 6, 2023 •

edited

Loading

uvNikita commented Jan 6, 2023

uvNikita commented Jan 6, 2023 •

edited

Loading

GaetanLepage commented Jan 6, 2023

uvNikita commented Jan 6, 2023 •

edited

Loading

GaetanLepage commented Jan 6, 2023

uvNikita commented Jan 6, 2023 •

edited

Loading

i3: add module #79

i3: add module #79

Conversation

uvNikita commented Sep 22, 2017 • edited Loading

uvNikita commented Oct 18, 2017

rycee Oct 18, 2017

Choose a reason for hiding this comment

uvNikita Oct 19, 2017

Choose a reason for hiding this comment

rycee commented Oct 18, 2017 • edited Loading

uvNikita commented Oct 19, 2017

pmiddend commented Oct 19, 2017 • edited Loading

uvNikita commented Oct 19, 2017

pmiddend commented Oct 19, 2017

rycee Oct 21, 2017

Choose a reason for hiding this comment

rycee commented Oct 21, 2017

uvNikita commented Oct 22, 2017

rycee commented Oct 22, 2017

uvNikita commented Oct 22, 2017

uvNikita commented Oct 31, 2017

rycee commented Oct 31, 2017

uvNikita commented Oct 31, 2017

rycee commented Oct 31, 2017

uvNikita commented Oct 31, 2017

rycee commented Oct 31, 2017

uvNikita commented Oct 31, 2017

rycee commented Oct 31, 2017

GaetanLepage commented Jan 6, 2023 • edited Loading

uvNikita commented Jan 6, 2023

uvNikita commented Jan 6, 2023 • edited Loading

GaetanLepage commented Jan 6, 2023

uvNikita commented Jan 6, 2023 • edited Loading

GaetanLepage commented Jan 6, 2023

uvNikita commented Jan 6, 2023 • edited Loading

uvNikita commented Sep 22, 2017 •

edited

Loading

rycee commented Oct 18, 2017 •

edited

Loading

pmiddend commented Oct 19, 2017 •

edited

Loading

GaetanLepage commented Jan 6, 2023 •

edited

Loading

uvNikita commented Jan 6, 2023 •

edited

Loading

uvNikita commented Jan 6, 2023 •

edited

Loading

uvNikita commented Jan 6, 2023 •

edited

Loading