i3: remove i3/i3-gaps distinction

[GaetanLepage]

Description

i3 4.22 has been released. The main change is the merge of i3-gaps features.
This (now merged) PR brings this update to nixpkgs/NixOS.

Hence, we should update the services.window-managers.i3 module by removing all the "i3 vs i3-gaps" distinctions.

My only interrogation is the default value of the i3.config.floating.titlebar and i3.config.window.titlebar options.
Before, there were set to false by default when using i3-gaps. The i3 documentation suggests that setting them to true is still valid so this is what I have done... TO BE TESTED
I didn't updated the test cases in consequence. Hence, some are failing because of this change of default value.

Checklist

• Change is backwards compatible.

• Code formatted with ./format.

• Code tested through nix-shell --pure tests -A run.all.

• Test cases updated/added. See example.

• Commit messages are formatted like

  {component}: {description}
  
  {long description}
  

  See CONTRIBUTING for more information and recent commit messages for examples.

• If this PR adds a new module

  • Added myself as module maintainer. See example.

  • Added myself and the module files to .github/CODEOWNERS.

[teto]

I can test once you fix the test

[GaetanLepage]

I can test once you fix the test

Thank you @teto for your proposition. Actually, I would like to be sure about what would be the best default to set.
Is there a technical reason to not use default_[floating_]border normal when using gaps ?
Or is it more of a sensible default ?

My guess would be to either set what i3 is defaulting to (i.e. normal) or set the option default value to null and in this case not write anything in the config.

I am not an i3 user anymore. It would be great to have feedback from actual experienced users :)

[uvNikita]

I think I found the reason for these defaults on i3-gaps: https://github.com/Airblader/i3/blob/gaps-next/README.md#gaps

Note: In order to use gaps you need to disable window titlebars. This can be done by adding the following line to your config.

# You can also use any non-zero value if you'd like to have a border
default_border pixel 0

[GaetanLepage]

Thank you for finding the reference !
I don't know what is the current situation since the 4.22 release... I will change the tests to let @teto test the behavior.

[GaetanLepage]

Actually, the sway tests are failing, because initially, the default_[floating_]border options were set by default to:

• true if the package was i3
• false if the package was i3-gaps or sway

And I actually have no idea why it is false in the case of sway. Do you know why this is the case @alexarice ?

[alexarice]

Sway has always had the equivalent of the i3-gaps feature so the default option for sway was probably copied from the default option for i3-gaps. I can't remember a specific reason for it.

As a side note, I believe in the past when making changes like this we've hidden them behind a stateVersion change. I'm not sure if this is still the way of doing things

[GaetanLepage]

All the tests are passing.
It could be nice if some i3/i3-gaps/sway users could test this update (@teto).
Anyway, I think it should be ok to merge.

@rycee

[alexarice]

Changing the border should at least have a changelog/news item so that people can find out why they suddenly have titlebars

[GaetanLepage]

Changing the border should at least have a changelog/news item so that people can find out why they suddenly have titlebars

Yes, it seems logical to me. Where should I mention this change ?

[alexarice]

I haven't changed anything in home manager for a while so I'm not sure how things are done at the moment, probably best to wait for someone who knows what they are talking about to input.

[olmokramer]

@GaetanLepage news items go in modules/misc/news.nix.

[teto]

this seems to change the default for sway as is shown in the test. Is that because the default in sway/i3 changed ? otherwise remove "default" probably ?

[GaetanLepage]

This was what I was wondering about previously.
After checking in the upstream projects code, the default value for those options is indeed normal (and not pixel) for both i3 and sway.
The author of the sway module confirmed that at the time he just wanted to copy what was done for i3-gaps.

Setting titlebar to true is consistent with the default value used in i3 and sway, i.e. default_[floating_]border normal.

[teto]

I completely ignored your previous posts very sorry. One bad example of context switching :( if normal is the default then yes we should have that. #3563 (comment) comments on using stateVersion and this seems like a good idea as it's easy to do. Pair it with a docs/release-notes/rl-2305.adoc entry (which can replace/complete the news.nix change)

[GaetanLepage]

All right ! How should I handle the stateVersion check ?
If stateVersion >= 23.05, then put *titlebar settings default to true with sway, else false ?

[teto]

yes

[alexarice]

Neither preserve the original behaviour for people who had gaps activated right?

[GaetanLepage]

You are right. If we go for preserving the previous behavior, let's do it seriously.
Latest push fixes that:

default = if versionOlder stateVersion "23.05" then
  (isI3 && (cfg.config.gaps == null))
else
  true;

[GaetanLepage]

The tests are failing. What is the stateVersion used in the tests ?

[GaetanLepage]

It seems that getting the stateVersion here:

  inherit (config.home) stateVersion;

Leads to the crash of nix-shell . -A install:

Creating initial Home Manager configuration...

Creating initial Home Manager generation...

error: The option `home.stateVersion' is used but not defined.
(use '--show-trace' to show detailed location information)
Uh oh, the installation failed! Please create an issue at

    https://github.com/nix-community/home-manager/issues

if the error seems to be the fault of Home Manager.
Error: Process completed with exit code 1.

Other modules seem to do it succesfully...

It works if I hardcode it:

stateVersion = "20.09";

[GaetanLepage]

Wow, this error was not easy to fix because the message was really misleading.
What caused the problem was adding a dependence on the stateVersion without providing a defaultText.
It was not obvious to me that the lack of defaultText lead to this error during nix-shell . -A install.

Anyway, I think everything is now correct.

[GaetanLepage]

@GaetanLepage news items go in modules/misc/news.nix.

I added an entry in news.nix :)