feature: highlight patterns like vim's hlsearch

[midichef]

This is a draft of a feature to highlight substrings within a column, or in every column. It looks like this, highlighting \d+ in the SKU column, and highlighting cat everywhere.

I'm floating this to get feedback. By default, highlighting is used after every search with / and ? and g/ and g?. options.highlight can be toggled to turn highlighting off. highlight-sheet will do the same highlighting as g/ and highlight-col will do the same as /, while keeping the cursor in place.

In the screenshot you can see that partial offscreen matches are highlighted for strings that don't fully fit in the cell, like where Cat is shown as C…. (But partial offscreen data will not be highlighted for collections like tuples, lists, and dictionaries. That treatment of offscreen contents is the same as the current behavior for /.)

Remaining work

1. Should | set the highlight pattern like / does? I'm not sure.
2. When a column has a highlight pattern, the sheet pattern is not used for that column. In the last line, in the SKU column, you can see that the column \d+ pattern is lit, but the sheet pattern cat is not.
3. Undo of search-* does not undo highlighting. (undo of highlight-* works properly, but search-* is in nonLogged so it is more complicated)
4. Ellipses at the edge of a column are sometimes absent, when they are adjacent to an offscreen highlighted string.
5. Matches that are partially offscreen might have off-by-1 errors in highlighting for corner cases of full-width characters and very narrow columns.

[midichef]

@daviewales @frosencrantz You two have contributed to past discussions and work on visidata's colorizers. Like those, pattern highlighting is a visual cue. Perhaps you have some feedback?

[daviewales]

Is there a way to clear the highlighting? For example, vim-sensible maps this to Ctrl+L:
https://github.com/tpope/vim-sensible/blob/0ce2d843d6f588bb0c8c7eec6449171615dc56d9/plugin/sensible.vim#L58

[midichef]

There's no command/binding to clear it now. The only way to clear it is to set a new nonexistent pattern. Having one is a good suggestion. Ctrl+L is an interesting direction. Maybe zCtrl+L?

On thinking about it more, allowing multiple simultaneous highlight patterns (one for each column, plus one for the entire sheet) is too complicated. Instead there should only be one at a time. That's what users expect from vim and less.

[frosencrantz]

Thank you for doing this. I think this is great.

Having only one highlighting format at a time is reasonable. Having multiple different highlights can get too complicated. Would the highlighting be per column or per sheet depending on the last type of search performed? Or would it be useful for it to control that by option? At some point it might be interesting to have a way do some sort of syntax highlight like vim and other tools can do, but that is a separate item.

I think the special cases where the matches are at the edge of the cell or screen should eventually be fixed.

[daviewales]

I agree that it makes sense just to have one highlight type at a time. In Vim at least, the highlight is based on the most recent search. I'd likely expect similar in Visidata. For example, I know that when I jump to the next match, it will be the next highlighted item.

[midichef]

@frosencrantz I've pushed a change. The last search sets the highlight type. So / does per-column, g/ does throughout-sheet.

Ctrl+L now clears the highlighting as part of redraw. It's an intuitive binding, thank you for suggesting it @daviewales.

For | and \, selecting rows by regex, my feeling is they should not change highlights. They already give visual feedback by changing selections. And someone who does want highlighting, they can just follow | up with / Up Enter.

So the remaining issues now are just 3, 4, and 5 from my initial list: fixing undo for / and variants, and fixing display problems at edges of columns.

[saulpw]

This is a neat feature. My main concern is that it has to be a core feature. If it could be segmented into a self-contained file in features/ that would make it a lot more compelling. I don't see how that can be though, without adding more complicated hooks. So let's keep it simple and minimal and if people really like the feature we can take it.

[frosencrantz]

Hopefully there is a way to resolve the feedback for this useful feature. It would be difficult to make this a separate feature without making of the cliptext code more generic. If the request is to make it smaller, maybe for now it would be limited to either per column or per sheet? Perhaps full sheet highlighting is the slightly more useful feature?

[midichef]

@frosencrantz I'm glad you find this feature useful. It's good motivation for me to revisit this. The reason for delay here is that I ran into a troublesome edge case when handling strings that get automatically truncated by … to fit into cells. A proper solution would need broader changes to Visidata, but that's probably beyond the scope of this PR.

I'll push a set of commits here with the current state of what I've got, for more discussion. I'll squash the previous commits together with some new work into 1 commit, that contains just the basics of the highlight feature. Then I'll tack on a few more commits that trigger the troublesome edge case, for discussion on whether to keep them or exclude them.

[midichef]

Last week, I wrote here:

I ran into a troublesome edge case when handling strings that get automatically truncated by … to fit into cells.

But it's not a problem any more, I found a workaround. So I've force-pushed a new set of commits. It uses the previous set as a foundation, and fixed some bugs, and squashed it all into 1 commit.

Then I added several commits that add highlighting of patterns that don't fit in the cell. When the highlighted match is fully left or right of the visible cell, the ellipses on the left or right side of the cell are highlighted. When the match is partly visible and partly outside the cell, the visible partial match is highlighted, and so are the ellipses where the match continues offscreen. I left them as multiple small commits in case we want to rework details of them. In particular, 207255f feels a bit awkward and repetitive. Any ideas on a better solution?

[midichef]

Also, there is one known flaw with this PR right now: highlighting has problems in multiline mode. Matches that cross lines fail to be highlighted, though matches that fully fit within a line are fine. And when matches are fully offscreen to the right of the cell, the ellipses fail to be highlighted. This is because calc_height() creates display values where the right edge is already truncated by textwrap.wrap(), making the edge text unsearchable by _highlight() later.

