Enhance Tab Bar #3795

cutelisp · 2025-07-01T09:35:24Z

This is a PR to implement tab dragging.

I chose the most simple approach I could think of.

Notes:

In case this pull request is merged, it will enhance the dragging experience.
If hovering tabbar arrows make scrolling continuesly, it will enhance the dragging experience (Needs a PR).
Making tabbar scrolling possible while dragging is possible?

dmaluka · 2025-07-06T15:59:33Z

internal/action/tab.go

@@ -114,13 +118,26 @@ func (t *TabList) HandleEvent(event tcell.Event) {
 					t.Scroll(4)
 				} else {
 					ind := t.LocFromVisual(buffer.Loc{mx, my})
-					if ind != -1 {
+					if ind == -1 {
+						ind = len(t.List) + 1


This is causing a crash when clicking on the tabbar somewhere after the last tab (since SetActive() is called for a non-existing tab).

Besides that, shouldn't this be len(e.List), not len(t.List) + 1? Tabs are indexed from 0, not from 1.

Offtopic: What do you think about creating a feature that creates a newtab on doubleclick after last tab?

I don't think that it something I would be using, but if you find it useful, maybe why not.

Perhaps not a double click but a single click with the right button or the middle button (so it is both faster to click and easier to implement)? WDYT?

I gave double click as an example because it's how it's done in other editors. I think we can make it right click to be cleaner. You talked about middle button... I wonder if we can make it close tabs?

Offtopic 2: Any particular reason this type of still hanging around?

Perhaps not a double click but a single click with the right button or the middle button (so it is both faster to click and easier to implement)? WDYT?

I'm not entirely sure if this feature makes sense in a terminal based editor, but if it's implemented it should IMO be both double click and middle click to be consistent with how tab bars work in other programs (web browsers, VS Code, Sublime Text, etc.). Using familiar keyboard shortcuts by default is one of the main selling points of micro.

it should IMO be both double click and middle click

you mean double click for new tab and middle click for close tab?

if it's implemented it should IMO be both double click and middle click to be consistent with how tab bars work in other programs (web browsers, VS Code, Sublime Text, etc.). Using familiar keyboard shortcuts by default is one of the main selling points of micro.

Not really familiar to me. It's the first time I hear about this behavior.

Just checked: Firefox indeed handles both double click and middle click this way, but Chrome doesn't.

XFCE Terminal and Gnome Terminal don't seem to do that either (although that's just because their tabs always occupy the entire tab bar, i.e. there is no free space to click on). Ditto gedit (aka Gnome Text Editor).

Never used VS Code or Sublime Text. They might do that, but they are just two of many editors.

Vim seems to opens a new tab on double click but not on middle click. Ditto Geany.

Emacs doesn't seem to open a new tab on either double click or middle click.

Just checked: Firefox indeed handles both double click and middle click this way, but Chrome doesn't.

I stand corrected – the middle click seems to be mostly just a Firefox thing (I tried a bunch of software and it only worked in Firefox, Vivaldi, Tor Browser, and Konsole). Double click on the other hand is more common and works in many other text editors too:

VS Code

Sublime Text

Kate / KWrite

Zed

Ok, I'm not against implementing opening a new tab on double click.

dmaluka · 2025-07-06T16:08:33Z

internal/action/tab.go

+					}
+
+					if t.release {
+						t.release = false


Shouldn't we set t.release to false always when the mouse is pressed, i.e. in the above scrolling cases (mx == 0 and mx == t.Width-1) as well?

Moreover, it seems we need to set t.release to false even when the click is not on the tabbar, i.e. when we don't handle the click event here but only propagate it to t.List[t.Active()].HandleEvent(). Why? Because if the mouse was clicked e.g. on a bufpane, not one the tabbar, but then dragged to the tabbar, if our t.release is still true, we erroneously handle mouse move events as mouse click events, so we are back to the behavior before your PR, i.e. we activate tabs instead of dragging them. (That is what actually still happens with your PR in this case, you can try it.)

Also I have a feeling that just relying on t.release isn't enough, we might need a separate field e.g. dragging (a bit similar to resizing in the Tab struct), to prevent unwanted dragging when the initial click was on the tabbar but not on any tab.

Should've checked this 🤦‍♂️.

if our t.release is still true, we erroneously handle mouse move events as mouse click events

Even if t.release is false it still leads to MainTab().Move(ind), so I agree with you relying on t.release isn't enough.
Instead of creating a separate field on Tab struct we could swap t.release to a variable (on tablist) with 3 states, tabclick, non-tabclick or released, maybe its more simple?.

I might have misunderstand you above.

we might need a separate field e.g. dragging

That field would be on Tab or TabList struct?

Tab already have a release var. At first glance that could help, right?

we might need a separate field e.g. dragging

That field would be on Tab or TabList struct?

I meant TabList, not Tab.

What I had in mind: when a tab handles mouse drag events on its own (without propagating them to a bufpane), it does that in order to resize bufpanes, and it uses its t.resizing field for that. And similarly, when the tablist handles mouse drag events on its own (without propagating them to a tab), it does that in order to drag tabs, so it could, similarly, use its t.dragging field for that.

Tab already have a release var. At first glance that could help, right?

I'm not sure how that could help. We don't want to traverse the list of tabs just to check if any of them has t.release set to false, do we? It's easier and cleaner to just track the release state in TabList as well, right?

I'm not sure how that could help. We don't want to traverse the list of tabs just to check if any of them has t.release set to false, do we? It's easier and cleaner to just track the release state in TabList as well, right?

In my mind MainTab().release would be enough, but it isn't since we can change tabs while mousepress.

I'm convinced t.dragging is the way. I'll try to implement it, thanks for the feedback.

dmaluka · 2025-07-06T16:30:16Z

internal/action/tab.go

 						t.SetActive(ind)
+					} else {
+						MainTab().CurPane().TabMoveCmd([]string{strconv.Itoa(ind + 1)})


This is hacky. Instead of abusing TabMoveCmd() and needlessly converting the index back and forth, I'd implement a separate generic function (as a method of Tab or TabList) for moving a tab at the given index, and use it both here and in TabMoveCmd().

Cool idea, I started creating it as a Tab method but it felt odd so I moved it for TabList although in tab would be more convenient.
Also, calling Tabs.SetActive(i) inside Move isn't much intuitive and potentially limiting for plugins. (reminds me #3750)

Mixing this all, wts your thoughts on this?

// Move moves the the tab to the given index func (t *Tab) Move(i int) { Tabs.MoveTab(t, i) Tabs.SetActive(i) } // MoveTab moves the specified tab to the given index func (tl *TabList) MoveTab(t *Tab, i int) { i = util.Clamp(i, 0, len(tl.List)-1) tl.RemoveTab(t.Panes[0].ID()) tl.List = append(tl.List, nil) copy(tl.List[i+1:], tl.List[i:]) tl.List[i] = t tl.Resize() tl.UpdateNames() }

// Move moves the the tab to the given index func (t *Tab) Move(i int) { Tabs.MoveTab(t, i) Tabs.SetActive(i) }

This assumes that whenever we want to move a tab, we also want to activate it. Why? There may be cases when we don't want to do that (especially if this may be used by an arbitrary plugin, with arbitrary use cases).

And AFAICS the tablist is already exported to lua, via Tabs(), so it should be already possible for plugins to activate a tab, via Tabs():SetActive(i)? Or if we want to make it "friendlier", we might add another Tab method, e.g. Activate()?

// MoveTab moves the specified tab to the given index func (tl *TabList) MoveTab(t *Tab, i int) {

At first glance looks good. After all, TabList already has AddTab() and RemoveTab(), so this would be "consistent" with that.

This assumes that whenever we want to move a tab, we also want to activate it. Why? There may be cases when we don't want to do that (especially if this may be used by an arbitrary plugin, with arbitrary use cases).

Yes, the idea was that in go we always want to call Tabs.SetActive(i) after moving a tab, so I added it for convenienceand. On lua if we wanted only move we could use TabList.Move(). By your comment I understand this isn't a good idea so if we keep Tab.Move() will be just a wrapper pass-through.

so it should be already possible for plugins to activate a tab, via Tabs():SetActive(i)?

Should be

Or if we want to make it "friendlier", we might add another Tab method, i.e. Activate()?

Not sure, AFAICS it would only be useful to lua rn. Overall there seems to be a hierarchic on methods, i.g. TabList>Tab>BP, that would "break" as Tab.Move does. It's up to you whether it makes sense to include these kinds of wrappers for convenience.

It's up to you whether it makes sense to include these kinds of wrappers for convenience.

I see no necessity for that.

dmaluka · 2025-07-06T16:34:20Z

internal/action/tab.go

 		case tcell.ButtonNone:
+			if !t.release {
+				t.release = true
+			}


nit: we can just unconditionally set it to true (like we do in Tab.HandleEvent()).

cutelisp · 2025-07-08T11:56:55Z

internal/action/command.go

-	if len(args[0]) <= 0 {
-		InfoBar.Error("Invalid argument: empty string")
+	if len(args) < 1 {
+		InfoBar.Error("Not enough arguments")


"Not enough arguments" as requested here #3783 (comment)

That case was slightly different... In this case, we are deliberately making the existing error message less informative?

Why is it different, just because this is a refactor instead of a new function? I think ideally we want to reach consistency among all commands, right? I think for most of commands "Not enough arguments" is enough but I agree when the arg is invalid makes sense to provide a specific error message.

Here instead of Invalid argument we could use "Invalid tab index"?

I'm not against making the error messages of set, setlocal, toggle, togglelocal, reset more informative as well, and consistent with this one (e.g. "Not enough arguments: provide an option name and value" and so on).

Here instead of Invalid argument we could use "Invalid tab index"?

I wasn't talking about "Invalid argument: empty string", you've removed it and that's ok, it is already covered by the strconv.Atoi() check below, and it is indeed not clear why such an edge case as an empty string would deserve a dedicated error message.

right, does "Not enough arguments: provide a tab position" & "Invalid tab position" sound good or
"~~position~~index" would be better?

Both seem ok to me.

cutelisp · 2025-07-08T11:59:47Z

internal/action/command.go

 	if err != nil {
-		InfoBar.Error("Invalid argument: ", err)


Do we really want to show an internal go error to the user?
e.g.
The command tabswitch +aaa shows the error "Invalid argument: strconv.Atoi: parsing "+asda": invalid syntax"

(This happens in other functions)

Good point, I don't think we want to show these internal Go guts to the user. But we could strip most of these via errors.Unwrap(). E.g.:

InfoBar.Error("Invalid tab index ", args[0], ": ", errors.Unwrap(err))

Then, tabmove aaa will show "Invalid tab index aaa: invalid syntax", and for example tabmove 10000000000000000000000000000 will show "Invalid tab index 10000000000000000000000000000: value out of range".

cutelisp · 2025-07-08T12:01:17Z

internal/action/command.go

 		return
 	}

-	// Preserve sign for relative move, if one exists
-	var shiftDirection byte
-	if strings.Contains("-+", string([]byte{args[0][0]})) {


Tried to simplify this a bit

Looks good. Although for the future: such extra refactoring would be better done in a separate commit.

dmaluka · 2025-07-12T14:46:42Z

internal/action/command.go

-	if len(args[0]) <= 0 {
-		InfoBar.Error("Invalid argument: empty string")
+	if len(args) < 1 {
+		InfoBar.Error("Not enough arguments")


That case was slightly different... In this case, we are deliberately making the existing error message less informative?

dmaluka · 2025-07-12T14:51:12Z

internal/action/command.go

 		return
 	}

-	// Preserve sign for relative move, if one exists
-	var shiftDirection byte
-	if strings.Contains("-+", string([]byte{args[0][0]})) {


Looks good. Although for the future: such extra refactoring would be better done in a separate commit.

dmaluka · 2025-07-12T14:53:34Z

internal/action/tab.go

+
+	// captures whether the mouse is released
+	release bool
+	// captures whether the mouse is released


?

"captures whether a tab is being dragged"?

dmaluka · 2025-07-12T15:16:25Z

internal/action/tab.go

+						}
+					}
+					if t.dragging {
+						Tabs.MoveTab(MainTab(), i)


We do all this (remove the tab from the list, add it to the list again etc) every time we slightly drag the mouse, even if the tab is already as this index (so it doesn't result in moving the tab)?

i.e. we should probably check for i != t.Active()?

Also, it's a bit inconsistent that here we are using MainTab() while in other places in HandleEvent() we are directly using t.List[t.Active()]? (Actually it seems more correct to use t.List[t.Active()] rather than MainTab() here, since it is more correct to use t rather than Tabs?)

Agree on both, nice catches

dmaluka · 2025-07-12T15:18:37Z

internal/action/tab.go

+					}
+					if t.dragging {
+						Tabs.MoveTab(MainTab(), i)
+						Tabs.SetActive(i)


We do this in both cases (dragging=true and dragging=false), so we can move this out of the if?

...And we should use t here, not Tabs?

dmaluka · 2025-07-12T15:21:36Z

internal/action/tab.go

+						if t.dragging {
+							i = len(t.List) - 1
+						} else {
+							//Non-tab click/drag


It's just non-tab click here, not drag? Or move this comment before if t.dragging?

Also nit: add space after //?

dmaluka · 2025-07-12T15:55:35Z

internal/action/tab.go

+			wasReleased := t.release
+			t.release = false
+			if !wasReleased && !t.dragging {
+				//Non-tabbar dragging


nit: add space after //?

dmaluka · 2025-07-12T16:03:18Z

internal/action/tab.go

 					}
 				}
 				return
 			}
+			if wasReleased {
+				t.dragging = false


What is this for?

dmaluka · 2025-07-12T16:26:15Z

internal/action/command.go

 	if err != nil {
-		InfoBar.Error("Invalid argument: ", err)


Good point, I don't think we want to show these internal Go guts to the user. But we could strip most of these via errors.Unwrap(). E.g.:

InfoBar.Error("Invalid tab index ", args[0], ": ", errors.Unwrap(err))

Then, tabmove aaa will show "Invalid tab index aaa: invalid syntax", and for example tabmove 10000000000000000000000000000 will show "Invalid tab index 10000000000000000000000000000: value out of range".

dmaluka · 2025-07-12T16:33:39Z

internal/action/tab.go

@@ -399,3 +434,14 @@ func (t *Tab) CurPane() *BufPane {
 	}
 	return p
 }
+
+// MoveTab moves the specified tab to the given index
+func (tl *TabList) MoveTab(t *Tab, i int) {


It's better to put this method not at the very end of tab.go but together with other TabList methods (for example, after AddTab() or RemoveTab())?

cutelisp · 2025-07-12T23:45:44Z

By the time you made the review I already had refactor most of this, thanks for the feedback anyways it will be helpful.

I was adding the features of deleting and creating tab but to make verifies it was getting messy with lots of variables inside the struct.

Note that here I tried to enhance TB as a whole i found this #3417 which addresses the problems. If we want to make sure that drags on TB don't mess with buffers and vice versa + adding TB features such as scrolling, dragging etc. adding t.actionName = T/F for each "action" is not ideal IMO. I was trying more elegant ways to implement this. I think I should probably change the name of the PR for something more general and start by refactoring (t *TabList) HandleEvent a bit.

dmaluka · 2025-07-13T00:16:01Z

I'm not sure I understand what you are talking about. #3417 doesn't address problems, it reports problems, and AFAICS all the problems it reports are already addressed by your PR in its current state? As for the features like deleting and creating tabs, they can be implemented separately as a next step (if at all), we don't need to mess with those features in the same PR (certainly not in the same commit)? And it isn't clear to me why would you need t.actionName = T/F for those features in the first place.

cutelisp · 2025-07-13T01:15:26Z

doesn't address problems, it reports problems

English issue.

all the problems it reports are already addressed by your PR in its current state?

Yes

And it isn't clear to me why would you need t.actionName = T/F for those features in the first place.

For the same reason as dragging. #3795 (comment)

we don't need to mess with those features in the same PR (certainly not in the same commit)?

Right, I just felt to add the add/remove features on top of this PR as it is I would need to refactor a bit the function so it's odd to make 2 PR in a row changing "the same thing" twice. Anyway, I might be overcomplicating this problem. I'll test a few things and update the PR. But just to be clear, is add/remove on m3 an accepted feature, feature-wise?

dmaluka · 2025-07-13T01:28:13Z

For the same reason as dragging. #3795 (comment)

I can't see why. Adding/removing a tab is triggered by a mouse click (even if it's a double click), not by a mouse move, so it doesn't depend on the global "release" state, all we need is to check if this click is on the tabbar or not, right?

But just to be clear, is add/remove on m3 an accepted feature, feature-wise?

What is m3?

cutelisp · 2025-07-13T01:58:42Z

I can't see why. Adding/removing a tab is triggered by a mouse click (even if it's a double click), not by a mouse move, so it doesn't depend on the global "release" state, all we need is to check if this click is on the tabbar or not, right?

Sounds right

What is m3?

mouse3 -> mouse middle/scroll click

dmaluka · 2025-07-13T09:42:12Z

mouse3 -> mouse middle/scroll click

Ok, so, closing tab when clicking the middle button on this tab, and opening a new tab when clicking the middle button on the empty area of the tabbar? Sounds good to me, why not.

This commits add a new function `(tl *TabList) MoveTab(t *Tab, i int)` `MoveTab` moves the specified tab to the given index in TabList.List

- Simplified the overall logic. - Improved error messages. - Moved tab movement responsibility to a `(tl *TabList) MoveTab` call - Enhanced the function description.

- Now `Tablist`tracks whether a mouse is released and whether last mouse click occurred on the tab bar. - Prevents any action from propagating to the active tab if the mouse is held from an initial tab bar click. - Explicitly disables all dragging on the tab bar. ref: zyedidia#3417

Adds a new tab if the tab bar is middle mouse clicked in an empty spot (no tab), and sets it as active.

cutelisp · 2025-07-15T06:22:16Z

It's not clear to me what is the best behavior for deleting on middle click perhaps someone can give their opinion.

If a tab contains only unmodified buffers, the behavior is straightforward, we can simply delete the tab on click.

But what should happen if the tab contains modified buffers?

Should we show a save prompt for each modified buffer individually, or group them?
If the tab has a mix of modified and unmodified buffers, should the unmodified ones be closed immediately on click, or only when (and if) the entire tab is confirmed for removal?
Could we show a single prompt like: “Do you want to discard the changes in X modified buffers?”

dmaluka · 2025-07-15T21:26:39Z

But what should happen if the tab contains modified buffers? [...]

Interesting dilemma... From a pure usability standpoint, I'm really not sure which option is the best one. From simplicity point view, the last option (a single prompt, all or nothing) seems the most attractive (and it seems not bad from the usability standpoint either)?

cutelisp · 2025-07-15T21:40:12Z

the last option (a single prompt, all or nothing) seems the most attractive

Right, I was leaned towards that option too

Whenever a non-tab double-click occurs within the tab bar, a new tab is added and set as active. - `TabList.tbLastClick` holds the timestamp of the last non-tab click within the tab bar - After adding a new tab, `TabList.tbLastClick` is reset to its zero value to avoid chaining unintended double-clicks

Removes a tab when middle-clicked. If the tab contains at least one dirty buffer, a prompt is shown. If the user cancels the prompt, they remain on the tab; otherwise, the previously active tab is restored.

cutelisp · 2025-07-16T23:29:18Z

As discussed above added new tab bar features: add, delete & drag.
Made separated commits for review process.

cutelisp · 2025-07-16T23:37:44Z

internal/action/tab.go

+						t.release = true
+						t.tbClick = false
+						t.tabDrag = false


Since we call YNPrompt on click, the release is handled by infobar and not by Tablist.
I assumed that when the user exits the prompt, they've already released mouse3, this might be untrue though. The user could enter and leave the prompt while still holding mouse3.. Not sure if it's worth solving this.

cutelisp · 2025-07-16T23:39:36Z

internal/action/tab.go

+						for _, p := range panes {
+							switch t := p.(type) {
+							case *BufPane:
+								t.ForceQuit()
+							case *RawPane:
+								t.Quit()
+							case *TermPane:
+								t.Quit()
+							}
+						}


Not sure if this is the best approach. Perhaps we could add Quit to Pane struct? It seems every Pane uses it.

cutelisp · 2025-07-16T23:46:13Z

internal/action/tab.go

@@ -147,9 +151,20 @@ func (t *TabList) HandleEvent(event tcell.Event) {
 				case t.Width - 1:
 					t.Scroll(4)


I think here we could enhance scroll by making a continuous scroll on mouse hold? otherwise we should disable scroll on arrows while tab dragging - it looks silly.

cutelisp marked this pull request as ready for review July 1, 2025 10:24

dmaluka reviewed Jul 6, 2025

View reviewed changes

cutelisp force-pushed the dragtab branch from bdc21e9 to a5d1f65 Compare July 8, 2025 11:53

cutelisp commented Jul 8, 2025

View reviewed changes

dmaluka reviewed Jul 12, 2025

View reviewed changes

cutelisp added 4 commits July 14, 2025 15:56

Add MoveTab Function

4d3eb56

This commits add a new function `(tl *TabList) MoveTab(t *Tab, i int)` `MoveTab` moves the specified tab to the given index in TabList.List

Refactor TabMoveCmd()

5718281

- Simplified the overall logic. - Improved error messages. - Moved tab movement responsibility to a `(tl *TabList) MoveTab` call - Enhanced the function description.

New Tab Bar Feature: Add Tab on Mouse Middle Click

bbaf523

Adds a new tab if the tab bar is middle mouse clicked in an empty spot (no tab), and sets it as active.

cutelisp added 3 commits July 15, 2025 23:37

New Tab Bar Feature: Drag Tabs

22ef9e9

New Tab Bar Feature: Remove Tab on Mouse Middle Click

5d5326c

Removes a tab when middle-clicked. If the tab contains at least one dirty buffer, a prompt is shown. If the user cancels the prompt, they remain on the tab; otherwise, the previously active tab is restored.

cutelisp force-pushed the dragtab branch from a5d1f65 to 5d5326c Compare July 16, 2025 23:16

cutelisp changed the title ~~Add Drag Tab Feature~~ Enhance Tab Bar Jul 16, 2025

cutelisp commented Jul 16, 2025

View reviewed changes

		@@ -147,9 +151,20 @@ func (t *TabList) HandleEvent(event tcell.Event) {
		case t.Width - 1:
		t.Scroll(4)

Enhance Tab Bar #3795

Are you sure you want to change the base?

Enhance Tab Bar #3795

Conversation

cutelisp commented Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cutelisp Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Andriamanitra Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cutelisp Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

cutelisp commented Jul 1, 2025 •

edited

Loading

cutelisp Jul 8, 2025 •

edited

Loading

Andriamanitra Jul 8, 2025 •

edited

Loading

cutelisp Jul 7, 2025 •

edited

Loading