Skip to content

Comments

Fix unable to perform proportional resize caused by chained parents after quiting a nested VSplit inside a HSplit#3708

Merged
JoeKar merged 2 commits intomicro-editor:masterfrom
Neko-Box-Coder:ChainedParentsFix
Apr 29, 2025
Merged

Fix unable to perform proportional resize caused by chained parents after quiting a nested VSplit inside a HSplit#3708
JoeKar merged 2 commits intomicro-editor:masterfrom
Neko-Box-Coder:ChainedParentsFix

Conversation

@Neko-Box-Coder
Copy link
Contributor

@Neko-Box-Coder Neko-Box-Coder commented Mar 30, 2025

When you do a HSplit, follow by a VSplit, then quit the last pane and do a HSplit again, the proportional resize won't work as expected.

Currently you will get a 50%, 25%, 25% HSplits.
Instead it should be 33%, 33%, 33%.

This problem is caused by failing to resolve chained parents and the resize is called on the "wrong" parent.

The patch fixes chained parents by simplifying the tree when unsplittng.

This problem can be visualized by calling the String() on the current tab. In lua, it will look something like this:

function MyFunction(bp)
    micro.Log("Node Tree:", bp:Tab():String())
end

So if you have something like this

-------------------------------------------------------------
|                                  |                        |
|                                  |             2          |
|              1                   |----------------------- |
|                                  |                        |
|                                  |             3          |
-------------------------------------------------------------

the tree looks like this:

-{0 0 211 46} 1
     |{0 0 105 46} 1🍁
     |{105 0 106 46} 2
         -{105 0 106 23} 2🍁
         -{105 23 106 23} 3🍁

If you quit the 3rd pane, the tree will look like this:

-{0 0 211 46} 1
    |{0 0 105 46} 1🍁
    |{105 0 106 46} 
        -{105 0 106 46} 2🍁

Then if you do a VSplit, the tree will look like

-{0 0 211 46} 1
    |{0 0 105 46} 1🍁
    |{105 0 106 46} 2
        -{105 0 106 46} 2
            |{105 0 53 46} 2🍁
            |{158 0 53 46} 4🍁

Which fails the resize since it is calling on a chained parent

After applying the patch, it will "simplify" the tree after quitting the 3rd pane

-{0 0 211 46} 1
    |{0 0 105 46} 1🍁
    |{105 0 106 46} 2🍁

Then when you do another VSplit, it will look like this:

-{0 0 211 46} 1
    |{0 0 70 46} 1🍁
    |{70 0 70 46} 2🍁
    |{140 0 71 46} 3🍁

and the resize will work as expected since it will be called on the correct parent.

@Neko-Box-Coder Neko-Box-Coder changed the title Fix unable to perform proportional resize caused by chained parents after quiting a nested HSplit inside a VSplit Fix unable to perform proportional resize caused by chained parents after quiting a nested VSplit inside a HSplit Apr 3, 2025
@dmaluka
Copy link
Collaborator

dmaluka commented Apr 13, 2025

Nice fix, thanks.

@Neko-Box-Coder Neko-Box-Coder force-pushed the ChainedParentsFix branch 2 times, most recently from b31dc40 to dadb96b Compare April 14, 2025 09:49
@Neko-Box-Coder Neko-Box-Coder force-pushed the ChainedParentsFix branch 5 times, most recently from b68dda9 to 14191dc Compare April 15, 2025 01:38
@Neko-Box-Coder Neko-Box-Coder force-pushed the ChainedParentsFix branch 2 times, most recently from 5f35974 to 025e822 Compare April 23, 2025 23:26
successor.parent = n.parent
if successor.IsLeaf() {
successor.Kind = n.Kind
} else if !successor.IsLeaf() && successor.Kind == n.parent.Kind {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!successor.IsLeaf() here is redundant?

And actually successor.Kind == n.parent.Kind seems redundant too, it should always be true, unless we have a bug?

successor.Kind = n.Kind
} else if !successor.IsLeaf() && successor.Kind == n.parent.Kind {
// If we have the same kind as the parent (caused by removing chained parent),
// replace successor node with its children.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we remove successor.Kind == n.parent.Kind above as redundant, we can update this comment like: "If the successor node has its own children, it means it is a redundant chained parent too, so replace it with its children."?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that is true. I wasn't thinking much when copying and pasting this part.

}

// Update propW and propH since the parent of the children might have been updated,
// so the children may have new siblings
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/might have been updated/has been updated/
s/may have new siblings/have new siblings/

?

// Replace current node with its child node to remove chained parent
n.parent.children[ind] = n.children[0]

successor := n.parent.children[ind]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a comment above this line: "Make sure the tree remains in a consistent state: any child node's kind (horizontal vs vertical) is the opposite of its parent's kind"?

Also why not rearrange the lines like this:

--- a/internal/views/splits.go
+++ b/internal/views/splits.go
@@ -495,10 +495,12 @@ func (n *Node) flatten() {
 	}
 
 	// Replace current node with its child node to remove chained parent
-	n.parent.children[ind] = n.children[0]
-
-	successor := n.parent.children[ind]
+	successor := n.children[0]
+	n.parent.children[ind] = successor
 	successor.parent = n.parent
+
+	// Maintain the tree in a consistent state: any child node's kind (horiz vs vert)
+	// should be the opposite of its parent's kind.
 	if successor.IsLeaf() {
 		successor.Kind = n.Kind
 	} else if !successor.IsLeaf() && successor.Kind == n.parent.Kind {

@JoeKar JoeKar merged commit 06fe85c into micro-editor:master Apr 29, 2025
6 checks passed
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.

3 participants