ratatui / templates

Templates for bootstrapping a Rust TUI application with Ratatui
https://ratatui.rs/templates
MIT License
255 stars 36 forks source link

Strip prefix/suffix #52

Open f-forcher opened 8 months ago

f-forcher commented 8 months ago

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 commented 8 months ago

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

f-forcher commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 https://github.com/ratatui-org/ratatui/discussions/627 for context around this.

f-forcher commented 8 months ago

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

kdheepak commented 8 months ago

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 commented 8 months ago

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 commented 6 months ago

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

f-forcher commented 6 months ago

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