Add updateTextFromFindWidgetOrSelection method to SearchView

[turara]

This PR resolves #78733.

I have added updateTextFromWidgetOrSelection method to SearchView to use search words from find widget when focused instead of using selection.

[roblourens]

Looks good, but could we share the block of code that does the update? We can have two methods like getTextFrom* and one place that does the update.

[turara]

@roblourens Thank you for your review!

I have extracted updateText method to share the block of code that does the update.

And added code that updates regular expression option when updating text from find widget.

---

About having getTextFrom* methods, I think to make getTextFromFindWidget method like

private getTextFromFindWidget(): string | null {
	const activeEditor = this.editorService.activeTextEditorControl;
	if (!isCodeEditor(activeEditor)) {
		return null;
	}

	const controller = CommonFindController.get(activeEditor as ICodeEditor);
	const searchString = controller.getState().searchString;
	return searchString !== '' && controller.isFindInputFocused() ? searchString : null;
}

is a bit confusing because this method checks whether the find widget is focused or not although getText~ does not imply any conditions of the find widget.

As to updateTextFromSelection, it also seems a bit difficult to extract a simple getText~ method because this method has the following conditional check:

if (this.searchWidget.searchInput.getRegex()) {
	selectedText = strings.escapeRegExpCharacters(selectedText);
}

What do you think?

Thanks :)

[roblourens]

This is fine, but I realized that the inconsistency bugs a little me that the search.seedWithNearestWord setting is not respected when getting text from the find widget. Can you figure out whether it's possible to check whether text in the find widget is selected? cc @rebornix @JacksonKearl

[turara]

@roblourens Thank you for your review. I took a look at that and I couldn't find a way to get selection information in the find widget.

However I wonder whether users want to use a very part of text in the find widget or not when they want to seed words from the find widget for the search view. I think it makes more sense to always use the whole text in the find widget.

[turara]

I've added the following lines to updateTextFromFindWidget method for the consistency with the regexp option.

this.searchWidget.searchInput.setCaseSensitive(controller.getState().matchCase);
this.searchWidget.searchInput.setWholeWords(controller.getState().wholeWord);

[JacksonKearl]

I agree that if we're in seeding from find widget case we can ignore the nearest word setting and just go with the full find input state.

[roblourens]

I mean, the reason that setting exists is for cases when people just want to move focus to the search view without changing its text, and this would break that, right? I guess we can try it and see how it goes?

[JacksonKearl]

search.seedWithNearestWord is so you can search for the work your cursor is on without needing to select it first, right? I don't know why that would be affected by this. IMO once focus is not in an editor all bets are off wrt search.seedWithNearestWord.

[roblourens]

I don't think a user is going to draw a strong distinction between "the editor" and the find widget. Either way, it's going to do something in the search view that they didn't expect.

[turara]

I mean, the reason that setting exists is for cases when people just want to move focus to the search view without changing its text, and this would break that, right?

I don't think this is right. When no words are selected in the editor, current behaviors are as follows:

1. With search.seedWithNearestWord off, the text in the search view will not change.
2. With search.seedWithNearestWord on, the nearest word to the cursor in the editor will replace the text in the search view.

When some words are selected in the editor, they will replace the search view text regardless of the setting.

[roblourens]

If you are describing what happens today (without your change) then that's right, and matches what I said (they leave the setting disabled if they want to not change the search view unless they select text)

If you are describing what happens in the find widget with your PR, then that's not what I see, I see it always update the search view when that setting is disabled and focus is in the find widget, regardless of editor selection.

[turara]

Now I see what you concern about. (I described what happens today. )

IMO I don't concern about that because this PR changes the behavior only when the find widget is focused. I think people want to use the entire word in the find widget for the search view when they focus on the find widget.

[roblourens]

I think people want to use the entire word in the find widget for the search view when they focus on the find widget.

I don't see any reason that people want different behavior when focus is in the find widget. But, there is a solution.

