Search: support multiple matches#532
Conversation
Pull Request Test Coverage Report for Build 21161689652Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
There was a problem hiding this comment.
Thank you very much for your contribution!
- Please make sure the PR checks pass, it seems like there a formatting issue (
cargo +nightly fmtshould fix it) - Could you please add a test for the new feature at the bottom of
editor.rs? Current coverage for kibi is unfortunately not comprehensive, but for new features having tests would be great
| let current = last_match.unwrap_or_else(|| num_rows.saturating_sub(1)); | ||
| // 清除所有行的 match_segments | ||
| for row in &mut self.rows { | ||
| row.match_segments.clear(); |
There was a problem hiding this comment.
Could this be moved to the for loop below, to save 2 lines of code?
| row.match_segment = Some(rx..rx + query.len()); | ||
| return Some(current); | ||
| let current = last_match.unwrap_or_else(|| num_rows.saturating_sub(1)); | ||
| // 清除所有行的 match_segments |
There was a problem hiding this comment.
Please keep all the comments in English, thanks :)
(and similarly below)
| } | ||
| let mut found_row = None; | ||
| for idx in 0..num_rows { | ||
| let i = (current + if forward { 1 } else { num_rows - 1 } + idx) % num_rows; |
There was a problem hiding this comment.
I tried running the code, searching backwards (pressing up arrow key while the search prompt is open) seems to be broken now
| /// If not `None`, the range that is currently matched during a FIND | ||
| /// operation. | ||
| pub match_segment: Option<std::ops::Range<usize>>, | ||
| pub match_segments: Vec<std::ops::Range<usize>>, |
There was a problem hiding this comment.
I think I would rather keep the current behavior of only highlighting one match at a time, otherwise we can't tell visually which position we matched (where the cursor will be after we press enter)
This way you can also return early in the loop in editor, without having to search through the entire file at once
There was a problem hiding this comment.
If "keep the current behavior of only highlighting one match at a time", then this PR can be turned off. Hahaha
If you want to support multi-highlight display, I can modify the above code according to your suggestions.
There was a problem hiding this comment.
For #487 the main concern is being able to have multiple matches per line, which is not currently supported, but your PR solves.
So we could have something like:
1 │𝐇𝐞𝐥lo Hello World
~ │
Search (Use ESC/Arrows/Enter): Hel
and when pressing the right arrow, the match would become
1 │Hello 𝐇𝐞𝐥lo World
~ │
Search (Use ESC/Arrows/Enter): Hel
I think highlighting every match would be valuable, but we need to be able to differentiate the current match (where the cursor will be after pressing Enter) versus all the other matches. I think the current match could keep the current styling; maybe the other ones could be underlined instead (ANSI code for underline: \x1b[4m). What do you think?
There was a problem hiding this comment.
Your idea is great. I understand what you mean. I'll try to implement it during this period of time. I might not have time these two days.
|
@all-contributors Please add @rtczza for code. |
|
I've put up a pull request to add @rtczza! 🎉 |
3 participants
fix: #487