Highlight all search matches in document #5702

wes-adams · 2023-01-27T13:58:38Z

Altering document clears search_matches highlights

(PS: the extra whitespace being highlighted in the demo gif is a glitch in asciicinema capture)

Updated display of matching text to use ui.cursor.match. this is the result:

References:
#4798

the-mikedavis · 2023-01-27T14:58:26Z

I haven't looked at this yet but also see #4635 which I think will have some overlap implementation-wise

wes-adams · 2023-01-27T17:06:21Z

I haven't looked at this yet but also see #4635 which I think will have some overlap implementation-wise

Thanks. I'll definitely look into that work.
This PR is 100% not ready. I'm still finding my way in this project and Rust in general, so any feedback and/or reviews I can get will be greatly appreciated!

wes-adams · 2023-01-30T00:49:12Z

i think the only optimization left is to search only the visible text for matches versus the entire document
i've given this a little effort, but would welcome any feedback i can get

EDIT: the above is fixed, but a bug is introduced:
if scrolling through matches moves current view, the search range is not updated, so highlights remain in the same place, but are wrong

pascalkuthe · 2023-01-30T12:09:23Z

Just a couple comments:

The matches should not be stored on the editor, they should be stored on the document or (even better) on the view
Right now the matches stick around forever which causes a host of problems if the text is edited. To fix this you either need to update the matches on every edit or hide them as soon as the document is edited. Regex matches has quite a bit of overhead because we need to convert Rope -> String (see Regex Automata #185) so that would not be ideal. I also dislike how long the highlights stick around in nvim (even tough I like the basic feature of search highlights) so I think it's actually a great solution to remove the search matches on edits.
Updating the matches while scrolling is too slow. I think you could recompute the matches on idle timeout if the view position changed.
This potentially should be rebased on rework positioning/rendering and enable softwrap/virtual text #5420 too as the way the "visible area" is calculated changes there (the view offset changes), this PR should also not be merged before that because I would have to update this command as part of rework positioning/rendering and enable softwrap/virtual text #5420 then (which is already very large)

wes-adams · 2023-02-08T02:53:48Z

Just a couple comments:

The matches should not be stored on the editor, they should be stored on the document or (even better) on the view

Right now the matches stick around forever which causes a host of problems if the text is edited. To fix this you either need to update the matches on every edit or hide them as soon as the document is edited. Regex matches has quite a bit of overhead because we need to convert Rope -> String (see Regex Automata #185) so that would not be ideal. I also dislike how long the highlights stick around in nvim (even tough I like the basic feature of search highlights) so I think it's actually a great solution to remove the search matches on edits.

Updating the matches while scrolling is too slow. I think you could recompute the matches on idle timeout if the view position changed.

This potentially should be rebased on rework positioning/rendering and enable softwrap/virtual text #5420 too as the way the "visible area" is calculated changes there (the view offset changes), this PR should also not be merged before that because I would have to update this command as part of rework positioning/rendering and enable softwrap/virtual text #5420 then (which is already very large)

i have this PR in the following state:

search_matches are now stored in View
search_matches highlights are updated with each new key pressed (in search_impl)
- if n(or others) is used to traverse searches, search_matches are not updated
; can be used to clear highlights, but highlights will resume if n is used to continue match traversal
highlights do not go away or update when doc updates
- i tried to make this happen with Document::is_modified() but i had troubles
- if is_modified() == true and the document is never saved, the search_matches highlights would never be rendered
- i think i need help with this (either updating search_matches on edit, or clearing search_matches on edit)
(also, FWIW, i tried all sorts of solutions using idle timeout, but it seems to be dependent on key presses. so i had some decent solutions in place, but they required a keystroke or two before highlights would update, etc)

@the-mikedavis
@pascalkuthe

helix-view/src/view.rs

wes-adams · 2023-02-08T18:27:27Z

helix-view/src/view.rs

+impl fmt::Debug for SearchMatches {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.debug_struct("SearchMatches")
+            .field("matches", &self.matches)
+            .field("new", &self.new)
+            .field("old", &self.old)
+            .finish()
+    }
+}


probably not needed - can remove

wes-adams · 2023-02-08T18:28:08Z

helix-view/src/view.rs

+    pub fn has_changed(&self, regex: &Regex) -> bool {
+        regex.as_str() != self.old.as_str()
+    }


could not figure out a clean way to compare Regex, so this is what i came up with

wes-adams · 2023-02-08T18:28:45Z

helix-view/src/view.rs

+                new: Regex::new("").unwrap(),
+                old: Regex::new("").unwrap(),


i assume unwrap is safe to use in a case like this? but it is a big assumption...

helix-view/src/view.rs

wes-adams · 2023-02-08T18:32:20Z

helix-term/src/commands.rs

+    // but set search_matches.old to "" so that if 'n' is pressed
+    // to continue to traverse matches, highlights resume
+    view.search_matches.clear();
+    view.search_matches.old = Regex::new("").unwrap();


again... is unwrap safe to use here?

As it is initialize code with empty string. I don't think unwrap will panic here in runtime. It's ok imo.

helix-term/src/ui/editor.rs

pathwave · 2023-02-09T12:05:00Z

Is the color here the same as selections? Might need to difference between those.

A feature I would like is to highlight words that match the current primary selection, and that seems to overlap with this (since when you search the search term gets selected)

wes-adams · 2023-02-10T17:31:16Z

Is the color here the same as selections? Might need to difference between those.

A feature I would like is to highlight words that match the current primary selection, and that seems to overlap with this (since when you search the search term gets selected)

the search_matches highlight color is dictated by ui-selection:

    pub fn get_search_matches(
        &self,
        theme: &Theme,
    ) -> Option<Vec<(usize, std::ops::Range<usize>)>> {
        let selection_scope = theme
            .find_scope_index("ui.selection")
            .expect("could not find `ui.selection` scope in the theme!");
. . .

an easy update or change to have a different highlight for selection and search_matches, but there would have to be a candidate definition in the theme configs.

wes-adams · 2023-02-10T21:29:53Z

helix-view/src/view.rs

+    pub fn update_search_matches(&mut self, contents: &str, regex: &Regex) {
+        // this is expensive. only update search matches if search criteria has changed
+        if self.search_matches.has_changed(regex) {
+            self.search_matches.update(regex);
+            self.search_matches.clear();
+            for one_match in self.search_matches.new.find_iter(contents) {
+                self.search_matches
+                    .matches
+                    .push((one_match.start(), one_match.end()));
+            }
+        }
+    }


the first if checks if the Regex search criteria has changed. this prevents this block from executing if the user is traversing match results using n or similar. if the Regex search criteria has indeed changed, old is set equal to new, and existing matches are cleared. new matches are calculated via expensive Regex operation based on new search criteria

wes-adams · 2023-02-10T22:05:44Z

this one is ready for a closer look guys

i think the behavior is close to what @pascalkuthe outlined in the comment above

@the-mikedavis
@pascalkuthe

References: helix-editor#4798

wes-adams · 2023-07-04T00:43:26Z

would either of you please take a look?
i'd be very grateful for some feedback on this one.
@the-mikedavis @pascalkuthe

tmpm697 · 2023-08-29T19:07:13Z

What's status of this PR? this is really helpful to have all matched words highlighted.

lpchaim · 2023-10-15T16:04:32Z

I've been looking into adding a search index/total matches statusline as I was used to back in nvim (e.g., indicating you're on match 2 out of 5 with something like [2/5] on the bottom) and it does look like I should wait until this gets merged in the interest of not duplicating the work made here so that the context holds search match information and whatnot, so I'll be keeping an eye on this.

wes-adams · 2023-10-16T14:07:14Z

@the-mikedavis @archseer @pascalkuthe @sudormrfbin
sorry to blindly tag you guys again, but i'm still hoping to get some sort of review on this PR
would anyone kindly help me gain some traction on this?

pascalkuthe · 2023-10-16T18:14:27Z

my concern from #5702 (comment) still apply/are unadressed.

Running a regex over document contents synchronously on every keypress is unacceotable. Particularly of the rope -> string conversion is very non-ideal and should be done rarely (or rather not at all, the current implementation is a workaround). I am working on a streaming regex engine but that still takes some time and even with that solved (which would improve pref a lot) running a regex on every keypress is stijj not something I am ok with. Using the transactional word mapping in #6447 could allow you to update the ranges without actually running a regex and then you could run the actual regex with a 5s debounce or similar (with #8021). I don't think regularly covering to string would be ok tough so that has to wait until we have a streaming regex engine... Which will be a while.

You could keep all of this simpler by dismissing changes when transactions are applied just as I said in my first comment.
Store the highlights in a HashMap<ViewId, SerachMatches> and then clear the entire map in apply_impl. Then you just need some sort of view position changed hook (with #8021) to update the matches when scrolling (similar to inlay hints but not based on the idle timeout like the current implementation. I don't want to merge new stuff that uses the idle timeout).

wes-adams force-pushed the search-highlight branch from 9be5377 to e8cd334 Compare January 27, 2023 14:09

wes-adams marked this pull request as draft January 27, 2023 14:10

wes-adams force-pushed the search-highlight branch 2 times, most recently from ec8de2a to c4e8146 Compare January 27, 2023 14:29

wes-adams changed the title ~~Tmp - highlight working, but still needs work~~ Highlight all search matches in document Jan 27, 2023

the-mikedavis added A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer. labels Jan 27, 2023

wes-adams force-pushed the search-highlight branch from 34dc5d2 to 0cb2cf9 Compare January 30, 2023 00:39

wes-adams marked this pull request as ready for review January 30, 2023 00:43

pascalkuthe added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Jan 30, 2023

wes-adams force-pushed the search-highlight branch from 5acdb12 to 91d2a61 Compare January 31, 2023 19:24

wes-adams force-pushed the search-highlight branch from 91d2a61 to 9361993 Compare February 8, 2023 02:45

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 Feb 8, 2023

wes-adams force-pushed the search-highlight branch 4 times, most recently from c46d038 to 5eb6e2f Compare February 8, 2023 18:09

wes-adams commented Feb 8, 2023

View reviewed changes

helix-view/src/view.rs Outdated Show resolved Hide resolved

wes-adams commented Feb 8, 2023

View reviewed changes

helix-view/src/view.rs Outdated Show resolved Hide resolved

wes-adams commented Feb 8, 2023

View reviewed changes

helix-term/src/ui/editor.rs Outdated Show resolved Hide resolved

wes-adams force-pushed the search-highlight branch from 5eb6e2f to 4cac675 Compare February 10, 2023 21:28

wes-adams commented Feb 10, 2023

View reviewed changes

wes-adams force-pushed the search-highlight branch 3 times, most recently from 606e0c1 to 629acc9 Compare February 10, 2023 22:02

ubitux mentioned this pull request Feb 12, 2023

Make * also select the word under the cursor #1762

Open

wes-adams force-pushed the search-highlight branch from 629acc9 to 99e46d5 Compare February 14, 2023 18:17

wes-adams force-pushed the search-highlight branch from 99e46d5 to a3a435d Compare March 3, 2023 15:42

wes-adams force-pushed the search-highlight branch from 376efed to a3a435d Compare July 4, 2023 00:06

Highlight all search matches in document

b4e8ddd

References: helix-editor#4798

wes-adams force-pushed the search-highlight branch from a3a435d to b4e8ddd Compare July 4, 2023 00:12

the-mikedavis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Feb 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Highlight all search matches in document #5702

Highlight all search matches in document #5702

wes-adams commented Jan 27, 2023 •

edited

Loading

the-mikedavis commented Jan 27, 2023

wes-adams commented Jan 27, 2023

wes-adams commented Jan 30, 2023 •

edited

Loading

pascalkuthe commented Jan 30, 2023

wes-adams commented Feb 8, 2023 •

edited

Loading

wes-adams Feb 8, 2023

wes-adams Feb 8, 2023

wes-adams Feb 8, 2023

wes-adams Feb 8, 2023

Proful Feb 8, 2023

pathwave commented Feb 9, 2023

wes-adams commented Feb 10, 2023 •

edited

Loading

wes-adams Feb 10, 2023

wes-adams commented Feb 10, 2023

wes-adams commented Jul 4, 2023

tmpm697 commented Aug 29, 2023

lpchaim commented Oct 15, 2023 •

edited

Loading

wes-adams commented Oct 16, 2023

pascalkuthe commented Oct 16, 2023 •

edited

Loading

Highlight all search matches in document #5702

Are you sure you want to change the base?

Highlight all search matches in document #5702

Conversation

wes-adams commented Jan 27, 2023 • edited Loading

the-mikedavis commented Jan 27, 2023

wes-adams commented Jan 27, 2023

wes-adams commented Jan 30, 2023 • edited Loading

pascalkuthe commented Jan 30, 2023

wes-adams commented Feb 8, 2023 • edited Loading

wes-adams Feb 8, 2023

Choose a reason for hiding this comment

wes-adams Feb 8, 2023

Choose a reason for hiding this comment

wes-adams Feb 8, 2023

Choose a reason for hiding this comment

wes-adams Feb 8, 2023

Choose a reason for hiding this comment

Proful Feb 8, 2023

Choose a reason for hiding this comment

pathwave commented Feb 9, 2023

wes-adams commented Feb 10, 2023 • edited Loading

wes-adams Feb 10, 2023

Choose a reason for hiding this comment

wes-adams commented Feb 10, 2023

wes-adams commented Jul 4, 2023

tmpm697 commented Aug 29, 2023

lpchaim commented Oct 15, 2023 • edited Loading

wes-adams commented Oct 16, 2023

pascalkuthe commented Oct 16, 2023 • edited Loading

wes-adams commented Jan 27, 2023 •

edited

Loading

wes-adams commented Jan 30, 2023 •

edited

Loading

wes-adams commented Feb 8, 2023 •

edited

Loading

wes-adams commented Feb 10, 2023 •

edited

Loading

lpchaim commented Oct 15, 2023 •

edited

Loading

pascalkuthe commented Oct 16, 2023 •

edited

Loading