zyedidia / micro

A modern and intuitive terminal-based text editor
https://micro-editor.github.io
MIT License
24.9k stars 1.17k forks source link

Improve return values of some actions + some improvements #3352

Closed dmaluka closed 2 months ago

dmaluka commented 3 months ago

Currently most actions always return true, regardless of whether they succeeded to do their job or not. This means that for most actions the user cannot really take advantage of action chaining with | or &.

So, to facilitate efficient usage of action chaining, improve return values of some actions, namely:

The rules are straightforward: Undo returns false if there is nothing to undo, Deselect return false if there is nothing selected, NextTab returns false if there is just one tab, and so on.

Additionally:

dmaluka commented 3 months ago

Besides the actions improved in this PR, there are also lots of cursor movement and selection actions (CursorUp, SelectWordLeft and so on) which also always return false and could be similarly improved. For example, CursorUp could return false if the cursor is already at the first line of the buffer.

But that is more complicated and debatable. For instance, CursorUp actually not just moves the cursor up one line, it also does the following:

All this makes it not obvious what should be its return value in what cases.

So leave those actions as they are for now, to avoid complicating things prematurely.

masmu commented 2 months ago

I verified all the following methods to work correctly.

I noticed that there is no counterpart for SkipMultiCursor. And since this action just changes selections (not text) this cannot be undone via Undo.

Not sure if this is the right time, but I would suggest a change for NextTab, PreviousTab, NextSplit and PreviousSplit. Currently those methods are topologically designed as rings. So once you reached the last (lets say a) tab, the next action gets you back to the first tab and vice versa.

My suggestion is change this topologically to a line for all mentioned methods. So e.g. in case you want to switch to the next tab, but you are already on it, return false.

After adding the following new actions:

// FirstTab switches to the first tab in the tab list
func (h *BufPane) FirstTab() bool {
    if Tabs.Active() == 0 {
        return false
    }
    Tabs.SetActive(0)
    return true
}

// LastTab switches to the last tab in the tab list
func (h *BufPane) LastTab() bool {
    lastTabIndex := len(Tabs.List) - 1
    if Tabs.Active() == lastTabIndex {
        return false
    }
    Tabs.SetActive(lastTabIndex)
    return true
}

// FirstSplit changes the view to the first split
func (h *BufPane) FirstSplit() bool {
    if h.tab.active == 0 {
        return false
    }
    h.tab.SetActive(0)
    return true
}

// LastSplit changes the view to the last split
func (h *BufPane) LastSplit() bool {
    lastTabIndex := len(h.tab.Panes) - 1
    if h.tab.active == lastTabIndex {
        return false
    }
    h.tab.SetActive(lastTabIndex)
    return true
}

you can restore the old behavior by the following settings:

    // this would be my recommendation for the defaults
    "Alt-,": "PreviousTab|LastTab",
    "Alt-.": "NextTab|FirstTab",

    // just for the sake of completeness
    "CtrlPageUp":     "PreviousTab|LastTab",
    "CtrlPageDown":   "NextTab|FirstTab",
    "Ctrl-w":         "NextSplit|FirstSplit",

But why breaking those actions into smaller actions in the first place? It adds a lot of flexibility for the user. You can use combinations of splits and tabs, like:

    "Alt-,": "PreviousSplit|PreviousTab|LastTab",
    "Alt-.": "NextSplit|NextTab|FirstTab",

Is this a breaking change? Potentially yes, but IMHO a small one. It only affects people having bindings for NextTab, PreviousTab, NextSplit and PreviousSplit in their own bindings.json.

dmaluka commented 2 months ago

I noticed that there is no counterpart for SkipMultiCursor. And since this action just changes selections (not text) this cannot be undone via Undo.

Yeah, it might be good to add SkipMultiCursorBack or something like that.

Not sure if this is the right time, but I would suggest a change for NextTab, PreviousTab, NextSplit and PreviousSplit.

[...]

It adds a lot of flexibility for the user. You can use combinations of splits and tabs, like:

[...]

Is this a breaking change? Potentially yes, but IMHO a small one. It only affects people having bindings for NextTab, PreviousTab, NextSplit and PreviousSplit in their own bindings.json.

Yeah, sounds good.

JoeKar commented 2 months ago

Looks like a small rebase is needed due to conflicts.