Skip to content

Fixing missing case for handling root node for splitting #3983

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.

Sign up for GitHub

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

Open
Neko-Box-Coder wants to merge 2 commits into
base: master
Choose a base branch
from
+28 −10
Open

Fixing missing case for handling root node for splitting #3983

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e618729
Fixing missing case for handling root node for splitting, fixes #3980
Neko-Box-Coder Jan 31, 2026
22adeef
Adding special case for root node for flatten()
Neko-Box-Coder Jan 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 28 additions & 10 deletions internal/views/splits.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,7 +413,7 @@ func (n *Node) HSplit(bottom bool) uint64 {
if !n.IsLeaf() {
return 0
}
if n.Kind == STUndef {
if n.parent == nil {
Copy link
Collaborator

@dmaluka dmaluka Feb 1, 2026

Choose a reason for hiding this comment

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

The next commit seems to make this change unneeded? I mean, if we guarantee that the root is always STUndef, why not use it? Otherwise, if we don't use STUndef, why do we even need it?

And IMHO using STUndef makes the code a bit more readable.

Copy link
Contributor Author

@Neko-Box-Coder Neko-Box-Coder Feb 1, 2026

Choose a reason for hiding this comment

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

Minor thing but I would think n.parent == nil is more straightforward no? Since that is the definition of root node.

A child node can be of type STUndef (Although it is a bug), but a child node definite cannot have a node parent of nil right?

Copy link
Collaborator

@dmaluka dmaluka Feb 2, 2026

Choose a reason for hiding this comment

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

Then, again, why need this STUndef constant at all?

Although, after more thought, - ok, let's keep it (even though it is causing a bit of redundancy), just to let it reflect that it is indeed undefined whether it is horizontal or vertical.

Copy link
Contributor Author

@Neko-Box-Coder Neko-Box-Coder Feb 2, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Then, again, why need this STUndef constant at all?

Nah, we don't need it. Didn't do it cuz I only did minimal change just to fix the issue.

It doesn't matter if it is horizontal or not if it gets changed anyway. If we agree to remove it, I can do it :)

n.Kind = STVert
}
if n.Kind == STVert {
Expand All @@ -429,13 +429,13 @@ func (n *Node) VSplit(right bool) uint64 {
if !n.IsLeaf() {
return 0
}
if n.Kind == STUndef {
if n.parent == nil {
n.Kind = STHoriz
}
if n.Kind == STVert {
return n.vVSplit(right)
if n.Kind == STHoriz {
return n.hVSplit(0, right)
}
return n.hVSplit(0, right)
return n.vVSplit(right)
}

// unsplits the child of a split
Expand Down Expand Up @@ -483,7 +483,17 @@ func (n *Node) Unsplit() bool {
// flattens the tree by removing unnecessary intermediate parents that have only one child
// and handles the side effect of it
func (n *Node) flatten() {
if n.parent == nil || len(n.children) != 1 {
if len(n.children) != 1 {
return
}

// Special case for root node
if n.parent == nil {
*n = *n.children[0]
n.parent = nil
Comment on lines +492 to +493
Copy link
Member

@JoeKar JoeKar Feb 2, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, yes...right. We overwrite the root node with the last remaining children, by which it receives the id of the children, with which it is identified in the tab resize.
But still we should keep the removed n.Kind = STUndef, otherwise func (n *Node) String() wouldn't print what we expect from the new root. Otherwise in case we agree to remove STUndef we've to adjust this function to print "/" on n.parent == nil.

Am I right that we should keep the fault in tab.go#L382 to identify that something went wrong, because GetNode() returns nil by intention in case the pane ID wasn't found?

Copy link
Contributor Author

@Neko-Box-Coder Neko-Box-Coder Feb 2, 2026

Choose a reason for hiding this comment

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

Otherwise in case we agree to remove STUndef we've to adjust this function to print "/" on n.parent == nil.

True. Forgot about that change when I made this change. Will add it back if we want to leave STUndef as it is.

Am I right that we should keep the fault in tab.go#L382 to identify that something went wrong, because GetNode() returns nil by intention in case the pane ID wasn't found?

Not sure what fault you are referring to. t.GetNode(p.ID()) should never fail no?

Copy link
Member

@JoeKar JoeKar Feb 2, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure what fault you are referring to. t.GetNode(p.ID()) should never fail no?

But it can splits.go#L140 and this is intended, when the given ID isn't present at the related nodes.

Will add it back if we want to leave STUndef as it is.

I vote for keeping it too.

Copy link
Contributor Author

@Neko-Box-Coder Neko-Box-Coder Feb 2, 2026

Choose a reason for hiding this comment

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

Not sure if I am missing something but the given id should always be present for the node unless something is wrong no?

Copy link
Contributor Author

@Neko-Box-Coder Neko-Box-Coder Feb 2, 2026

Choose a reason for hiding this comment

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

I vote for keeping it too.

I vote for removing it :) since it's sole purpose is to identify root node and that can be done by just checking if parent exists or not.

Just need @dmaluka 's vote or anyone else interested.

Copy link
Contributor Author

@Neko-Box-Coder Neko-Box-Coder Feb 2, 2026

Choose a reason for hiding this comment

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

Okay got it. Added back the n.Kind = STUndef

Copy link
Member

@JoeKar JoeKar Feb 3, 2026

Choose a reason for hiding this comment

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

You don't like this n.Kind = STUndef statement, right? 😉

Copy link
Contributor Author

@Neko-Box-Coder Neko-Box-Coder Feb 3, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

@JoeKar
Forgot to explain it why I removed n.Kind = STUndef after adding it 😅

I initlially kept it when adding

		for _, c := range n.children {
			c.parent = n
		}

So like

	if n.parent == nil {
		*n = *n.children[0]
		n.parent = nil
		n.Kind = STUndef
		for _, c := range n.children {
			c.parent = n
		}
		return
	}

for fixing the crash @dmaluka observed.

Then when I was testing it, it failed to resize the last split correctly when unsplitting the splits one by one, where the last split only took half of the screen instead of taking the whole screen.

After some debugging, I realized the kind for the root node cannot be undef when we remove the intermediate one since it can have more than one grandchildren.

Like so:

2026/02/03 18:46:40 Node Tree:
 -{0 0 211 46} 1 0
	|{0 0 105 46} 1 1🍁
	|{105 0 106 46} 2 1
		-{105 0 106 15} 2 2🍁
		-{105 15 106 15} 3 2🍁
		-{105 30 106 16} 4 2🍁

Becomes this if I removed leaf node ID 1:
(forget about the coords and parent id. This is just for demo purposes, I cba to rebuild micro to get the logs)

2026/02/03 18:46:40 Node Tree:
 -{0 0 211 46} 1 0
		-{105 0 106 15} 2 2🍁
		-{105 15 106 15} 3 2🍁
		-{105 30 106 16} 4 2🍁

Looking at the tree before being flattened.
If I unsplit leaf node ID 1, the unsplit will flatten the intermediate non leaf node ID 2 with grandparent leaf nodes 2, 3 and 4. If the kind for the root node is Undef, the resize won't work correctly.

Copy link
Collaborator

@dmaluka dmaluka Feb 4, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

After some debugging, I realized the kind for the root node cannot be undef when we remove the intermediate one since it can have more than one grandchildren.

Good point. Indeed, making it STUndef just because it becomes the root node doesn't make sense and is very incorrect.

But I think @JoeKar 's very original point still stands: it does makes sense to make it STUndef if it is in such a state as if it has been just created via NewRoot() with STUndef, which actually means: if it becomes the root node and has no children?

So how about this?

diff --git a/internal/views/splits.go b/internal/views/splits.go
index 5014d3ba..d310db21 100644
--- a/internal/views/splits.go
+++ b/internal/views/splits.go
@@ -494,6 +494,9 @@ func (n *Node) flatten() {
 		for _, c := range n.children {
 			c.parent = n
 		}
+		if len(n.children) == 0 {
+			n.Kind = STUndef
+		}
 		return
 	}

And furthermore, now in this regard I'm also thinking about this / character when printing the tree. Now it doesn't quite make sense to me to always print a special character (instead of - or |) for a node just because it is the root node. As we can see now, if it has children, it must be either STVert or STHoriz, so it is not really different from other (non-root) nodes in this regard, so we should just normally print - or | for it? (Otherwise, we are just hiding the information about its Kind, whereas the purpose of printing the tree is, on the contrary, to print any useful information for debugging.)

And in the case if it is STUndef (since it has no children, so it is the only node in the tree), we may just not print any character at all?

So how about:

diff --git a/internal/views/splits.go b/internal/views/splits.go
index 5014d3ba..d310db21 100644
--- a/internal/views/splits.go
+++ b/internal/views/splits.go
@@ -494,6 +494,9 @@ func (n *Node) flatten() {
 		for _, c := range n.children {
 			c.parent = n
 		}
+		if len(n.children) == 0 {
+			n.Kind = STUndef
+		}
 		return
 	}
 
@@ -541,18 +544,18 @@ func (n *Node) flatten() {
 func (n *Node) String() string {
 	var strf func(n *Node, ident int) string
 	strf = func(n *Node, ident int) string {
-		var marker string
+		marker := ""
+		if n.Kind == STHoriz {
+			marker = "-"
+		} else if n.Kind == STVert {
+			marker = "|"
+		}
+
 		var parentId uint64 = 0
-		if n.parent == nil {
-			marker = "/"
-		} else {
-			if n.Kind == STHoriz {
-				marker = "-"
-			} else if n.Kind == STVert {
-				marker = "|"
-			}
+		if n.parent != nil {
 			parentId = n.parent.id
 		}
+
 		str := fmt.Sprint(strings.Repeat("\t", ident), marker, n.View, n.id, parentId)
 		if n.IsLeaf() {
 			str += "🍁"

Copy link
Member

@JoeKar JoeKar Feb 5, 2026

Choose a reason for hiding this comment

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

But I think @JoeKar 's very original point still stands: it does makes sense to make it STUndef if it is in such a state as if it has been just created via NewRoot() with STUndef, which actually means: if it becomes the root node and has no children?

So how about this?

Yes, this should work.

And in the case if it is STUndef (since it has no children, so it is the only node in the tree), we may just not print any character at all?

I'm fine with it! :)

for _, c := range n.children {
c.parent = n
}
return
}

Expand Down Expand Up @@ -531,11 +541,19 @@ func (n *Node) flatten() {
func (n *Node) String() string {
var strf func(n *Node, ident int) string
strf = func(n *Node, ident int) string {
marker := "|"
if n.Kind == STHoriz {
marker = "-"
var marker string
var parentId uint64 = 0
if n.parent == nil {
marker = "/"
} else {
if n.Kind == STHoriz {
marker = "-"
} else if n.Kind == STVert {
marker = "|"
}
parentId = n.parent.id
}
str := fmt.Sprint(strings.Repeat("\t", ident), marker, n.View, n.id)
str := fmt.Sprint(strings.Repeat("\t", ident), marker, n.View, n.id, parentId)
if n.IsLeaf() {
str += "🍁"
}
Expand Down