Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Highlight history searches correctly #9066

Merged
merged 1 commit into from
Jul 13, 2022
Merged

Conversation

SeekingBlues
Copy link
Contributor

Description

Previously, the search text is used to find out which part of the updated command line should be highlighted during a history search. This approach will cause the incorrect part to be highlighted when the line contains multiple instances of the search text.

For example, if we execute:

echo hohoho

And type this in the command line:

echo ho

Then do a token search on ho, we get an unexpected highlight:

# Expected:
echo hohoho
     ^^
# Actual:
echo hohoho
  ^^

That is, the ho in echo is highlighted instead of ho that is the current token.

To address this, we have to find out exactly where to highlight, i.e. the offset of the current token in the command line (0 if not a token search) plus the offset of the search text in the match.

TODOs:

  • Changes to fish usage are reflected in user documentation/manpages.
  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.rst

src/reader.cpp Outdated
assert(match_index_ < matches_.size() && "Invalid match index");
return matches_.at(match_index_);
}

/// \return the string we are searching for.
const wcstring &search_string() const { return search_.original_term(); }

/// \return the range of the original search string in the new command line.
maybe_t<std::pair<size_t, size_t>> search_range_if_active() const {
Copy link
Member

Choose a reason for hiding this comment

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

Excellent fix and changes, thanks!

I just have one minor quibble: instead of std::pair we should use source_range_t.
(Else if we really wanted std::pair, we should include <utility> for include-what-you-use-correctness).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course. I have switched to source_range_t instead.

Previously, the search text is used to find out which part of the
updated command line should be highlighted during a history search. This
approach will cause the incorrect part to be highlighted when the line
contains multiple instances of the search text.

To address this, we have to find out exactly where to highlight, i.e.
the offset of the current token in the command line (0 if not a token
search) plus the offset of the search text in the match.
@krobelus krobelus added this to the fish 3.6.0 milestone Jul 13, 2022
@krobelus krobelus merged commit 173914a into fish-shell:master Jul 13, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants