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

Make parse_macro work for "-" outside "<..>" #8475

Merged
merged 2 commits into from
Oct 12, 2023

Conversation

bjorn-ove
Copy link
Contributor

When running the integration tests, I ended up with temp file paths containing "-" in the name. This in turn triggered an issue where the key "" could not be resolved. This is one way of fixing this issue, another is to check for "-" in parse_macro() and replace it with "minus".

@kirawi kirawi added the A-helix-term Area: Helix term improvements label Oct 6, 2023
Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

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

- isn't a valid part of a key sequence though, it's only used for connecting multiple keys into a chord like C-c, and minus stands for the - key. So parsing it into a KeyEvent or in parse_macro isn't correct.

Instead we could encode filenames as key sequences (so a-b.txt would become a<minus>b.txt). It might be possible to use <C-r>% to enter the buffer name into the prompt in the test cases instead though which would be simpler.

@the-mikedavis the-mikedavis added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 6, 2023
@bjorn-ove
Copy link
Contributor Author

This was my second solution to the issue. I wasn't sure. Thanks for the feedback. Will fix when I'm back after the weekend.

@bjorn-ove
Copy link
Contributor Author

bjorn-ove commented Oct 9, 2023

I understand why - is not valid for KeyEvent::from_str, but not why it also applies to parse_macro(). In the latter case, C-a and similar is always inside <..>. So, I passing - to parse_macro() does not appear ambiguous or anything like that.

For example parse_macro("BigC-world") in my head should be the letters as they are, while parse_macro("Big<C-w>orld") would be the way when - matters.

So, because of this it feels to me that the correct solution would be to check for - in parse_macro() and translate it to minus.

What am I missing here?

@bjorn-ove bjorn-ove changed the title Made KeyEvent::from_str work for "-" Made parse_macro work for "-" outside "<..>" Oct 9, 2023
Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

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

I was mostly worried about KeyEvent, it seems ok for parse_macro. I would prefer that converting to/from the key sequence format be round-trip, so parsing then encoding a key sequence would give the input string. Being able to use - in a macro should be very convenient though so I'm ok with the change here.

The solution I had in mind was to encode the path in terms of a key sequence. Other than minus I don't think we have any special characters that are also valid in path names though so I don't think we really need that solution.

helix-view/src/input.rs Outdated Show resolved Hide resolved
@the-mikedavis the-mikedavis changed the title Made parse_macro work for "-" outside "<..>" Make parse_macro work for "-" outside "<..>" Oct 11, 2023
@the-mikedavis the-mikedavis added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 11, 2023
@pascalkuthe pascalkuthe merged commit 574f821 into helix-editor:master Oct 12, 2023
6 checks passed
@bjorn-ove bjorn-ove deleted the fix-keyevent-minus branch October 12, 2023 15:03
danillos pushed a commit to danillos/helix that referenced this pull request Nov 21, 2023
* Translate  to   when a part of the outher string in

* Changed the if a little
dgkf pushed a commit to dgkf/helix that referenced this pull request Jan 30, 2024
* Translate  to   when a part of the outher string in

* Changed the if a little
mtoohey31 pushed a commit to mtoohey31/helix that referenced this pull request Jun 2, 2024
* Translate  to   when a part of the outher string in

* Changed the if a little
smortime pushed a commit to smortime/helix that referenced this pull request Jul 10, 2024
* Translate  to   when a part of the outher string in

* Changed the if a little
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants