Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
50 changes: 11 additions & 39 deletions internal/action/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -160,55 +160,27 @@ func (h *BufPane) TextFilterCmd(args []string) {
}
}

// TabMoveCmd moves the current tab to a given index (starts at 1). The
// displaced tabs are moved up.
// TabMoveCmd moves the current tab to a given index (starting at 1),
// or by a relative offset using +N/-N. Displaced tabs shift accordingly.
func (h *BufPane) TabMoveCmd(args []string) {
if len(args) <= 0 {
InfoBar.Error("Not enough arguments: provide an index, starting at 1")
return
}

if len(args[0]) <= 0 {
InfoBar.Error("Invalid argument: empty string")
if len(args) < 1 {
InfoBar.Error("Not enough arguments: provide a tab position")
return
}

num, err := strconv.Atoi(args[0])
i, err := strconv.Atoi(args[0])
if err != nil {
InfoBar.Error("Invalid argument: ", err)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Collaborator

Choose a reason for hiding this comment

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

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".

InfoBar.Error("Invalid tab position")
return
}

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

Choose a reason for hiding this comment

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

Tried to simplify this a bit

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

shiftDirection = args[0][0]
}

// Relative positions -> absolute positions
idxFrom := Tabs.Active()
idxTo := 0
offset := util.Abs(num)
if shiftDirection == '-' {
idxTo = idxFrom - offset
} else if shiftDirection == '+' {
idxTo = idxFrom + offset
if strings.ContainsRune("-+", rune(args[0][0])) {
i = util.Clamp(i+Tabs.Active(), 0, len(Tabs.List)-1)
} else {
idxTo = offset - 1
i = util.Clamp(i-1, 0, len(Tabs.List)-1)
}

// Restrain position to within the valid range
idxTo = util.Clamp(idxTo, 0, len(Tabs.List)-1)

activeTab := Tabs.List[idxFrom]
Tabs.RemoveTab(activeTab.Panes[0].ID())
Tabs.List = append(Tabs.List, nil)
copy(Tabs.List[idxTo+1:], Tabs.List[idxTo:])
Tabs.List[idxTo] = activeTab
Tabs.Resize()
Tabs.UpdateNames()
Tabs.SetActive(idxTo)
// InfoBar.Message(fmt.Sprintf("Moved tab from slot %d to %d", idxFrom+1, idxTo+1))
Tabs.MoveTab(MainTab(), i)
Tabs.SetActive(i)
}

// TabSwitchCmd switches to a given tab either by name or by number
Expand Down
162 changes: 138 additions & 24 deletions internal/action/tab.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package action

import (
luar "layeh.com/gopher-luar"
"time"

"github.com/micro-editor/tcell/v2"
"github.com/zyedidia/micro/v2/internal/buffer"
Expand All @@ -17,6 +18,15 @@ import (
type TabList struct {
*display.TabWindow
List []*Tab

// captures whether the mouse is released
release bool
// captures whether the last mouse click occurred on the tab bar
tbClick bool
// captures whether a tab is being dragged within the tab bar
tabDrag bool
// timestamp of the last non-tab click within the tab bar
tbLastClick time.Time
}

// NewTabList creates a TabList from a list of buffers by creating a Tab
Expand All @@ -35,6 +45,7 @@ func NewTabList(bufs []*buffer.Buffer) *TabList {
}
tl.TabWindow = display.NewTabWindow(w, 0)
tl.Names = make([]string, len(bufs))
tl.release = true

return tl
}
Expand Down Expand Up @@ -75,6 +86,19 @@ func (t *TabList) RemoveTab(id uint64) {
}
}

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

Choose a reason for hiding this comment

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

Use the same naming style for arguments as in other TabList methods? i.e.

 func (t *TabList) MoveTab(p *Tab, i int)

if i == tl.Active() || i < 0 || i >= len(tl.List) {
return
}
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()
}

