Improve return values of some actions + some improvements #3352
Conversation
Return false if there is nothing to undo/redo. This also fixes false "Undid action" and "Redid actions" infobar messages in the case when no action was actually undone or redone.
|
Besides the actions improved in this PR, there are also lots of cursor movement and selection actions ( But that is more complicated and debatable. For instance,
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. |
ClearInfo and ClearStatus actions do exactly the same thing. Let's keep them both, for compatibility reasons (who knows how many users are using either of the two), but at least document that there is no difference between the two.
When there is no selection (i.e. selection is empty), SkipMultiCursor searches for the empty text, "finds" it as the beginning of the buffer, and as a result, jumps to the beginning of the buffer, which confuses the user. Fix it.
If the original selection was not done by the user manually but as a result of the initial SpawnMultiCursor, deselect this original selection if we execute RemoveMultiCursor and there is no multicursor to remove (i.e. the original spawned cursor is the only one). This improves user experience by making RemoveMultiCursor behavior nicely symmetrical to SpawnMultiCursor.
Use Deselect() in order to place the cursor at the beginning of the selection, not at the end of it, and to refresh its LastVisualX.
- SpawnMultiCursor and RemoveMultiCursor actions change the set of cursors, so we cannot assume that it stays the same. So refresh the `cursors` list after executing every action in the chain. - If execAction() did not execute an action since it is not a multicursor, it should return true, not false, to not prevent executing next actions in the chain.
Instead of calling execAction() and then letting it check whether it should actually execute this action, do this check before calling execAction(), to make the code clear and straightforward. Precisely: for multicursor actions, call execAction() in a loop for every cursor, but for non-multicursor actions, call execAction() just once, without a loop. This, in particular, allows to get rid of the hacky "c == nil" check, since we no longer iterate a slice that may change in the meantime (since SpawnMultiCursor and RemoveMultiCursor are non-multicursor actions and thus are no longer executed while iterating the slice).
dmaluka
force-pushed
the
action-return-values
branch
from
1d30794
to
1785fea
Compare
There was a problem hiding this comment.
Amazing refactoring! It is way clearer now what the code is suppose to do. And you squashed a bug along the way. In the original implementation the correct cursors reference gets lost after switching to a different BufPane.
| @@ -150,29 +150,31 @@ func BufMapEvent(k Event, action string) { | |||
| actionfns = append(actionfns, afn) | |||
| } | |||
| bufAction := func(h *BufPane, te *tcell.EventMouse) bool { | |||
| cursors := h.Buf.GetCursors() | |||
There was a problem hiding this comment.
The cursors assignment would need to be moved 2 lines down into the actions loop. Otherwise you would still use the cursors of the original pane even though you switched to a new one in a next iteration. Line 170 updates the reference to h. 👍
|
I verified all the following methods to work correctly. I noticed that there is no counterpart for Not sure if this is the right time, but I would suggest a change for 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 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? "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 |
Yeah, it might be good to add
Yeah, sounds good. |
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:
UndoRedoDeselectUnhighlightSearchClearInfoClearStatusNextTabPreviousTabNextSplitPreviousSplitRemoveMultiCursorRemoveAllMultiCursorsThe rules are straightforward:
Undoreturns false if there is nothing to undo,Deselectreturn false if there is nothing selected,NextTabreturns false if there is just one tab, and so on.Additionally:
RemoveMultiCursor,RemoveAllMultiCursorsandSkipMultiCursoractions.RemovedI've changed my mind and restored it (compatibility concerns) but documented that it is an alias forClearStatusaction as redundant (sinceClearInfodoes the same things).ClearInfo.