Fix tab ID becoming 0 once the tab contains splits

[4RH1T3CT0R7]

Tab embeds its root views.Node, so tab:ID() in Lua ends up calling Node.ID(). That method returns 0 for any node that has children, and once a tab contains a split its root is no longer a leaf. A plugin that keys anything on the tab id loses track of the tab the moment the user splits it, which is what #4186 reports.

Tab now carries its own id, taken from the root node when the tab is created, and ID() returns that. For an unsplit tab the value is the same as before, it just no longer changes after a split. A small test in internal/action covers the split case and checks that two tabs get distinct ids.

Fixes #4186

[JoeKar]

I'm unsure, if we really need to add a further id within the tab, which is already a view, instead of taking care of this:

micro/internal/views/splits.go

Lines 53 to 55 in 393cf24

	// The id is unique for each leaf node and provides a way to keep track of a split
	// The id cannot be 0
	id uint64

micro/internal/views/splits.go

Lines 92 to 98 in 393cf24

	// ID returns this node's id or 0 if it is not viewable
	func (n *Node) ID() uint64 {
	if n.IsLeaf() {
	return n.id
	}
	return 0
	}

[4RH1T3CT0R7]

Fair point. 1af86dd drops the extra field and lets Node.ID() return n.id for containers too; a node that gets split keeps its id and hands it to the child taking its place, so GetNode() still resolves to the same leaf and only the 0 for non-leaf nodes goes away. One thing to be aware of: flatten() copies the last remaining child over the root, so once the pane a tab was created with is closed while another pane is left, the tab id becomes that pane's id (a root leaf has to carry the id of its only pane for GetNode() to find it, so I don't see a way around that within views); if that case matters I'd rather go back to the tab-level id.

[JoeKar]

@Neko-Box-Coder:
What do you think about it?

[Neko-Box-Coder]

With this change applied, when a split happens, wouldn't there be 2 nodes (tab parent node and the split node) with the same ID? If so, I don't think this should happen.

[Edit]:
If not, could you call Node.String() on the tab and paste the node trees before and after this merge in your initlua script? (You will need to run micro in debug mode with micro -debug.
If not I can do it when I am free, maybe later this week.

[4RH1T3CT0R7]

Yes, after a split the container node and its first child hold the same id, but this change doesn't create that. hVSplit/vHSplit deliberately pass the old id down to the child that takes the split node's place (vn1 := NewNode(STVert, n.X, n.Y, n.W/2, n.H, n, n.id)), so the duplicate is already there on master; ID() was just hiding it by returning 0 once a node had children. Nothing resolves to the wrong node because GetNode() only matches leaves, and the scans in ResizeSplit, Unsplit and flatten compare siblings within a single children list.

Here is Node.String() on the tab, before a vsplit, after it, and after unsplitting again. I got these from a small Go test driving the same calls rather than from a plugin, and the output is byte-identical on master, since String() prints the id field instead of calling ID():

before vsplit:
{0 0 80 24} 1 0🍁

after vsplit:
-{0 0 80 24} 1 0
	|{0 0 40 24} 1 1🍁
	|{40 0 40 24} 2 1🍁

after unsplit:
{0 0 80 24} 1 0🍁

The only thing that differs is tab:ID() in the middle state: 0 before this patch, 1 after.

I did try the alternative of giving the container a fresh id so the ids stay unique, and it costs more than the bug is worth: the pane that was already there keeps splitID 1, GetNode(1) then returns nil because its leaf is now 3, and Tab.Resize() dereferences that nil, so VSplit/HSplit would have to return the reassigned id too and BufPane would have to update its own. I'd rather keep this patch and leave the tree's existing convention alone. If a tab reporting the same number as one of its panes is the real objection, that is already true before any split, since NewTabFromBuffer does e.SetID(t.ID()) — but I'm happy to go back to my first version instead, which gave Tab its own id copied from the root at creation and left views untouched.

[Neko-Box-Coder]

@4RH1T3CT0R7

Thanks for your reply, I appreciate your graph and in-depth explanations.

The only thing that differs is tab:ID() in the middle state: 0 before this patch, 1 after.

Hm... while I understand what you are saying, I am kinda iffy about having nodes not having a unique ID tbh.

Previously if a lua end user wants to iterate/track buffer nodes, they can just store the IDs that are non-zero. With this change, however, they now would need to do an additional check to see if the node is leaf or not.

That being said, I am not aware of any plugins that rely on the node ID afaik.

I did try the alternative of giving the container a fresh id so the ids stay unique, and it costs more than the bug is worth: [...]

If you just need to have an unique Id for tracking tabs in lua, would storing the tab pointers themselves work? I think they are persistent.

[4RH1T3CT0R7]

I checked this with a small test through luar, and the pointer only half works. luar wraps the *Tab in a new userdata each time it's passed to Lua, so == works (the pointer metatable's __eq compares the Go pointers), but table lookups use raw identity and never match:

local a, b = micro.CurPane():Tab(), micro.CurPane():Tab()
print(a == b)  -- true
local t = {}
t[a] = 1
print(t[b])    -- nil

So a plugin can find a tab it remembered by scanning micro.Tabs().List with ==, but it can't keep per-tab state in a table keyed by the tab, which is what the id is useful for. Given that you'd rather keep node ids unique, I'd go back to my first version: Tab gets its own id copied from the root when it's created, views stays untouched, and non-leaf nodes keep returning 0. That also keeps the tab id stable when the pane the tab was opened with gets closed, which the current version doesn't. I pushed that as 860ba85, a revert of 1af86dd, and if you and @JoeKar would rather not touch this at all, I'm fine closing the PR.