Fixing these problems would require changes to calc_height(). That's more than I'm willing to implement. It may be that the impact of these flaws is acceptably small, because multiline display, when it is on, only affects the row that has the cursor. Which the user will be paying attention to when / or g/ takes the cursor there.

And multiline mode has had flaws of its own for a while. For example, multiline mode does not properly word-wrap full-width symbols. And multiline mode causes display oddities with edit-cell, because edit-cell only uses the top line of the buffer.

So I'm proposing we take this PR. I think the highlight feature is helpful enough that it's worth it.

[saulpw]

This is a lot of code! Can we move this functionality into a separate file like features/hlsearch.py, such that importing the file implements it, but everything works fine as it currently does without it? The other functions should be trivial to move (with the implementation being enabled by command replacements in that file), but I'm not sure how easy it would be for this code in draw(). I can help figure out a strategy if you want.

[midichef]

Sure, separating it should be possible. I should have made _highlight(chunks) take hp as a parameter, I just missed the dependence on hp. Once I do that, it can be extracted from drawRow().

How does features/ handle the default initialization of instance variables, like self.highlight_regex = None for TableSheet? Right now it's done in TableSheet.__init__() in sheets.py. Should that be moved to hlsearch.py too?

[saulpw]

This is the purpose of the TableSheet.init('highlight_regex', lambda: None, copy=False), so that these variables can be instantiated within the same .py that they are used (and won't exist if that file is removed).

The copy kwarg is whether the value should be copied if the sheet is copied (for instance, to make a subsheet). Up to you in this instance, if the search highlighting should persist when you use " to dup-selected.

[saulpw]

Do we really want to clear the highlights? My model for Ctrl+L is that it simply redraws the screen as it was (after e.g. extra-terminal corruption) and doesn't change anything.

[midichef]

I found it a convenient binding. But that behavior is different from vim, where Ctrl+L leaves the pattern visible. So I'm fine with taking it out.

Are there keys free to bind to highlight-clear? Without a good key, people are likely to just do / asdf to run another search for a nonexistent pattern. That's bad because it pollutes the search history.

[saulpw]

Hm, I see. The only key that feels possibly intuitive is Esc (though that implies a larger modality which is not the case here). Let me think about Ctrl+L having a side-effect like this.

[daviewales]

I suggested Ctrl+L, because that's the binding in the vim-sensible plugin to clear highlights:
https://github.com/tpope/vim-sensible/blob/master/plugin/sensible.vim#L55

It also appears to have been adopted by Neovim for the same purpose:
https://neovim.io/doc/user/various.html#CTRL-L

But you're right that in Vim, without plugins, it doesn't clear the search highlight:
https://vimhelp.org/various.txt.html#CTRL-L

[midichef]

Okay, I've made the requested changes.

What's the right way to make the standalone highlight_chunks() utility function visible for use by drawRow()? For now I've just made it part of Sheet using @Sheet.api, but it doesn't belong there.

There are still a couple of places where the highlight features are used outside of features/hlsearch.py: drawRow() uses sheet.options.highlight, and redraw uses highlight_clear(). I couldn't see a clean way to move them into hlsearch.py.

[midichef]

@saulpw Just a bump to let you know this PR is currently tagged waiting for contributor but it's actually ready for further review.

[saulpw]

What's the right way to make the standalone highlight_chunks() utility function visible for use by drawRow()? For now I've just made it part of Sheet using @Sheet.api, but it doesn't belong there.

This is fine for now. I use VisiData.api, Sheet.api, and Column.api as places to stick utility functions like this, depending on whether their primary context is a Sheet, a Column, or neither. As long as the function has a reasonably non-conflicting name and is relegated to a module that can be not imported without breaking things, then that's fine.

There are still a couple of places where the highlight features are used outside of features/hlsearch.py: drawRow() uses sheet.options.highlight, and redraw uses highlight_clear(). I couldn't see a clean way to move them into hlsearch.py.

Using sheet.options.highlight is okay for now, as it will only produce a warning if the option doesn't exist. (Make sure though that getting the default of "None" for an unknown option still results in valid behavior.). The best thing to do for highlight_clear() is to make it a more general semantic concept and provide a stub that can be legitimately called (even if it does nothing), and then gets added to in the external module (with @Sheet.after for instance).

[saulpw]

Just a couple of notes. Let's consider the option name, and make the highlight_clear() function more generic and give it a stub so that nothing breaks if hlsearch.py is no longer imported. Then I think we can merge!

[midichef]

Using sheet.options.highlight is okay for now, as it will only produce a warning if the option doesn't exist.

Reading the option that doesn't exist produces a ValueError (though setting such an option gives a warning):

  File "/home/midichef/.venv/lib/python3.12/site-packages/visidata/sheets.py", line 1034, in drawRow
    if self.options.highlight_search:
  File "/home/midichef/.venv/lib/python3.12/site-packages/visidata/settings.py", line 261, in __getattr__
    return self.__getitem__(optname)
  File "/home/midichef/.venv/lib/python3.12/site-packages/visidata/settings.py", line 270, in __getitem__
    raise ValueError('no option "%s"' % optname)
ValueError: no option "highlight_search"

How should I handle that? A try/except?

[saulpw]

How should I handle that? A try/except?

Hmm...it seems like it should give a warning rather than an exception. For now, move the option (default False) to sheets.py so it always exists, with a comment next to it indicating that the entire implementation is in features/hlsearch.py. I'll look into how best to deal with checking unknown options, and I can move it then.

[midichef]

Okay, that's a good temporary solution. Done!

[saulpw]

okay, I made a few small changes. We'll set the option to True by default for now, to give the feature visibility on develop. Depending on response we may turn it to False before the next release. Thanks again!