Allow creation of new workspace above top most workspace

[FluxTape]

Add the layout option allow-workspace-above-first to allow adding new workspaces above the top most workspace. When this option is enabled the first and last workspace on a monitor are always empty (compared to just the last one) allowing the movement of windows/columns above the first workspace.

[YaLTeR]

Please add checks into verify_invariants() that when this flag is active, there is an empty workspace above first, similarly to checks about there being an empty workspace below last. See

niri/src/layout/mod.rs

Line 1819 in 8d08782

monitor.workspaces.last().unwrap().columns.is_empty(),

Also run the randomized tests as described here: https://github.com/YaLTeR/niri/wiki/Developing-niri#running-tests

I expect you'll need to fix several more places that assume there's only an empty workspace in the end.

[FluxTape]

I've made the necessary changes to verify_invariants() and fixed any issues that popped up. Anything else that needs changing?

[YaLTeR]

I'm so sorry, I forgot to submit the review several days ago..

[FluxTape]

Implemented all your suggestions, let me know if there are any more issues

[YaLTeR]

I'll take it from here, thanks.

[YaLTeR]

If you don't mind, I will postpone this to after the upcoming release. The logic has some remaining bugs, which are not difficult to fix, but they cannot get caught by the current testing system. I want to extend the testing system a bit to make sure they are caught and that there are no other similar bugs left.

[YaLTeR]

So that I don't forget, the current bugs are:

1. Interactive move uses add_tile() but assumes the workspace idx doesn't change, which it does if a workspace is added at the top. Possibly similar bugs elsewhere in the code. Not caught because the testing system needs to be able to advance animations to complete a workspace switch up.
2. Toggling the option at runtime doesn't add or remove the topmost workspace. Not caught because there are no tests that change the options.

[FluxTape]

So that I don't forget, the current bugs are:

1. Interactive move uses add_tile() but assumes the workspace idx doesn't change, which it does if a workspace is added at the top. Possibly similar bugs elsewhere in the code. Not caught because the testing system needs to be able to advance animations to complete a workspace switch up.

2. Toggling the option at runtime doesn't add or remove the topmost workspace. Not caught because there are no tests that change the options.

To fix 2) it should be enough to add

        if self.options.empty_workspace_above_first != options.empty_workspace_above_first
            && self.workspaces.len() > 1
        {
            if options.empty_workspace_above_first {
                self.add_workspace_top();
            } else if self.workspace_switch.is_none() {
                self.workspaces.remove(0);
                self.active_workspace_idx = self.active_workspace_idx.saturating_sub(1);
            }
        }

to the top of update_config(..) in monitor.rs

[YaLTeR]

Working on advancing animations in the test system in #825.

[YaLTeR]

Turns out, there was one more case that could trigger issue 1 and does not need advancing the time. Added it too.

[YaLTeR]

I've been running the randomized operations test a lot, and I've also implemented a proptest-state-machine version of the test that is a bit smarter in terms of generating valid sequences of operations in hopes that it will be more efficient at finding the issue 1 edge case. Unfortunately, I couldn't seem to get it to find it faster than the existing random operations test, and it adds quite a bit of code, so I kept it in a private branch for now. It did find one oversight in the AddWindowRightOf test implementation though which I just committed to main.

Both of these random tests manage to find the second output version of issue 1, but only after several minutes of runtime. Neither of the random tests has yet managed to find the single-output version of issue 1, which is slightly worrying. Makes me suspicious that there might be other problems that the current randomized tests have a hard time catching (probably due to the complexity and amount of operations).

Maybe it would be worth to also hook up cargo-fuzz with its coverage guidance. It uses a different Arbitrary trait though...

I'm not blocking this PR on this, just commenting what I've been spending time on. I'll next add a test for issue 2 and then the fix from your suggestion, and give it some more manual testing.

[FluxTape]

For 2) you can take a look at this branch for inspiration, which helped me find and fix some issues.

[YaLTeR]

Thanks, I used that as a reference, but made a slightly simpler variation. Which immediately found an existing bug in update_config() 🤦

[YaLTeR]

With the config fix, one more problem that is not caught by tests: when enabling ewaf during a workspace switch animation or gesture, it gets glitched because its indices are now one off.

Need to check if it's ok to update the indices, or better to just cancel the gesture/animation.

Also, probably want to preserve the first empty workspace when disabling ewaf in case it was focused.

[YaLTeR]

Note to self, remember to add it to the config wiki page

[YaLTeR]

I'm adding some ad-hoc conditions to the randomized tests, and I caught another "workspace index changed underneath" issue:

1. Start a workspace switch gesture on the first empty workspace.
2. Open a window.
3. The workspace index jumps up, but the index stored in the gesture remains unchanged.

I'll see if maybe simply stopping the gesture in add_workspace_top() is the best solution for now. Outside of this PR, maybe the workspace switch anim/gesture should store a workspace ID instead? Though, it won't magically fix all problems either. Need better tests for this.

[YaLTeR]

I'll see if maybe simply stopping the gesture in add_workspace_top() is the best solution for now.

It might not be since this means calling clean_up_workspaces() which can further mess up workspace indices...

[YaLTeR]

Another thing: need to make it so at startup with named workspaces, the named workspace is focused, rather than the first empty above named.