We can know when the find input is focused (you already check this) and we know that it is a native text input. So we can check window.getSelection https://developer.mozilla.org/en-US/docs/Web/API/Window/getSelection to check whether any text is selected. If you can check this for whether to use the text, depending on the search.seedWithNearestWord, then I will happily merge this.

To make it simple, we can say, if there is any selection in the find widget, take the full value. If there is no selection, don't take the value (when the setting is disabled)

[turara]

Thank you for the suggestion! I'll take a look.

[turara]

@roblourens I've updated updateTextFromFindWidgetOrSelection and updateTextFromFindWidget to check selection in the find widget and search.seedWithNearestWord value.

[roblourens]

Looks good, but we are stabilizing for release this week so I will merge it next week.

[gmathis]

Thanks for your work on this!
It's been a pet peeve of mine in a UI that is otherwise excellent in most regards.

Looks like I'm a bit too late to sway the final result, but as the filer of the bug, I thought it might be worth chiming in with my 2c.

---

TL;DR: To illustrate where the modified behavior here fails my use case:

• User enters a string of multiple words in the find widget that doesn't match in the current file
• By definition, the failed search will have been manually entered, since it is not present on the page, so it will not be selected
• User presses [Ctrl]-[Shift]-[F] to search across files

Expected:

• The full search term is escalated to the search view

Actual:

• search.seedWithNearestWord === true: Only the last word in the string is escalated to the search view
• search.seedWithNearestWord === false: Nothing is escalated to the search view

---

The behavior that best addresses the use case I presented in the bug is to always use the full search terms in the find widget when "escalating" a failed local search to the search view.
For another data point, I just checked VSPro and this is how it behaves.

I think the key point is that the user has already specified search terms once they choose to escalate the search.
This differs from the case where the user opens the find widget or search view from an editor with no selection, where search.seedWithNearestWord is a handy shortcut to pre-populate search terms if they have not already been selected.
The use cases where the user wants to both escalate the search and change / abandon the search terms seem less common than the case where the user wants to perform exactly the same search, but across the workspace instead of the current file.

As far as using the selection in the find widget in the determination of what to escalate to the search view, my gut reaction is that consistently escalating the full terms would be the least confusing behavior, though I could see a case for using a selection, if present. It's easier to pare down the terms after escalating than to retype missing portions if the unexpected thing happens for the user's use case.

[roblourens]

search.seedWithNearestWord === true: Only the last word in the string is escalated to the search view

I don't see this, I see the whole string go in.

I see your point but I'm just really hesitant to put stuff in the search view and trigger a new search, it could be really annoying if that's not what the user wanted.

[gmathis]

search.seedWithNearestWord === true: Only the last word in the string is escalated to the search view

I don't see this, I see the whole string go in.

If that's the case, then I'm all set.

It seemed like the proposal was to treat text in the find widget like editor text w.r.t. the search.seedWithNearestWord setting when determining what to pre-populate the search view with.
But, now that I'm actually reading through the code, I see that it actually appears to use the setting as a proxy for whether to populate the search view at all (i.e. all or nothing). Probably should have double-checked the code before writing a dissertation.

I think this should work great for my use case and you can ignore the above.
Thanks, and apologies for the confusion!

Looking forward to trying it out in the new version.

[turara]

@gmathis

Thanks for your work on this!
It's been a pet peeve of mine in a UI that is otherwise excellent in most regards.

Thank you for your comment. I'm glad to hear that! 😄

[turara]

@roblourens
Is there anything I'm supposed to do about https://github.com/microsoft/TypeScript/issues/41484?

[roblourens]

@gmathis That was a fair assumption. I read that setting with the emphasis on "seed", and the contents of the find widget are just the relevant chunk of text, even if it's not a "word" and doesn't work exactly the same as in the editor. And your point about the whole scenario of upgrading from an editor find to a search is still good and I will continue thinking about it.

Nope, that's a language server issue, it doesn't have anything to do with your PR in particular 😁