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

Add search widget #1054

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add search widget #1054

wants to merge 1 commit into from

Conversation

jphalip
Copy link
Contributor

@jphalip jphalip commented Dec 2, 2024

This widget is activated while doing a search (forward/backward, word search, etc.). It shows the current occurrence that you're on, as well as the total number of occurrences for the search terms. Works similarly to IntelliJ's built-in search tool.

This relies on search highlights being shown. But perhaps this is acceptable since the users who would care about this widget would also likely have highlights turned on?

If tests are expected for this, could you point me to some similar examples? I couldn't find tests, for example, for the macro widget.

2024-12-01_22-51-10

@AlexPl292
Copy link
Member

Related ticket: https://youtrack.jetbrains.com/issue/VIM-2544

@AlexPl292
Copy link
Member

Hi, thank you for your interest, this is a nice feature. I've added a related ticket.
I think this PR needs the following updates, but let's discuss if you have a different opinion.

  • Let's disable this widget if the shortmess option has a S flag. This will match the Vim behaviour: https://stackoverflow.com/a/60840571. However, this looks fine to have this widget enabled by default. This will also match the neovim default behavior.
  • Relying on highlighting is not okay. We should properly count the number of occurrences and show them in the widget, as the widget and highlighting are two orthogonal features of the search.
    • In the same way, removing the widget inside the clearSearchHighlight is incorrect.
  • Also, in Vim the widget is disabled on the new search (when / is pressed), or when the other mode is entered. This doesn't happen here.

@AlexPl292 AlexPl292 self-requested a review January 9, 2025 07:31
}

class VimSearchWidget : StatusBarWidget, VimStatusBarWidget {
var content: String = ""
Copy link
Member

Choose a reason for hiding this comment

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

Directly accessing the content here leaks incapsulation. Let's have special functions like setSearch(current, total) and clearSearch().
(function names are subject to change).

@@ -1340,13 +1340,15 @@ abstract class VimSearchGroupBase : VimSearchGroup {
} else if (lastPatternTrailing!![ppos + 1] == '?') {
Direction.BACKWARDS
} else {
injector.listenersNotifier.notifySearchUpdated(editor, res)
Copy link
Member

Choose a reason for hiding this comment

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

This is not a good place for running listeners. The finItOffset should be a clean function that returns the result of the search. If it's not clean, let's not make it worse at least.

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.

2 participants