Fixing missing case for handling root node for splitting

[Neko-Box-Coder]

At the beginning, the node tree looks like this, where x = STUndef

// <STUndef> <Coords> <ID> <Parent>
Node Tree:
 x{0 0 211 46} 1 root🍁

When performing VSplit or HSplit at the beginning, we check if we are root node using STUndef, in which we will always put the new split as its children, then we set the node kind for the root node. Which then looks like this:

Node Tree:
 -{0 0 211 46} 1 root
	|{0 0 105 46} 1 1🍁
	|{105 0 106 46} 2 1🍁

Since we have set root node kind after the initial split, the special handling for the root node using STUndef will no longer be valid.

When using quit instead of unsplit, you can only get to the following node tree, and doing quit on the child will quit micro entirely.

Node Tree:
 -{0 0 211 46} 1 root
	|{0 0 211 46} 1 1🍁

However, if you are using unsplit, you can actually go back to the original state, like this:
One pane left:

Node Tree:
 -{0 0 211 46} 1 root
	|{0 0 211 46} 1 1🍁

After unsplit:

Node Tree:
 -{0 0 211 46} 1 root🍁

The fix is simply check if the parent is nil to determine if we are the root node or not.

I have also rearrange VSplit last if condition to match HSplit where we are treating current node as non-leaf node and add the new split to it's children.

Also updated the String() to show the parent node id for easier debugging.

Fixes #3980

[JoeKar]

When this is the initial state...

Node Tree:
 x{0 0 211 46} 1 root🍁

...and this the state after the first unsplit (back to just one pane)...

Node Tree:
 -{0 0 211 46} 1 root
	|{0 0 211 46} 1 1🍁

...shouldn't we flat this state into the initial state already to return to the initial state with this one and last split?

Node Tree:
 x{0 0 211 46} 1 root🍁

So that any further unsplit doesn't do anything, even not in the background.
Because right now we can once again unsplit this tree to the initial state, while the user presentation doesn't change.

Besides of that the rearrangement of VSplit() is a good idea, because it fooled me while debugging. 😅

[Neko-Box-Coder]

So that any further unsplit doesn't do anything, even not in the background.
Because right now we can once again unsplit this tree to the initial state, while the user presentation doesn't change.

I did think about this while creating the PR fix. The thing is I think this behavior has always been a valid state for the root node, whether it is before or after my flatten() fix.

One of the trick point is that the flatten() function needs to access the grandparent (parent's parent) of the node being unsplit. Which won't work in this case since the grandparent of the root node child doesn't exist:

Node Tree:
?? <-- unflatten will try to access this.
 -{0 0 211 46} 1 root
	|{0 0 211 46} 1 1🍁

I probably could change the logic of flatten() but I don't remember anything on how it works to be frank 😂, simply because it is a complicated logic.

I vaguely remember the first iteration of flatten() was recursive, which might have worked on this scenario but I think we refactored/removed it due to how complicated it is.

So in short, in theory yes I agree with you. In practice I will probably need to spend more time to recover my memory on how it works and see if I can modify it without making it recursive. And also need to test it as well.

I can take a look at it at some point but can't promise anything for now. If you want to have a try, go for it :)

[JoeKar]

If you want to have a try, go for it :)

func (n *Node) flatten() {
	if len(n.children) != 1 {
		return
	}

	if n.parent == nil {
		n.Kind = STUndef
		n.children = n.children[:0]
		return
	}
	[...]

Not 100% sure, if this covers all and everything, but looks good from behavior and node printing.

[dmaluka]

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.

[Neko-Box-Coder]

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?

[dmaluka]

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.

[dmaluka]

Just tried testing this PR... Apparently it is fixing one crash but introducing another one, which is even easier to reproduce: for example:

1. VSplit
2. Focus the 1st (i.e. leftmost) split.
3. Unsplit

Result:

Micro encountered an error: runtime.errorString runtime error: invalid memory address or nil pointer dereference
runtime/panic.go:262 (0x477659)
runtime/panic.go:261 (0x477629)
github.com/zyedidia/micro/v2/internal/action/tab.go:385 (0x8dd494)
github.com/zyedidia/micro/v2/internal/action/actions.go:2045 (0x8c474e)
github.com/zyedidia/micro/v2/internal/action/bufpane.go:567 (0x8cbbad)
github.com/zyedidia/micro/v2/internal/action/tab.go:224 (0x8c9fb1)
github.com/zyedidia/micro/v2/internal/action/bufpane.go:183 (0x8c9dd9)
github.com/zyedidia/micro/v2/internal/action/bufpane.go:33 (0x8c9d71)
github.com/zyedidia/micro/v2/internal/action/bufpane.go:546 (0x8cb919)
github.com/zyedidia/micro/v2/internal/action/bufpane.go:464 (0x8cb3bc)
github.com/zyedidia/micro/v2/internal/action/tab.go:337 (0x8dcd45)
github.com/zyedidia/micro/v2/internal/action/tab.go:141 (0x8dbe65)
github.com/zyedidia/micro/v2/cmd/micro/micro.go:542 (0x90d06a)
github.com/zyedidia/micro/v2/cmd/micro/micro.go:481 (0x90cbb0)
internal/runtime/atomic/types.go:194 (0x441f4b)
runtime/asm_amd64.s:1700 (0x47d341)
 
If you can reproduce this error, please report it at https://github.com/zyedidia/micro/issues