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

Strip prefix/suffix #52

Open
f-forcher opened this issue Mar 21, 2024 · 11 comments
Open

Strip prefix/suffix #52

f-forcher opened this issue Mar 21, 2024 · 11 comments
Labels
enhancement New feature or request

Comments

@f-forcher
Copy link

Hi just a small bug, I think you meant to use strip_suffix for >, here?

https://github.com/ratatui-org/templates/blob/966cf2e2b5808de8c905eacd1f4209fe82f804fe/component/template/src/config.rs#L251-L252

@orhun
Copy link
Member

orhun commented Mar 21, 2024

Yup, do you mind sending a PR to fix it? 🐻

@f-forcher
Copy link
Author

Yes sure.

But to be honest, the main issue that made me read the parse_key_sequence fn is that as far as I understand it, it cannot parse a < or > key. In some layout like mine (italian) < is a main key in place of \, so it would be quite useful to be able to set it.

I just came up with this regex based alternative fn, what do you think? You can then do "<\\>>": "Quit" to escape any key. I can polish and test it then make a PR for this instead, maybe?

use regex::Regex;

pub fn parse_key_sequence(raw: &str) -> Result<Vec<KeyEvent>, String> {
  let key_event_re = Regex::new(r"<(?<keyevent>(?:[^<>\\]|\\.)*)>").unwrap();
  let no_backslash_re = Regex::new(r"\\(.)").unwrap();
  key_event_re.captures_iter(raw)
    .map(|c| {
      let (_, [key_event_string]) = c.extract();
      key_event_string
    })
    .map(|s| 
      {
        let no_backslash: &str = &*no_backslash_re.replace_all(s, "$1").into_owned();
        parse_key_event(no_backslash)
    })
    .collect()
}

@orhun
Copy link
Member

orhun commented Mar 22, 2024

I'm not sure if the code can be simplified a bit but improving this sounds good to me. Any thoughts @kdheepak @joshka ?

@f-forcher
Copy link
Author

f-forcher commented Mar 22, 2024

Yea no ofc need to be cleaned up a bit, no need for 2 regexes probably, it was just to show the idea more clearly.

Otoh I hope am not missing anything important on the technical details of keyboards and character classes, I'm not super knowledgeable about them.

@kdheepak
Copy link
Contributor

Nice find! This is what I get for not writing comprehensive tests :) I'm fine with a regex fix for this with more tests.

Also, certainly check out https://github.com/Canop/crokey/ for a more robust key parsing. I thought @joshka had something too but can't find it on his GitHub.

@joshka
Copy link
Member

joshka commented Mar 23, 2024

I'd suggest to add to this:

  1. Use verbose mode (ignore whitespace, allow newlines and end of line comments) to make the regex easier to read
  2. Compile the regex for perf
  3. Add tests and docs (this is complex and it's difficult to figure out the exact intent behind this and the expected edge cases)
  4. I think this could be better (simpler config, easier code) without the angle brackets. I.e. just use a comma to separate keys.
  5. Consider moving the regex up one level in the call stack and handle single key strokes as a single item sequence.

I thought @joshka had something too but can't find it on his GitHub.

You're talking about https://crates.io/crates/keybinding. It's still a placeholder. Also read ratatui/ratatui#627 for context around this.

@f-forcher
Copy link
Author

Is there any particular reason crokey isn't used for the parsing here?

@kdheepak
Copy link
Contributor

Just personal preference when I wrote the template and I pulled code from an existing project and refactored it to add it to the template. I think crokey would be the way to go.

@f-forcher
Copy link
Author

Alright, then no need to reinvent the wheel with regex, prob better to go that way directly.

I'm a bit busy right now, but I'll try to see how much work porting the settings to crokey would be.

@orhun orhun changed the title prefix/suffix Strip prefix/suffix Mar 27, 2024
@orhun
Copy link
Member

orhun commented May 18, 2024

Hey @f-forcher, kindly ping to check if you are still interested in going forward with this 🐻

@orhun orhun added the enhancement New feature or request label May 18, 2024
@f-forcher
Copy link
Author

Hi guys, sorry I didn't forgot, but I have been interviewing full time since then 😓 I had to put personal projects on the backlog a bit. Now I have a bit of a lull next week l, I'll try to see to it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants