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

Refactor macros and mark as experimental #1253

Merged
merged 7 commits into from
Dec 27, 2021
Merged

Conversation

pickfire
Copy link
Contributor

@pickfire pickfire commented Dec 12, 2021

cc @Omnikar

This does not solve the macro internal representation issues but it should be a quick fix for now for some of the issues mentioned in #1234

Leftover

  • move macro display to status bar (not sure if it's better since doom emacs move to normal status line unlike vim)
  • better internal macro representation, to be done by @Omnikar, he say in this pull request, not sure if we want to wait

@pickfire pickfire changed the title Switch macro Q and q Refactor macros Dec 12, 2021
@pickfire pickfire changed the title Refactor macros Refactor macros and mark as experimental Dec 12, 2021
@pickfire pickfire marked this pull request as draft December 12, 2021 14:06
@pickfire pickfire marked this pull request as ready for review December 12, 2021 14:26
@Omnikar
Copy link
Contributor

Omnikar commented Dec 12, 2021

Keypress delimiting has been addressed here pickfire#1

@archseer
Copy link
Member

Keypresses are no longer separated by spaces

I'm not sure we want this, since that makes < and > ambiguous.

<C-p> could either be <, C, -, p, > or ctrl-p.

@archseer
Copy link
Member

Since keys also have longer names, I thought this made them more readable: space f enter vs <space>f<enter>

@Omnikar
Copy link
Contributor

Omnikar commented Dec 13, 2021

I'm not sure we want this, since that makes < and > ambiguous.

< and > are unambiguously interpreted as delimiters, and there is already lt and gt for the keys themselves.

@pickfire
Copy link
Contributor Author

pickfire commented Dec 15, 2021

Since keys also have longer names, I thought this made them more readable: space f enter vs f

One of the reason for this patch is space f enter is not a good idea I think. It shouldn't have space in between since if user want to copy macro into register, writing spaces in between is not very natural.

And space delimited have an issue. Try Qi <esc>Qq, it says macro is empty even though you did inserted a space but somehow can't replay. Both vim and kak also don't use this.

@Omnikar But I think besides looking at <space> I think we also need to look into escape characters directly, both kakoune and codemirror converts some of them like ^M, in case user copied a newline from clipboard and it should still work like expected. Not sure if newlines work currently.

helix-term/src/commands.rs Outdated Show resolved Hide resolved
@pickfire
Copy link
Contributor Author

I guess this should be good now. But https://github.com/helix-editor/helix/runs/4528007282?check_suite_focus=true is that a spurious failure? I run cargo xtask docgen locally but there are no changes.

@archseer
Copy link
Member

You probably need to rebase to catch up with latest master

pickfire and others added 6 commits December 24, 2021 22:54
Macro needs to be defined first before playing so replay is more accurate.
Also, replay have the same length as record which makes it looks nice.
Given that currently macro does not integrate well with registers and
the internal representation of macros is expected to be changed.
* Keypresses are no longer separated by spaces
* Single-character keypresses are serialized as-is
* Multi-character keypresses are delimited by `<>`
@Omnikar
Copy link
Contributor

Omnikar commented Dec 25, 2021

So, is there anything blocking this? I'd really like to see this get merged.

Err(e) => {
cx.editor.set_error(format!("{}", e));
return;
// TODO: macro keys should be parsed one by one and not space delimited (see kak)
Copy link
Member

Choose a reason for hiding this comment

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

It generally looks good, but can we extract this to helix-view and add some unit tests?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yeah; and I'm going to need the extracted macro parsing functionality for #997 too.

Copy link
Contributor

Choose a reason for hiding this comment

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

@pickfire I have done this in pickfire#2.

@archseer archseer merged commit 8340d73 into helix-editor:master Dec 27, 2021
@pickfire pickfire deleted the macro branch December 27, 2021 05:05
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.

4 participants