// Resize resizes all elements within the tab list
// One thing to note is that when there is only 1 tab
// the tab bar should not be drawn so resizing must take
Expand Down Expand Up @@ -105,40 +129,130 @@ func (t *TabList) HandleEvent(event tcell.Event) {
t.Resize()
case *tcell.EventMouse:
mx, my := e.Position()
switch e.Buttons() {
case tcell.Button1:
if my == t.Y && len(t.List) > 1 {
if mx == 0 {
t.Scroll(-4)
} else if mx == t.Width-1 {
t.Scroll(4)
} else {
ind := t.LocFromVisual(buffer.Loc{mx, my})
if ind != -1 {
t.SetActive(ind)
}
}
return
}
case tcell.ButtonNone:
if e.Buttons() == tcell.ButtonNone {
t.release = true
t.tbClick = false
t.tabDrag = false
if t.List[t.Active()].release {
// Mouse release received, while already released
t.ResetMouse()
return
}
case tcell.WheelUp:
if my == t.Y && len(t.List) > 1 {
} else if my == t.Y && len(t.List) > 1 {
switch e.Buttons() {
case tcell.Button1:
if !t.release && !t.tabDrag {
// Invalid tab bar dragging
return
}
isDrag := !t.release
t.release = false
t.tbClick = true
switch mx {
case 0:
t.Scroll(-4)
case t.Width - 1:
t.Scroll(4)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, it seems better to disable scroll on arrows while tab dragging.

As for scrolling on mouse hold, I'm not against that in principle, but that's not so trivial to implement...

default:
i := t.LocFromVisual(buffer.Loc{mx, my})
if i != -1 {
t.tabDrag = true
if i != t.Active() {
if isDrag {
t.MoveTab(t.List[t.Active()], i)
}
t.SetActive(i)
}
} else {
if isDrag {
if i = len(t.List) - 1; i != t.Active() {
t.MoveTab(t.List[t.Active()], i)
t.SetActive(i)
}
} else {
if time.Since(t.tbLastClick)/time.Millisecond < config.DoubleClickThreshold {
t.List[t.Active()].CurPane().AddTab()
t.tbLastClick = time.Time{}
} else {
t.tbLastClick = time.Now()
}
}
}
}
case tcell.Button3:
if !t.release {
// Tab bar dragging
return
}
t.release = false
t.tbClick = true

if i := t.LocFromVisual(buffer.Loc{mx, my}); i == -1 {
t.List[t.Active()].CurPane().AddTab()
} else {
anyModified := false
for _, p := range t.List[i].Panes {
if bp, ok := p.(*BufPane); ok {
if bp.Buf.Modified() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This produces false negatives if the same buffer is also opened in another pane in another tab (so closing this tab would not cause losing unsaved changes in this buffer).

The similar problem was addressed in #3559 + #3719 by adding Shared() which checks if the buffer is opened in more than one pane. Unfortunately we cannot just reuse Shared() here, since we need to distinguish the case when all the panes with this buffer are in the tab we are closing (then we need to prompt the user) and the case when at least one such pane is in a different tab (then we don't need to prompt the user).

anyModified = true
break
}
}
}

removeTab := func() {
panes := append([]Pane(nil), t.List[i].Panes...)
for _, p := range panes {
switch t := p.(type) {
case *BufPane:
t.ForceQuit()
case *RawPane:
t.Quit()
case *TermPane:
Copy link
Collaborator

Choose a reason for hiding this comment

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

We are closing the terminal pane without prompting the user. Whereas the user might run anything at all in this terminal, for example another instance of micro, with some unsaved changes, lol.

I don't really like the fact that we support this embedded terminal functionality (which is doomed to never be on par with proper terminals), and I never used it myself, but apparently a lot of users do use it (as they often report bugs about this or that terminal functionality not working in it)...

t.Quit()
}
}
Comment on lines +205 to +214
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

a := t.Active()
if anyModified {
t.SetActive(i)
InfoBar.YNPrompt("Discard unsaved changes? (y,n,esc)", func(yes, canceled bool) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The user may middle-click the tab name accidentally and may not be aware that it closes the tab, so the user may not know why this prompt is being shown and which exact unsaved changes (in which buffers) are going to be discarded.

So we'd better make it more clear, at least something like "Discard unsaved changed in this tab?"?

if !canceled {
if yes {
removeTab()
if i <= a {
a--
Copy link
Collaborator

Choose a reason for hiding this comment

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

This causes a crash when closing a tab which is the first tab in the tab bar and which is currently active.

Micro encountered an error: runtime.boundsError runtime error: index out of range [-1]
runtime/panic.go:115 (0x43ccb4)
github.com/zyedidia/micro/v2/internal/action/tab.go:338 (0x90a68f)
github.com/zyedidia/micro/v2/cmd/micro/micro.go:467 (0x90a0d2)
github.com/zyedidia/micro/v2/cmd/micro/micro.go:455 (0x909e70)
internal/runtime/atomic/types.go:194 (0x441f4b)
runtime/asm_amd64.s:1700 (0x47d341)

And besides that, when closing an active tab, it is more intuitive to activate the tab after it, not the tab before it (except the case when there is no tab after it)?

So, change i <= a for example to i < a || (i == a && a > 0 && a == len(t.List)-1) (both here and at line 236)?

}
}
t.SetActive(a)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We return to the original active tab if the user pressed n but not if the user pressed esc? Why?

}
})
t.release = true
t.tbClick = false
t.tabDrag = false
Comment on lines +231 to +233
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

} else {
removeTab()
if i <= a {
t.SetActive(a - 1)
}
}
return
Copy link
Collaborator

Choose a reason for hiding this comment

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

This return seems redundant?

}
case tcell.WheelUp:
t.Scroll(4)
return
}
case tcell.WheelDown:
if my == t.Y && len(t.List) > 1 {
case tcell.WheelDown:
t.Scroll(-4)
return
}
return
} else if t.release {
// Click outside tab bar
t.release = false
}
}
t.List[t.Active()].HandleEvent(event)
if t.release || !t.tbClick {
t.List[t.Active()].HandleEvent(event)
}
}

// Display updates the names and then displays the tab bar
Expand Down
Loading