Ctrl-T to use word under cursor or current selection

[mihailik]

Implements #29974

[msftclas]

@mihailik,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

[mihailik]

@bpasero Can you please comment?

Apart from general common sense UX, in practice this helps a lot navigating large (unfamiliar) codebases (such as VSCode).

I tried to fit it with the existing design (which you @bpasero worked on), with the flag autoPickWord passed in the same way as prefix to the constructor of QuickOpenAction base class. The job of working out the word and passing it along is put to that base class too.

Additionally, the auto-picked word is pre-selected in the search box. That way user starting typing will instantly override the auto-picked word.

Hope to get this in the VSCode, for our team's benefit -- and to help the community too.

[mihailik]

Also can you please suggest any examples for testing these classes? I'd love to follow the approach and spirit of the project in terms of testing and SDLC.

Many thanks!

[mihailik]

I've figured out the place to stick the tests -- added there.

[bpasero]

@mihailik I do not understand why this has to be put down into the quick open action in such a generic way, can you not just add it to the action that opens the symbols picker since it is only used there?

[mihailik]

@bpasero it's because this.prefix is private, and set once in the constructor.

In case of Ctrl-T it would change from one call to another.

But that's a good point -- let me refactor it a different way, so the complexity resides in the descendant. And I need to update the tests too.

[bpasero]

@mihailik I think for Ctrl+T it is fine to always use the selection as a seed for the input but I am not sure about Ctrl+Shift+O where this may not be expected because by default we show a list of all symbols of that file and someone might like that behaviour to quickly glance at the list of symbols of a file.

As for Ctrl+T: if you seed the prefix like that I think you should select the prefix up until the "#" portion such as typing overwrites whatever the seed is. You can look at ShowAllCommandsAction to see how this is done. I would actually suggest to do the ShowAllSymbolsAction in similar fashion and let it not extend QuickOpenAction.

[mihailik]

@bpasero done.

Removed all the customisation from QuickOpenAction, pushed it down to ShowAllCommandsAction.

And I kept the piece where I've extracted getSelectionSearchString from its former home in CommonFindController -- so the same logic for picking current word is shared between Ctrl-F and Ctrl-T.

[mihailik]

BTW there are 2 failing tests in one (out of two) instances of Travis - due to 10s timeout. NOT related to my changes, might be flaky tests?

https://travis-ci.org/Microsoft/vscode/jobs/252995386#L4241

1. colorization scss-test.scss:
   Error: timeout of 10000ms exceeded. Ensure the done() callback is being called in this test.

2. colorization typescript-test.ts:
   Error: timeout of 10000ms exceeded. Ensure the done() callback is being called in this test.

[bpasero]

@rebornix assigning you to review the change in editorCommon.ts.

[bpasero]

LGTM

[rebornix]

@mihailik I like your approaching of avoiding code duplication but I don't think getSelectionSearchString should go into editorCommon. As we already have getWordRangeAtPosition exposed, I would be good to keep getSelectionSearchString just in search part, especially when we have some similar code in https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/parts/search/browser/searchViewlet.ts#L830

cc @roblourens

[roblourens]

I think it would be good if editor find and search could share a helper function with that code, but I don't know where to put it, since they don't share any code yet.

[bpasero]

+1 for sharing this piece of code in more places if there are more cases

@rebornix can you make a suggestion where to move this shared code so that we can continue working on this PR? I am not sure @mihailik can make that decision easily.

[mihailik]

Yes please. I will do the work, just guide me high level :-)

BTW, being used from multiple places shall I write some tests too? Just give me some directions, please.

[bpasero]

Since I leave for vacation soon I am unassigning myself in favour of @roblourens and @rebornix (also within #29974). Change LGTM 👍

[rebornix]

@mihailik I would suggest creating a new file called find.ts in contrib/find/common and put those sharing code into it. You can start with just getSelectionSearchString. The file can be similar to contrib/format/common/format.ts.

Besides, you can add a corresponding test file named as find.test.ts in contrib/find/test/common (you can follow how we write test cases for find controller). It's okay that you leave the tests work to me though. Thanks again for your work on this.

[rebornix]

@mihailik LGTM, thanks for your help! Let me know when you think it's good to merge.

[mihailik]

How about that, with some unit tests too! :-D

[mihailik]

Actually wait, if no editor is open Ctrl-T now fails. Let me add some defensive checks.

[mihailik]

Fixed that, works without focused editor too now.

[rebornix]

@mihailik well done sir!