Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bindings: Allow raw escape sequence to be bound with bind #2959

Merged
merged 2 commits into from
Mar 14, 2024

Conversation

JoeKar
Copy link
Collaborator

@JoeKar JoeKar commented Oct 11, 2023

This will allow to use e.g. bind "\x1b[D" "lua:comment.comment" from within micro directly. The respective sequence will then be implicitly converted into \u notation when it's stored to the bindings.json.

#2950

@@ -258,6 +258,9 @@ func TryBindKey(k, v string, overwrite bool) (bool, error) {
return false, errors.New("Error reading bindings.json: " + err.Error())
}

// Convert possible received string of raw escape sequence
k = strings.ReplaceAll(k, "\\x1b", "\x1b")
Copy link
Collaborator

@dmaluka dmaluka Oct 14, 2023

Choose a reason for hiding this comment

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

TryBindKey() is also used from Lua, so this escaping might mess up with Lua's own escaping of its strings. So perhaps it's better to do it in BindCmd(), not here? (i.e. do it only on the UI level, specifically for the bind command)

(I see that, although Lua >=5.2 supports \x hex escaping in its strings, it doesn't work in micro. The reason is that gopher-lua implements only Lua 5.1, as it explicitly states. But it may change in the future...)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, what about the unbind command?

Copy link
Collaborator Author

@JoeKar JoeKar Oct 15, 2023

Choose a reason for hiding this comment

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

TryBindKey() is also used from Lua, so this escaping might mess up with Lua's own escaping of its strings. So perhaps it's better to do it in BindCmd(), not here?

We can do so, but we've to keep in mind, that the Lua plugins will then most probably have the same "issue" that it can't simply forward "\x1b" as a string, because this will be seen by micro as "\\x1b"...at least I expect so.

Also, what about the unbind command?

Again, you right and I should receive the 🤦. I adapted accordingly with the topping to reflect the unbind in tcell too, but this now is an API extension (test build failed by intention) and thus needs zyedidia/tcell#25 merged first.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can do so, but we've to keep in mind, that the Lua plugins will then most probably have the same "issue" that it can't simply forward "\x1b" as a string, because this will be seen by micro as "\x1b"...at least I expect so.

IIUC, not really. I think what happens is:

  1. If we don't do escaping in TryBindKey (i.e. as it works now), Lua plugins cannot use the \x notation at all, i.e. neither "\x1b" nor "\\x1b".
  2. If we do escaping in TryBindKey, then Lua plugins can use "\\x1b" but not "\x1b", because this escaping is not at the Lua language level, it's an extra level of escaping, while Lua has its own escaping, i.e. \ is a special character in Lua too, so need to escape it too. IMO this extra level of quoting creates mess and confusion.
  3. As I said, the reason why Lua plugins cannot currently use \x is that gopher-lua does not implement it. But it is supported in newer Lua versions. So if we care about it so much, I think we'd better add the \x support to gopher-lua too, it should not be too hard. And then plugins would be able to use "\x1b", not "\\x1b", and also it would be consistent with other Lua implementations.
  4. In the meantime Lua plugins can use Lua's decimal \ddd notation, i.e. "\27" for escape sequences.

Anyway, I see you've already moved it from TryBindKey to BindCmd. Just a pedantic note: you did it in the next (unrelated) commit, which is confusing.

Regarding unbind: thanks, I didn't realize it requires unregistering raw sequences in tcell as well, I thought we only need to add parsing "\x1b" to UnbindCmd as well.

BTW, side note: unbind is currently buggy: unbinding does not take effect immediately, i.e. it only removes the binding from bindings.json, so it only takes effect after micro restart, when micro reads bindings.json again. (This is a regression, but this regression has been there for like 3 years (probably starting from d33c28e, which made BufUnmap() do nothing). I was always about to fix it, but it has always been low priority...)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Shit, there is also the showkey command...

(If fix it too, it's time for a tiny little wrapper function for strings.ReplaceAll(args[0], "\\x1b", "\x1b") to avoid duplicating it in 3 places?..)

Copy link
Collaborator Author

@JoeKar JoeKar Oct 15, 2023

Choose a reason for hiding this comment

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

Anyway, I see you've already moved it from TryBindKey to BindCmd. Just a pedantic note: you did it in the next (unrelated) commit, which is confusing.

Done.

Shit, there is also the showkey command...

Now you're using the right words! 😆
Especially since it was just one function above the changed ones. Never mind, it's fixed too.

BTW, side note: unbind is currently buggy

Hm, sounds like a good moment to fix this as well, but doesn't seem to be a one liner.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, sounds like a good moment to fix this as well, but doesn't seem, to be a one liner.

Yep, and I'm afraid that fixing it properly requires a good grasp of the overall micro's keybindings implementation (KeyTree and all that stuff), which is quite intricate. So don't feel obliged to do it.

internal/action/command.go Outdated Show resolved Hide resolved
internal/action/command.go Outdated Show resolved Hide resolved
internal/action/command.go Outdated Show resolved Hide resolved
@JoeKar JoeKar force-pushed the fix/raw-esc-sequence branch 2 times, most recently from b721548 to 041844d Compare October 16, 2023 18:00
@dmaluka
Copy link
Collaborator

dmaluka commented Oct 16, 2023

zyedidia/tcell#25 got merged, so now in order to merge this PR, need to bump tcell version in go.mod and go.sum.

@dmaluka
Copy link
Collaborator

dmaluka commented Oct 16, 2023

zyedidia/tcell#25 got merged, so now in order to merge this PR, need to bump tcell version in go.mod and go.sum.

Sorry, you've already done it...

go.mod Outdated Show resolved Hide resolved
internal/action/command.go Outdated Show resolved Hide resolved
@JoeKar
Copy link
Collaborator Author

JoeKar commented Oct 22, 2023

Removed the tcell bump commit within this PR, since @zyedidia did it by his own with 68d88b5.

@dmaluka dmaluka merged commit 80db98d into zyedidia:master Mar 14, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants