Teach Find command to search selected text

[Don-Vito]

PR Checklist

• Closes Selected text should populate search box #8307
• CLA signed.
• Tests added/passed
• Documentation updated - not yet
• Schema updated
• I've discussed this with core contributors already.

Detailed Description of the Pull Request / Additional comments

Add selectionMode field to the find command.

• If no mode is specified (backward-compatibility) or none is provided - behave as before.
• If selectionMode is set to singleLine and a single line of text is selected,
  use it to populate the search box.
• If selectionMode is set to multiLine concatenate all lines with line-breaks omitted,
  and use the resulting string to populate the search box.

The population logic will be applied whenever the search box control gets loaded /
reset by the consequent find command.

If the population logic should be applied while the search box is already populated,
an old value will be overridden.

Validation Steps Performed

• Manual tests

[skyline75489]

Do we need to add this to settings? Some people may not like the idea of arbitrarily populating the selected text.

[Don-Vito]

Do we need to add this to settings? Some people may not like the idea of arbitrarily populating the selected text.

@skyline75489 - this is a good question 😊
Probably, I could add it as a parameter to the Find operation.
Something like

{ "command" : { "action": "find", "populateFromSelectionMode": "None"}}

Where populateFromSelectionMode is one of:

• None - do not populate
• SingleLineSelected - populate when single line selected
• MultilineSelectedUseFirstLine - use the first line if more than one line selected
• MultilineSelectedUseAll - use the entire text for search

And the default by now will be None?
WDYT?

[skyline75489]

Honestly I'm good as long as the default behaviour remains the same. Remember the MRU tab switching thing? That just backfired in like no time. So considering this is also a breaking change, we should learned from the tab switching lesson.

[carlos-zamora]

Something like

{ "command" : { "action": "find", "populateFromSelectionMode": "None"}}

Where populateFromSelectionMode is one of:

• None - do not populate
• SingleLineSelected - populate when single line selected
• MultilineSelectedUseFirstLine - use the first line if more than one line selected
• MultilineSelectedUseAll - use the entire text for search

Great work! I like the design for this setting with just a few nits:

• don't forget to make the enums camel case
• Does any text application do multilineSelectedUseFirstLine? I feel like that's an odd case, but if someone else uses it, go for it.
• I wonder if we could make the names smaller? Maybe fillSearchBox, singleLine, multiLine (if we remove the other case)? Summoning @cinnamon-msft for some thoughts on this.

[Don-Vito]

@carlos-zamora - I added selectionMode to the find command with values none (default), singleLine, multiLine. Please review.

[carlos-zamora]

Meh. These are just nits at this point. So excited for this to go in! Plan on using it all the time!!

[carlos-zamora]

Suggested change

// - caseSensitive: boolean that represents if the current search is case sensitive

I don't see a caseSensitive param here haha

[Don-Vito]

My bad. Will fix it (in any case need to work on docs)

[carlos-zamora]

Oh yeah, I completely forgot to ask you to make a PR on the docs site haha.

[cinnamon-msft]

This is something I would actually consider making default. VS Code does this by default and we tend to follow in their footsteps for a lot of our features. I'm curious how the team feels about this.

For the setting, I'm wondering if this should just be a boolean instead: to either populate with a selection or not.

[carlos-zamora]

This is something I would actually consider making default. VS Code does this by default and we tend to follow in their footsteps for a lot of our features. I'm curious how the team feels about this.

For the setting, I'm wondering if this should just be a boolean instead: to either populate with a selection or not.

I'm down for just making it a bool (true --> multiLine, false --> none). Also down to make true the default because chromium edge and VSCode have it enabled by default.

[DHowett]

What if we don’t make it a setting at all? Let the users complain if they hate it

[DHowett]

Do it by default and only. Users can delete the search needle if they don’t want it

[Don-Vito]

@DHowett - in general I like your approach: let's put it in the preview and see if anyone complains.

The question is wether it usually works? I am not experienced enough, my only input is that it didn't work for MRU. But probably then it was an issue with the timing. I am not sure if this one goes to 1.5 or 1.6, but probably we should put it optional in 1.5 and default in 1.6 preview? Won't it allow us to get as much inputs as possible? Then we can cleanup the code.

I guess the main cost for this approach is translating the resources 😄

So probably, let's use a single useSelection flag, set to false in 1.5 and to true in 1.6 preview?

Unrelated to above, should we allow only single line selection (aka VS Code) or allow multiple lines by concating them (aka notepad++)?

[Don-Vito]

This is something I would actually consider making default. VS Code does this by default and we tend to follow in their footsteps for a lot of our features. I'm curious how the team feels about this.
For the setting, I'm wondering if this should just be a boolean instead: to either populate with a selection or not.

I'm down for just making it a bool (true --> multiLine, false --> none). Also down to make true the default because chromium edge and VSCode have it enabled by default.

@carlos-zamora - hmm from what I played right now:

• Edge Chromium, VS, FoxitReader - does not use selection.
• Firefox, Chrome, Word - do multi-line but cuts at some stage
• VSCode works only with single line.
• Notepad++ works with multiline
• Outlook - no one knows how to search there

[Don-Vito]

Created an alternative PR here: #8521.
Simply rollbacked the configuration commit.

[zadjii-msft]

I'm obviously biased towards whatever Sublime does, and they will pre-populate the search box with the selection, so I think that'd be cool 😎

@skyline75489 I definitely appreciate you thinking about this changing a default behavior. We were burned super hard about this, and we've had no shortage of discussion of "DONT CHANGE THE DEFAULTS" in the past month. I guess I can imagine a user wanting this to be disabled. That being said, it's a lot less breaking than in-order/mru tab switching IMO. For tab switching, there was no good remedy without changing the settings. You couldn't get an in-order-like behavior in MRU without changing the settings. For this one, you can still get the old behavior by pressing Bkspc.

I think IF someone comes along saying "but muh cheese moved", then we have a plan for how we would make this configurable.

(We do have a team sync planned for later today, so we'll be discussing this one)

---

On another topic, we've maybe revealed a bit of a shortcoming with the Terminal's search functionality here. We don't support multi-line searching at all. We probably should! But I don't think we should really start worrying about this in this PR. Let's just make the behavior here the same as "select some text, copy it, open the find box, paste the text". We can loop back around on multiline in the future.

[DHowett]

Mike's rationale matches mine for making it non-configurable. This doesn't need to be a knob that users can turn; I expect that if we did make it a knob we would only ever see users use two things: "turn it off" and "do the freakin' sane thing" (which is "it acts just like copy/paste of the selection would").

We are possibly the most configurable terminal that exists (I kid, but maybe?). That doesn't mean that we need to make literally everything an option.

Down that road lies madness. Down that road lies "I want the letter A to render differently on Tuesdays; perhaps you should have a tuesdayAAppearance config field?". Down that road lies "we have a search box, but users want to reclaim the 16 pixels taken up by the Case Sensitive button so why not let them hide it?" and the 60,000 individual UI feature toggles that come out of that. It's mad. Mad!

[Don-Vito]

Mike's rationale matches mine for making it non-configurable. This doesn't need to be a knob that users can turn; I expect that if we did make it a knob we would only ever see users use two things: "turn it off" and "do the freakin' sane thing" (which is "it acts just like copy/paste of the selection would").

We are possibly the most configurable terminal that exists (I kid, but maybe?). That doesn't mean that we need to make literally everything an option.

Down that road lies madness. Down that road lies "I want the letter A to render differently on Tuesdays; perhaps you should have a tuesdayAAppearance config field?". Down that road lies "we have a search box, but users want to reclaim the 16 pixels taken up by the Case Sensitive button so why not let them hide it?" and the 60,000 individual UI feature toggles that come out of that. It's mad. Mad!

@DHowett - what's wrong with madness? Sanity is overrated after all 😄
I see your point, and cannot argue (I worked on either drivers or cloud backend, so hardly know how it works with humans).
Then please review: #8521

[DHowett]

From our team meeting today: Team fully agrees that this should not be configurable. Redirect all to #8510 😄

[DHowett]

Erm, #8521