Add search function within patch viewer - #2
Conversation
dd17c92 to
e3679b1
Compare
Allows search within patch files just like vim's search function (i.e. supports regex, same n/N controls, etc). Colors of the matches differ between dark and light mode. Signed-off-by: Rosemarie O'Riorden <rosemarie@redhat.com>
/<up_arrow>/<down_arrow> search history just like in vim's search. Does not persist across sessions. Signed-off-by: Rosemarie O'Riorden <rosemarie@redhat.com>
Before this commit, esc and q had the same function inside the patch viewer during a search, which is that they would both exit from the search, and then with a second press they'd exit from the patch viewer and go back to the list of patches. With this commit, esc exits from the patch viewer even mid-search, and q exits from the serach upon the first press, and exits from the patch viewer upon the second press. However, if nothing was found in a search, q will exit from the patch viewer immediately as if an active search was not ongoing. Signed-off-by: Rosemarie O'Riorden <rosemarie@redhat.com>
dceara
left a comment
There was a problem hiding this comment.
Thanks @roseoriorden for this feature, I tried it locally and it seems to work quite nicely!
Here's some comments on the actual code from my side. Please take them with a grain of salt as it's the first time I really look closely at the leadlight code so some of them might be completely off.
Thanks,
Dumitru
| viewportLoading bool | ||
| viewportOffset int | ||
| viewExpanded bool | ||
| searching bool |
There was a problem hiding this comment.
Nit: Should all this search related state be part of a separate group of fields, lower. I mean, it applies to the compare mode too and that has its own separate section of fields.
| func (m *Model) handleSearchInputMode(msg tea.KeyMsg, updateMatches func()) (tea.Model, tea.Cmd) { | ||
| key := msg.String() | ||
| switch key { | ||
| case "enter": |
There was a problem hiding this comment.
I'm not sure I like the mixing of search logic with key handling. keys.go seems like the appropriate place to handle all input keys for all modes. Would it make sense to keep it like that?
| viewportLoading bool | ||
| viewportOffset int | ||
| viewExpanded bool | ||
| searching bool |
There was a problem hiding this comment.
searching is a bit misleading IMO. It actually is used to reflect the "the user is typing the search query" state. Only once the search is "committed" (enter is hit) we actually are in searching state.
| right = comparePadLine(right, rightWidth, | ||
| compareDiffKind(m.compare[1].kinds, idx)) | ||
|
|
||
| left = m.highlightLineIfMatched(left, idx) |
There was a problem hiding this comment.
Here, if the match is only on one side (left or right) but not on the other we'll try to highlight on both sides and fail for the side that doesn't have the match.
But does that mean that we'd also potentially scroll incorrectly? Shouldn't the searchMatches be part of the compareSide struct instead?
Which means, shouldn't we have a let's call it SearchMatcher struct, that stores all matches. And we could potentially use it in three places:
- for the viewportLines (in patch view mode)
- for the left lines (in compare view mode)
- for the right lines (in compare view mode)
Maybe that will also remove the need for the slightly unnatural if m.viewMode == viewCompare { checks in search.go, what do you think?
|
Some LLM review comments: Commit 1: 9314b07 — "tui: search: Add vim-like expression search /<expr>." (3 issues, medium)
Commit 2: 0f38713 — "tui: search: Add search history." (2 issues, low)
Commit 3: 4af6be2 — "tui: search: Make esc exit patch, q clear search then exit." (2 issues, low)
I didn't look too deep into the code myself, but one of the big things I tried to avoid in different places in |
Add a vim-like expression search /
Supports regex like vim and has the same n/N controls
Assisted-by: Claude Sonnet 4.5, Claude Code