Skip to content

Conversation

@nikita-r
Copy link
Contributor

The name of the command already has "Selection" in it, so it does not make sense to respect editor.find.seedSearchStringFromSelection==never... Intuitively, this command should
behave similarly to the Ctrl+D default.

…*` ignore

the `editor.find.seedSearchStringFromSelection` setting
@rebornix rebornix modified the milestones: November 2022, On Deck Nov 23, 2022
if (editor.getOption(EditorOption.find).seedSearchStringFromSelection !== 'never') {
selectionSearchString = getSelectionSearchString(editor, 'single', seedSearchStringFromNonEmptySelection);
}
const selectionSearchString = getSelectionSearchString(editor, 'single', false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

seeding the search from cursor position alone should still be possible if seedSearchStringFromSelection is set to always. Hard coding the last parameter to false will break that scenario

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have you tried it, or just going off the variable name? IIRC the last parameter has somewhat tricky name and means the exact opposite of what you mean here, unless I mistake your meaning... setting it to false hard-codes it to seed either from selection or cursor position alone — both cases are respected... I'll do some testing though once I get to my home machine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

note that I meant the new code to behave exactly as the old code with seedSearchStringFromSelection==='always'; now it passes 'none' instead of 'single' at line 749, but that was effectively a no-op as far as I can tell

Copy link
Collaborator

Choose a reason for hiding this comment

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

from looking at getSelectionSearchString, this change will require that text is selected for it to be seeded as the search string, whereas currently the word at the cursor location will be used as the seed if nothing is selected

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, so I compiled this branch and here is a screencast: Ctrl+F3 definitely picks up the word at cursor without requiring a selection... Note that this behaviour is different from the current version mainline, where "inline" will be searched instead of "never" upon Ctrl+F3 with these settings.
seed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is a separate issue that I see though, where the Find Widget is not populated — but the search is still seeded! Just to make sure to not mix it up with my issue...
It happens (1) only on the very first invocation after startup (2) if the find widget is closed, and (3) the same happens on the mainline version with the default settings — so should be out of scope for this issue.
seed-atFirst

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok, you were right, I still had it backwards. This will always use the current word as the seed when nothing is selected, regardless of the settings. Seems ok since the regular find command is configurable to only seed when something is selected.

@amunger amunger requested a review from rebornix December 16, 2022 18:36
@rebornix
Copy link
Member

Thank you both for the discussions and contribution.

@rebornix rebornix merged commit 43a3d0a into microsoft:main Dec 16, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Jan 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants