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

`tree_flatten` sometimes behaves in unexpected ways #3503

Open
ilikeheaps opened this Issue Nov 5, 2018 · 12 comments

Comments

Projects
None yet
4 participants
@ilikeheaps

ilikeheaps commented Nov 5, 2018

I'm submitting a…

[x] Bug
[ ] Feature Request
[ ] Documentation Request
[ ] Other (Please describe in detail)

Current Behavior

Relevant issues:
#664
#3001
#3003
Relevant PRs:
#3235
#3501

Expected Behavior

Don't actually know. Points to consider:

  1. I think that stacked/tabbed containers are never redundant as they structurize the layout (there are people that think otherwise though).
  2. Assume Ci <- {H, V}, C1 != C2; then C1[C2 [...]] is visually the same as C2[...] but sometimes you want to create that C1 container to open something in it
  3. Assume that C1 <- {H, V}; then C1[w1... C1 [wk...]] is visually the same as C1[w1... wk...] but has different behaviour (resizing, changing layout, going fullscreen)
  4. No matter what change we make, it will be a fundamental change and might break some scripts (unless we mimic the old behaviour by default). I suppose we can hope that nobody relies on edge cases that we want to fix.

My proposal (just a loose one)

Find chains of splith/v containers such that:

  • each except the last container has only one child
  • there are at least 3 containers in the chain

From such a chain leave only two containers (the last one and I don't know which would be the other, it seems to not matter much so it could be its parent or could be the top one if it was easier to implement)

@i3 i3 deleted a comment from i3bot Nov 5, 2018

@i3 i3 deleted a comment from i3bot Nov 5, 2018

@orestisf1993

This comment has been minimized.

Member

orestisf1993 commented Nov 5, 2018

I generally think that non-leaf containers with no leaf children are redundant like in #3001 (comment) and #3235 (comment)

EDIT: I'd also want this to be addressed to write extensive tests for #3085.

@orestisf1993

This comment has been minimized.

Member

orestisf1993 commented Nov 5, 2018

I know it is a bit early to discuss this but to encourage extensive tests I think it would help if we implemented a set of functions that easily create a layout and then verifies it. So, we would be able to do something like this:

# opens windows a, b, c, d, assigns them to variables, creates the specified layout and focuses a
create_layout 'H[ V[ a* V[ b c ] d ]';
cmd 'move right';
verify_layout 'H[ V [ b c ] a* d ]`;

by the way, this is another example where i3 creates redundant containers:

a

@Airblader

This comment has been minimized.

Member

Airblader commented Nov 5, 2018

Yes, implementing such functions would be a good idea in my opinion as well! Maybe it would also mark each window with the name chosen in the string.

@orestisf1993

This comment has been minimized.

Member

orestisf1993 commented Nov 5, 2018

Yes, marks would be more elegant than variables. We could then use [con_mark=…] to do operations.

@ilikeheaps

This comment has been minimized.

ilikeheaps commented Nov 5, 2018

I generally think that non-leaf containers with no leaf children are redundant like in #3001 (comment) and #3235 (comment)

In H[V[a b] V[c d]] H is a non-leaf container with no leaf children. Regarding these comments:

containers with only one non-leaf child are redundant

I mostly agree, but I think we must allow one such container because when the user is making a new window in a new split (split h; <new window>) the first command creates such a container. And I think that if the user introduces a tabbed/stacked layout, he means to categorize things and we shouldn't touch these at all. Possibly we could make it optional, it sounds easy enough.

@Airblader

This comment has been minimized.

Member

Airblader commented Nov 5, 2018

Please no optional behavior. This topic is complex enough, it should work the same for all users.

@orestisf1993

This comment has been minimized.

Member

orestisf1993 commented Nov 5, 2018

In H[V[a b] V[c d]] H is a non-leaf container with no leaf children.

Correct. I change my proposition to "non-leaf containers with 1 non-leaf children".

I mostly agree, but I think we must allow one such container because when the user is making a new window in a new split (split h; <new window>) the first command creates such a container. And I think that if the user introduces a tabbed/stacked layout, he means to categorize things and we shouldn't touch these at all. Possibly we could make it optional, it sounds easy enough.

I think my policy doesn't break this behavior

@ilikeheaps

This comment has been minimized.

ilikeheaps commented Nov 5, 2018

I think my policy doesn't break this behavior

Yes, you are right. I missed the "non-leaf" part.

@orestisf1993

This comment has been minimized.

Member

orestisf1993 commented Nov 5, 2018

I'll try to write the new test functions, make them a separate PR (unless if you wanted to do it?).

@Airblader what's your opinion on the proposed policy?

@Airblader

This comment has been minimized.

Member

Airblader commented Nov 5, 2018

I haven't thought about it yet (and am traveling the next couple days). I think adding the test utilities is a good start regardless.

Then we can define some examples through which we can (by hand) run the algorithm to see what would happen

@orestisf1993

This comment has been minimized.

Member

orestisf1993 commented Nov 6, 2018

I had progress with the test functions: next...orestisf1993:tree.t, I think it's a pretty convenient way to do tests.

If merged, I may use it for #3085, #2954, #3407.

@Airblader

This comment has been minimized.

Member

Airblader commented Nov 6, 2018

I think it'd be good to externalize these functions from the actual testcases file, both for better overview and reusability across testfiles.

orestisf1993 added a commit to orestisf1993/i3 that referenced this issue Nov 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment