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

Fix: [Fuzzy-Select] Special chars cause panic #183

Closed

Conversation

deg0nz
Copy link
Contributor

@deg0nz deg0nz commented Mar 7, 2022

This fixes #181

@deg0nz
Copy link
Contributor Author

deg0nz commented May 10, 2022

I just realised, that this does not include the changes regarding fuzzy-select highlighting (#179).

I suggest, we wait until a fix for #195 is merged and then adjust this PR here accordingly before merging.

@Araxeus
Copy link

Araxeus commented May 13, 2022

Now that we can search using special characters, I encountered a bug where searching for a left to right char would break the order of the string, after some debugging it seems that chained ANSI escaped chars do not preserve their text order

I've opened an issue in the rust repo, maybe it will be fixed someday
rust-lang/rust#97020

EDIT: apparently it's an issue with the terminal implementation

EDIT2: I've fixed this locally in my fork using a regex that tests whether the char is Hebrew/Arabic before highlighting it
https://github.com/Araxeus/dialoguer/pull/4/files

EDIT3: regex dependency is way too big and not actually needed, here's a much simpler way:

        //check if char is right to left aligned (arabic/hebrew unicode range)
        fn is_rtl(c: char) -> bool {
            ('\u{0591}'..='\u{07FF}').contains(&c) // c >= '\u{0591}' && c <= '\u{07FF}'
        }

then just check if its rtl before highlight indices

if indices.contains(&idx) && !is_rtl(c) {...

@Araxeus Araxeus mentioned this pull request May 13, 2022
@deg0nz deg0nz changed the title Fix: Special chars cause panic in fuzzy select Fix: ¶¢Special chars cause panic in fuzzy select May 15, 2022
@deg0nz deg0nz changed the title Fix: ¶¢Special chars cause panic in fuzzy select Fix: [Fuzzy-Select] Special chars cause panic May 15, 2022
@pksunkara
Copy link
Collaborator

Ideally, this should be handled by console. But console isn't maintained regularly and I don't have time to work on it even though I do have access to it. For that reason, I plan to refactor this crate to use crossterm.

@deg0nz
Copy link
Contributor Author

deg0nz commented May 29, 2022

For that reason, I plan to refactor this crate to use crossterm.

Sounds good. My summer is pretty packed already, so I don't have much time for coding in my free time. But if this transition is still open in fall/winter, I'm happy to contribute to this.

@pksunkara
Copy link
Collaborator

I am aiming for summer but realistically I probably can't get to it before fall.

@Araxeus
Copy link

Araxeus commented May 29, 2022

I'm using crossterm for key selection in my fork https://github.com/Araxeus/dialoguer
(select and fuzzy-select only)

since it allows me to check for key modifiers, which I personally needed

@pksunkara
Copy link
Collaborator

Nice. Does anyone know how to write tests for crossterm? If we have proper tests for dialoguer, it would make my life so much easier.

@Araxeus
Copy link

Araxeus commented May 29, 2022

The crossterm source code has a lot of internal tests you can take inspiration from

@grunweg grunweg mentioned this pull request Aug 18, 2022
5 tasks
@grunweg
Copy link
Contributor

grunweg commented Aug 18, 2022

I am aiming for summer but realistically I probably can't get to it before fall.

I had some leftover time and started moving to crossterm. Find my partial attempt here in case it's helpful.

@grunweg
Copy link
Contributor

grunweg commented Aug 18, 2022

The crossterm source code has a lot of internal tests you can take inspiration from

Both to fix some bugs I found and/or for helping with the rewrite, I'd like to add tests verifying the actual output on the terminal: "run this commands on the terminal/spin up this dialogue, then capture the current output", then repeat with more code. I would guess this already exists, but I can't seem to find it.
crossterm's tests are different; I don't see any of this kind. @Araxeus Do you know if such a thing exists?

@pksunkara
Copy link
Collaborator

Yeah, crossterm doesn't really have good TUI based tests.

What I have been looking for recently is a good library to write TUI end-to-end tests. I did a bit of research and found that some python tools use pyte(https://pyte.readthedocs.io/en/latest/)

Trying to find something similar in Rust ecosystem pointed me to the following:

For us to have to good testing, we need to either modify term-transcript or write a new one on our own based on https://github.com/alacritty/vte.

@epage I would appreciate your thoughts on this since this is related to rust-cli ecosystem as a whole.

@epage
Copy link

epage commented Aug 18, 2022

Historically, TUIs have been out of scope of rust-cli (there has been enough to do for normal CLIs) but I expect there is a lot we can improve about the TUI landscape. atm I don't have a TUI use case to help me dig in and see what the experience is like and speak on it, so any thoughts I have will be theoretical

@grunweg
Copy link
Contributor

grunweg commented Aug 28, 2022

@psunkara I see; that makes a lot of sense. That's out of my depth. (I'd be happy to help adding tests once there is a testing framework.)

Searching for TUI on crates.io yields the tui-rs and cursive crates (which are used for building TUI interfaces). I don't know if they allow writing TUI tests; just dropping this in case it's helpful.

@Gordon01
Copy link
Contributor

This can be closed as fixed in #245

@pksunkara pksunkara closed this Sep 12, 2023
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.

Fuzzy Select: Special characters lead to panic
6 participants