Fixing commandbar autocomplete regression from #4028 - #4200
Fixing commandbar autocomplete regression from #4028#4200Neko-Box-Coder wants to merge 1 commit into
Conversation
|
From @Andriamanitra's #4028 (comment):
We could solve both cases, as we did in the past multiple times: EDIT: diff --git a/internal/action/actions.go b/internal/action/actions.go
index ebd51899..2ad2a0a2 100644
--- a/internal/action/actions.go
+++ b/internal/action/actions.go
@@ -916,7 +916,7 @@ func (h *BufPane) Autocomplete() bool {
return true
}
- return b.Autocomplete(buffer.BufferComplete)
+ return b.AutocompleteCheck(buffer.BufferComplete, true)
}
// CycleAutocompleteBack cycles back in the autocomplete suggestion list
diff --git a/internal/action/infopane.go b/internal/action/infopane.go
index 6ecd9552..2537873c 100644
--- a/internal/action/infopane.go
+++ b/internal/action/infopane.go
@@ -192,7 +192,7 @@ func (h *InfoPane) HistorySearchDown() {
func (h *InfoPane) CommandComplete() {
b := h.Buf
if b.HasSuggestions {
- b.CycleAutocomplete(true)
+ b.CycleAutocompleteCheck(true, false)
return
}
@@ -205,15 +205,15 @@ func (h *InfoPane) CommandComplete() {
if h.PromptType == "Command" {
if len(args) == 1 {
- b.Autocomplete(CommandComplete)
+ b.AutocompleteCheck(CommandComplete, false)
} else if action, ok := commands[cmd]; ok {
if action.completer != nil {
- b.Autocomplete(action.completer)
+ b.AutocompleteCheck(action.completer, false)
}
}
} else {
// by default use filename autocompletion
- b.Autocomplete(buffer.FileComplete)
+ b.AutocompleteCheck(buffer.FileComplete, false)
}
}
diff --git a/internal/buffer/autocomplete.go b/internal/buffer/autocomplete.go
index d0832557..26f83bde 100644
--- a/internal/buffer/autocomplete.go
+++ b/internal/buffer/autocomplete.go
@@ -25,7 +25,11 @@ func (b *Buffer) GetSuggestions() {
// Autocomplete starts the autocomplete process
func (b *Buffer) Autocomplete(c Completer) bool {
- if !b.GetActiveCursor().CanAutocomplete() {
+ return b.AutocompleteCheck(c, false)
+}
+
+func (b *Buffer) AutocompleteCheck(c Completer, check bool) bool {
+ if check && !b.GetActiveCursor().CanAutocomplete() {
return false
}
b.Completions, b.Suggestions = c(b)
@@ -33,12 +37,16 @@ func (b *Buffer) Autocomplete(c Completer) bool {
return false
}
b.CurSuggestion = -1
- b.CycleAutocomplete(true)
+ b.CycleAutocompleteCheck(true, check)
return true
}
// CycleAutocomplete moves to the next suggestion
func (b *Buffer) CycleAutocomplete(forward bool) {
+ b.CycleAutocompleteCheck(forward, true)
+}
+
+func (b *Buffer) CycleAutocompleteCheck(forward bool, check bool) {
prevSuggestion := b.CurSuggestion
if forward {
@@ -56,7 +64,7 @@ func (b *Buffer) CycleAutocomplete(forward bool) {
activeWord := make([]byte, len(tmpWord))
copy(activeWord, tmpWord)
for _, c := range b.cursors {
- if !c.CanAutocomplete() {
+ if check && !c.CanAutocomplete() {
continue
}
|
I will have a try at it, but I don't understand how it was working before 🤔 since we do perform the same guard when auto-completing before #4200.
Thanks for the patch. Just wondering have we ever considered versioning the functions (Since I am seeing more of this happening)? Something like |
e5f5213 to
867c0a9
Compare
|
If not those plugins, we could fix this very easily: diff --git a/internal/buffer/autocomplete.go b/internal/buffer/autocomplete.go
index d0832557..8741538d 100644
--- a/internal/buffer/autocomplete.go
+++ b/internal/buffer/autocomplete.go
@@ -86,6 +86,10 @@ func (c *Cursor) autocomplete(prevSuggestion int) {
}
func (c *Cursor) CanAutocomplete() bool {
+ if c.buf.Type == BTInfo {
+ return true
+ }
+
if c.X == 0 {
return false
} |
| if h.PromptType == "Command" { | ||
| if len(args) == 1 { | ||
| b.Autocomplete(CommandComplete) | ||
| b.AutocompleteCheck(CommandComplete, false) |
| return b.AutocompleteCheck(c, false) | ||
| } | ||
|
|
||
| func (b *Buffer) AutocompleteCheck(c Completer, check bool) bool { |
There was a problem hiding this comment.
- Yes, the name sucks.
AutocompleteWithCheck()? Or, don't know, justAutocompleteEx()or whatever? - Instead of this ad-hoc non-extensible
checkflag, why not pass a callback? We know thatbufpane.gois the only user ofAutocomplete()that wants this check (and before Add multicursor autocompletions #4028 it wasbufpane.gothat was performing this check), so, letbufpane.gopass a callback implementing this check, and let others (i.e. infopane and plugins) pass a stub callback which always returns true?
|
|
||
| // CycleAutocomplete moves to the next suggestion | ||
| func (b *Buffer) CycleAutocomplete(forward bool) { | ||
| b.CycleAutocompleteCheck(forward, true) |
There was a problem hiding this comment.
Why not be consistent with Autocomplete() and pass false by default, not true? And thereby also avoid breaking plugins that are using CycleAutocomplete() if there are any, exactly the same way and exactly for the same reasons as for Autocomplete()?
Maybe I am missing something but this works no? |
This (i.e. my "fix") only fixes the regression for the command bar, it doesn't fix the regression for plugins pointed out by @Andriamanitra, right? Frankly I didn't try to reproduce that plugins regression myself, but @Andriamanitra 's description of it sounds quite clear to me.
That |
867c0a9 to
de048f1
Compare
Ah...right, fair enough.
I hadn't read the plugin code when I was asking, but I see. Yeah, now it makes sense. @Andriamanitra It's simply because it wasn't designed to work with multi-cursor (reasonably so) judging from it does its own text replacement, and also because it was doing |
No. I've just tried that myself and I see exactly the behavior described by @Andriamanitra : with that plugin installed, before #4028, if I type 
After #4028, it doesn't. Although, the plugin's behavior doesn't seem quite friendly in this particular case when before pressing Tab we type But that's another story. #4028 still caused a regression, making this not work at all. |
|
|
||
| return b.Autocomplete(buffer.BufferComplete) | ||
| return b.AutocompleteWithCheckCB(buffer.BufferComplete, func(c *buffer.Cursor) bool { | ||
| return c.CanAutocomplete() |
There was a problem hiding this comment.
Why not move the implementation of CanAutocomplete() into actions.go so we can just pass it to AutocompleteWithCheckCB() as a callback as is? The very idea is that this CanAutocomplete() check is bufpane-specific, i.e. no one except these bufpane's Autocomplete and CycleAutocompleteBack actions will use it, thus it belongs here in the bufpane package, not in the buffer package?
| func (b *Buffer) Autocomplete(c Completer) bool { | ||
| if !b.GetActiveCursor().CanAutocomplete() { | ||
| return b.AutocompleteWithCheckCB(c, func(*Cursor) bool { | ||
| return true |
There was a problem hiding this comment.
I know it was me who suggested to "pass a stub callback which always returns true", but now looking e.g. at our SaveCB() or SaveAsCB(), we can just pass nil and let AutocompleteWithCheckCB() check it? That would make the code a bit shorter and more readable?
...While we're at it, maybe let's also rename AutocompleteWithCheckCB() to just AutocompleteCB()? That would at least make it look much less ugly.
I am confused... Do you mean #4200 or #4028 ? That quote from me was meant for this PR, not #4028 
And yes you need to get out of the autocomplete and press tab to do the transformation, which was the behavior before #4028 |
de048f1 to
50891e1
Compare
| return false | ||
| } | ||
|
|
||
| func CanAutocomplete(c *buffer.Cursor) bool { |
There was a problem hiding this comment.
No need to make this function public?
...Also regarding the commit message, it would be useful to provide a bit of details on what regression this commit is fixing. I know there are details provided in the PR description, but it is useful to be able to see the essential info in the git log right away, without the need to switch to the web browser and go to github and find the PR to read its description.
BTW now looking at that PR description, it doesn't seem to clearly describe the problem either (i.e., like, what exactly got broken from the user perspective).
50891e1 to
68916e1
Compare
PR micro-editor#4028 moved the check for autocomplete from action autocomplete to the buffer one, which caused a few regression for upstream callers (commandbar autocomplete & plugins that use buffer autocomplete) that use the buffer autocomplete. This commit restores pre micro-editor#4028 behavior by adding a callback variant for buffer autocomplete functions while keeping the multi-cursor autocomplete function.
68916e1 to
55305c1
Compare
Fixing commandbar autocomplete regression from #4028
Previously, commandcomplete calls the buffer autocomplete directly and there was an autocomplete guard in the action autocomplete.
#4028 moved the guard from action autocomplete to the buffer autocomplete, which now applies to commandcomplete which it wasn't applied before.
The causes the command bar autocomplete to not behave as before (not autocomplete until a character is entered), it also affected plugin that uses the buffer autocomplete.
This PR restores the previous behavior by adding a check callback variant that allows the caller to specify the check for autocomplete if needed.