Skip to content

mark mode: begin selection at focused search result#19550

Merged
carlos-zamora merged 1 commit intomainfrom
dev/cazamor/mark-mode/init-from-search
Nov 18, 2025
Merged

mark mode: begin selection at focused search result#19550
carlos-zamora merged 1 commit intomainfrom
dev/cazamor/mark-mode/init-from-search

Conversation

@carlos-zamora
Copy link
Member

@carlos-zamora carlos-zamora commented Nov 11, 2025

Summary of the Pull Request

Searching in terminal highlights all search results. However, those results are considered separate from a selection. In the past, the highlighted result would be selected, resulting in it being the initial position for mark mode. Now that it's separate, mark mode doesn't start there.

To fix this, there's 2 changes here:

  1. When we exit the search, we now select the focused search result. This becomes the initial position for mark mode.
  2. When we're in the middle of a search and mark mode becomes enabled, the focused search result becomes the initial position for mark mode.

With this change, mark mode's initial position is determined in this order:

  1. the position of an active selection
  2. the position of the focused search result (if one is available)
  3. the top-left position of the viewport (if there is a scrollback) (see mark mode: begin selection at viewport when scrolled up #19549)
  4. the current cursor position

Validation Steps Performed

Entering mark mode in scenario X results in a starting position of Y:
✅ selected text during a search --> selected text

  • NOTE: this seems to only occur if you start a search, then manually click on the terminal to bring focus there, but keep the search results active

✅ performed a search and results are available -->focused search result
✅ performed a search and no results are available

  • scrolled up --> top-left of viewport
  • no scrollback --> cursor position

✅ performed a search, got results, then closed search --> focused search result

Closes #19358

Comment on lines +1791 to +1797
// GH #19358: select the focused search result before clearing search
if (const auto focusedSearchResult = _terminal->GetSearchHighlightFocused())
{
_terminal->SetSelectionAnchor(focusedSearchResult->start);
_terminal->SetSelectionEnd(focusedSearchResult->end);
_renderer->TriggerSelection();
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I think leaving behind selected text after performing a search feels a little weird, but I think it's necessary. This enables the user to begin a selection at the focused search result using only the keyboard. If we get rid of this, the user would need to manually click on the terminal or transfer focus over somehow.

@DHowett
Copy link
Member

DHowett commented Nov 12, 2025

Hmmmm. I don't love that we now have three search states: active result, inactive result, and selected. I'm not sure how I feel all-up

@carlos-zamora carlos-zamora merged commit 19a8501 into main Nov 18, 2025
19 checks passed
@DHowett DHowett deleted the dev/cazamor/mark-mode/init-from-search branch November 20, 2025 19:45
DHowett pushed a commit that referenced this pull request Dec 2, 2025
## Summary of the Pull Request
Fixes a bug where the dangling selection from a search would be applied
to the wrong position. Specifically, the issue is that
`SetSelectionAnchor()` and `SetSelectionEnd()` expect viewport positions
whereas the searcher outputs buffer positions.

This PR simply applies the scroll offset to the search result before
calling the functions.

In a separate iteration, I changed the functions to allow for
viewport-relative vs buffer-relative positions. However, that ended up
feeling a bit odd because this is the only scenario where the functions
were receiving buffer-relative positions. I chose this approach instead
because it's smaller/cleaner, even though we convert to
viewport-relative before the call just to change it to buffer-relative
in the function.

Bug introduced in #19550

## Validation Steps Performed
The correct region is selected in the following scenarios:
✅ no scrollback
✅ with scrollback, at bottom
✅ with scrollback, not at bottom (selection isn't scrolled to, but I
think that's ok. Can be fixed easily if requested)
✅ alt buffer
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.

Moving the Mark-Mode cursor using the find or marks capabilities

3 participants