Add support for macOS globalFindClipboard #11233 #28896 #35956

melvin0008 · 2017-10-10T05:51:08Z

Fixes #11233 #28896 and #3431 by adding support for macOS globalFindClipboard

Summary of Changes:
I added API to IClipboardService.
Used dependency injection to inject the service in CommonFindController and wrote helper functions to use the apis. Should this be on the Controller or the editor object?
Then I used these helper functions in StartFindAction and NextMatchFindAction

msftclas · 2017-10-10T05:51:16Z

All CLA requirements met.

melvin0008 · 2017-10-11T05:31:27Z

I would like to know whether dependency injection into CommonFindController is the right thing to do.

melvin0008 · 2017-10-19T05:34:19Z

@alexandrudima I would love some help with this.

melvin0008 · 2017-10-21T19:24:12Z

@bpasero I would love to finish this PR and submit the PR :) In issue 11233 in reply to one of your comments I had a question.

I wanted to know what you meant by "the adoption should take place in vs/editor.." and "you need to provide a shim for this method (or no-op implementation)." in your previous comment
Is it to help editors to implement the same underlying interface?

Let me know if I am missing anything.
I added API to IClipboardService.
Used dependency injection to inject the service in CommonFindController and wrote helper functions to use the apis. Should this be on the Controller or the editor object?
Then I used these helper functions in StartFindAction and NextMatchFindAction.

Let me know if I'm correct in my approach. Thank You

alexdima

make the IClipboardService optional
write more often to the global find clipboard. I'm not sure if that's expected on OSX, but I'd write to it, everytime the user changes the find string, not just when starting to find.
I'm not sure when it is the best time to read from the global find clipboard? The NextMatchFindAction seems as a random place to read from it.
write some tests to express what the desired behaviour is.

alexdima · 2017-10-24T16:54:41Z

src/vs/editor/common/commonCodeEditor.ts

@@ -119,7 +120,8 @@ export abstract class CommonCodeEditor extends Disposable implements editorCommo
 		domElement: IContextKeyServiceTarget,
 		options: editorOptions.IEditorOptions,
 		instantiationService: IInstantiationService,
-		contextKeyService: IContextKeyService
+		contextKeyService: IContextKeyService,
+		clipboardService: IClipboardService


There's no need for the IClipboardService in this class, so it should be removed from here.

alexdima · 2017-10-24T16:57:23Z

src/vs/editor/contrib/find/browser/find.ts

@@ -27,9 +28,10 @@ export class FindController extends CommonFindController implements IFindControl
 		@IContextKeyService contextKeyService: IContextKeyService,
 		@IKeybindingService keybindingService: IKeybindingService,
 		@IThemeService themeService: IThemeService,
-		@IStorageService storageService: IStorageService
+		@IStorageService storageService: IStorageService,
+		@IClipboardService clipBoardService: IClipboardService


I recommend using @optional(IClipboardService) clipBoardService: IClipboardService
like it is used here. This will help with the standalone editor (the monaco editor).

optional is defined in vs/platform/instantiation/common/instantiation

Then, the code should assume that this service could be undefined/null

nitpick: perhaps name it clipboardService instead of IClipboardService to keep in spirit with the service name.

alexdima · 2017-10-24T16:58:21Z

src/vs/editor/contrib/find/common/findController.ts

@@ -308,6 +311,14 @@ export class CommonFindController extends Disposable implements editorCommon.IEd
 		}
 		return true;
 	}
+
+	public showGlobalBufferTerm(): string {
+		return this._clipboardService.readFindText();


Once optional is adopted, some null/undefined checks should be performed here

I'm also not sure why this method is called show..., it looks like a getter/reader

alexdima · 2017-10-24T16:58:26Z

src/vs/editor/contrib/find/common/findController.ts

+	}
+
+	public setGlobalBufferTerm(text: string) {
+		this._clipboardService.writeFindText(text);


Once optional is adopted, some null/undefined checks should be performed here

alexdima · 2017-10-24T17:01:10Z

src/vs/editor/contrib/find/common/findController.ts

@@ -378,7 +391,21 @@ export class NextMatchFindAction extends MatchFindAction {
 	}

 	protected _run(controller: CommonFindController): boolean {


Here we are always using the search string found in the global search clipboard, but we write to the global search clipboard only in the StartSearchAction. Perhaps the controller should write to the global search clipboard on each and every findState change ? i.e. anytime the user types in the find input ?

Everytime time the user types in the find input, I am writing to the global buffer . Also, everytime the user changes the global buffer using some other app and returns back to vscode, if the find box is open the find search string changes based on the global buffer

TODO: Add test and restrict it to OSX

melvin0008 · 2017-10-26T06:56:47Z

Thanks for the review. I will look into it and make the required changes. Could you let me know which version of node and npm is preferred.

alexdima · 2017-10-26T07:34:27Z

Here are the tooling requirements: https://github.com/Microsoft/vscode/wiki/How-to-Contribute#installing-prerequisites

liyanage · 2017-10-26T19:32:01Z

Out of curiosity I built and ran this branch but it didn't seem to interact with the find clipboard yet when I tried it. Is that supposed to work already or is more work needed?

…atform checks

melvin0008 · 2017-10-28T00:29:03Z

@liyanage It should be working now. Previously, I had rebased my branch which included few tests to pass before building an executable and probably thats why the feature didn't work. Give the latest one a shot and let me know.

melvin0008 · 2017-10-30T04:15:43Z

@alexandrudima

Resolved review comments.
Added tests to check whether it reads and writes to the global buffer correctly.
Made a change in the find model to pick changes from the global buffer if someone switches between vscode and some other application.

Let me know your thoughts on the same

liyanage · 2017-10-30T05:00:52Z

I pulled the latest and tried again, and it works great!

One small detail that is unexpected and that I would change is that hitting Cmd-E in VSC with a text selection does not just populate the find clipboard, it also enters find mode and brings up the small find overlay in the upper right corner. That is not how Cmd-E usually works, in all other Mac apps it just silently populates the find clipboard but does not perform any other action or visibly change the UI.

Thanks for your work on this, can't wait to see it in a build!

melvin0008 · 2017-10-30T07:35:35Z

@liyanage Thanks a lot for trying it out. In VScode if you select something and press Cmd F it opens the find overlay in upper right corner with the selected text in the find box. Since, Cmd E is a just an extension of Cmd F it behaves similar to Cmd F

melvin0008 · 2017-11-07T02:12:12Z

@alexandrudima Let me know if more changes need to be done. Thanks

…ind_11233

alexdima · 2017-11-14T14:11:08Z

Giving to @rebornix as he is back. I've resolved the conflicts my refactorings have caused.

@rebornix

Forwarding the PR to @rebornix

rebornix · 2017-11-22T21:24:59Z

@melvin0008 the code looks good to me, thanks for your contribution! Before merging this PR, there are just two little things to do

Resolve conflicts.
Have a simple summary of what this feature does (I'll put it into our release note). Even though I use macOS for a while, I didn't have any clue with the global search buffer (or maybe I never noticed it).

Speaking of the second one, we may need to think about how often this feature surprises users who are not familiar with this mac feature, and then decide whether we need a setting from the very beginning.

melvin0008 · 2017-11-22T23:27:25Z

@rebornix Thanks for the feedback. I feel it shouldn't surprise Mac users since this behavior is supported in most of the tools. Chrome, Sublime Text, Safari, Notes, Firefox are few tools I know which support this feature.

melvin0008 · 2017-11-23T07:03:33Z

Where should I add the summary?

rebornix · 2017-11-27T16:14:25Z

@melvin0008 you can send PR against vscode-docs vnext branch and add this to Notable Changes, write it like #pr-number: short-description, similar to others https://github.com/Microsoft/vscode-docs/blob/vnext/release-notes/v1_19.md#notable-changes . Thanks for your help!

Add support for macOS's NSFindPboard, which is a global shared find buffer. This allows one to put text into the find buffer from other native apps like Chrome, TextEdit, Safari, Sublime, etc. and then switch to other Cocoa apps and use cmd-g to find next matching text. Pull request for this change is microsoft/vscode#35956

melvin0008 · 2017-11-28T03:11:07Z

@rebornix Added the PR against vscode-docs vnext branch like suggested. There was a difference in the format though, it was #issue-number :short-description.

Thanks let me know if there is anything else to be done

rebornix · 2017-11-30T22:53:12Z

@melvin0008 it looks great! Thanks for your contribution ;)

melvin0008 · 2017-11-30T23:35:18Z

:) My pleasure. Always fun contributing to the products I use extensively.

Thanks for vscode :)

Add support for macOS's NSFindPboard, which is a global shared find buffer. This allows one to put text into the find buffer from other native apps like Chrome, TextEdit, Safari, Sublime, etc. and then switch to other Cocoa apps and use cmd-g to find next matching text. Pull request for this change is microsoft/vscode#35956

wprater · 2017-12-03T00:43:35Z

so happy for this feature!! found a couple of implementation bugs. want me to open new issues @melvin0008 @rebornix ?

melvin0008 · 2017-12-04T06:18:51Z

Sure.

bradkemper · 2017-12-20T17:07:31Z

I'm also very happy for this feature. Other Mac apps that support it also include BBEdit, TextEdit, Mail, and Preview.

JasonHise · 2018-01-24T01:11:32Z

I use vscode on windows, and have been a bit frustrated that when I have editors open in multiple panes, all of the panes have their own find state - so if I start searching for something, and want to continue my search in the next pane, I have to copy the search string, switch to the other pane, start a new search and then paste the string. By any chance would it be possible to implement a 'global find clipboard' for the windows version not to share the find string across the whole OS, but just to make it possible for the different panes to share a common find state?

IanBellomy · 2019-07-11T13:45:19Z

It's real nice that a version of this is in there, but the behavior is still inconsistent with other programs. In short: shifting the focus to the find menu is atypical. On this end at least, the behavior causes problems with the following pattern:

command+e to copy selection to find-clipboard.
edit selection in place in code, e.g. mySymbolName -> myNewSymbolName
shift+option+left to select new text. [or similar]
command+c to copy new text.
command+g to jump to next instance of old text (mySymbol).
command+v to paste the updated text.
[repeat 5 and 6 through document]

Steps 3 and 4 break when the focus shifts to the find pallet. I get tripped up on this regularly.

I'm not sure how common the above pattern is among other users, but I use it a frequently, especially when bashing out prototypes—mostly in cases where VSCode does not recognize all instances of a symbol, or if I'm not certain it'll find them all and would like to quickly step through each replacement to make sure, or the selected text occurs in parts of other related symbol names and I'd also like to update those, e.g. my_var / my_var_previous, or when the symbol name occurs across multiple languages in a project or document (e.g. php/html/css/js...)

liyanage · 2019-07-11T14:22:22Z

I agree, the find clipboard behavior is still not comparable to a native app. As described by @IanBellomy it's because of the find UI appearing when it should not. I should be able to Hit Cmd-E in VC or another app, switch to VC and hit Cmd-G and the selection should move to the next hit. At no point in this interaction should the find UI ever appear.

IanBellomy · 2019-07-11T19:26:06Z

Another way to put it is this: selecting text then pressing command+e is a way to specify a search token, and so is editing some text in a find-input. Jumping to a find input to specify a search token after specifying the search token may be redundant in many situations.

macrael · 2019-08-16T21:47:00Z

Is this supposed to work? I'm using cmd-e in various apps and nothing seems to affect VSCode. Do I have to enable a setting?

macrael · 2019-09-18T00:12:59Z

Update: you have to turn on Editor > Find: Global Find Clipboard and Search: Global Find Clipboard to get this feature to work.

It's not perfect, but it works with my muscle memory at least.

The only bug I'd file is that if you cmd-E in one editor pane, the search string in another editor pane is not immediately updated, even though if you cmd-G in that other editor pane it does immediately use the correct string (not the displayed one)

egamma assigned bpasero Oct 10, 2017

bpasero assigned alexdima and unassigned bpasero Oct 11, 2017

alexdima previously requested changes Oct 24, 2017

View reviewed changes

melvin0008 force-pushed the melvin0008/global_find_11233 branch from 7f58d1c to 2f267ee Compare October 25, 2017 03:38

Add support for macOS globalFindClipboard microsoft#11233

41a422c

TODO: Add test and restrict it to OSX

melvin0008 force-pushed the melvin0008/global_find_11233 branch from 2f267ee to 41a422c Compare October 25, 2017 04:53

Add feature for findWidgets to pick up from the global buffer. Add pl…

3c29acd

…atform checks

Fix tests by adding null check

e24e13f

melvin0008 changed the title ~~Add support for macOS globalFindClipboard #11233~~ Add support for macOS globalFindClipboard #11233 #28896 Oct 29, 2017

melvin0008 added 2 commits October 29, 2017 20:32

Add tests to test globalFindBuffer read and write

9a7aaa7

Add platform check to tests that check global clipboard

9c79ccd

Merge remote-tracking branch 'origin/master' into melvin0008/global_f…

45b0d50

…ind_11233

alexdima assigned rebornix and unassigned alexdima Nov 14, 2017

alexdima added the editor-find Editor find operations label Nov 14, 2017

melvin0008 added 2 commits November 14, 2017 22:49

Merge branch 'master' into melvin0008/global_find_11233

72d0593

Merge branch 'master' into melvin0008/global_find_11233

e3d8224

rebornix added the feature-request Request for new features or functionality label Nov 22, 2017

Merge branch 'master' into melvin0008/global_find_11233

20b5c70

melvin0008 mentioned this pull request Nov 28, 2017

Add support for macOS NSFindPboard as an action microsoft/vscode-docs#1278

Merged

rebornix merged commit dec4686 into microsoft:master Nov 30, 2017

melvin0008 deleted the melvin0008/global_find_11233 branch November 30, 2017 23:37

alexdima mentioned this pull request Dec 4, 2017

Find widget never empty #39422

Closed

rebornix mentioned this pull request Dec 4, 2017

Test macOS Global Find Clipboard #39585

Closed

2 tasks

wprater mentioned this pull request Dec 5, 2017

Find All input should be populated with Global Search Buffer #39609

Closed

alexdima added this to the November 2017 milestone Dec 8, 2017

github-actions bot locked and limited conversation to collaborators Mar 30, 2020

		@@ -378,7 +391,21 @@ export class NextMatchFindAction extends MatchFindAction {
		}

		protected _run(controller: CommonFindController): boolean {

Add support for macOS globalFindClipboard #11233 #28896 #35956

Add support for macOS globalFindClipboard #11233 #28896 #35956

Conversation

melvin0008 commented Oct 10, 2017 • edited Loading

msftclas commented Oct 10, 2017 • edited Loading

melvin0008 commented Oct 11, 2017

melvin0008 commented Oct 19, 2017

melvin0008 commented Oct 21, 2017

alexdima left a comment

Choose a reason for hiding this comment

alexdima Oct 24, 2017

Choose a reason for hiding this comment

alexdima Oct 24, 2017

Choose a reason for hiding this comment

alexdima Oct 24, 2017

Choose a reason for hiding this comment

alexdima Oct 24, 2017

Choose a reason for hiding this comment

alexdima Oct 24, 2017

Choose a reason for hiding this comment

alexdima Oct 24, 2017

Choose a reason for hiding this comment

melvin0008 Nov 1, 2017

Choose a reason for hiding this comment

melvin0008 commented Oct 26, 2017 • edited Loading

alexdima commented Oct 26, 2017

liyanage commented Oct 26, 2017

melvin0008 commented Oct 28, 2017

melvin0008 commented Oct 30, 2017

liyanage commented Oct 30, 2017 • edited Loading

melvin0008 commented Oct 30, 2017

melvin0008 commented Nov 7, 2017

alexdima commented Nov 14, 2017

rebornix commented Nov 22, 2017

melvin0008 commented Nov 22, 2017 • edited Loading

melvin0008 commented Nov 23, 2017

rebornix commented Nov 27, 2017

melvin0008 commented Nov 28, 2017

rebornix commented Nov 30, 2017

melvin0008 commented Nov 30, 2017 • edited Loading

wprater commented Dec 3, 2017

melvin0008 commented Dec 4, 2017

bradkemper commented Dec 20, 2017

JasonHise commented Jan 24, 2018

IanBellomy commented Jul 11, 2019 • edited Loading

liyanage commented Jul 11, 2019

IanBellomy commented Jul 11, 2019

macrael commented Aug 16, 2019

macrael commented Sep 18, 2019

melvin0008 commented Oct 10, 2017 •

edited

Loading

msftclas commented Oct 10, 2017 •

edited

Loading

melvin0008 commented Oct 26, 2017 •

edited

Loading

liyanage commented Oct 30, 2017 •

edited

Loading

melvin0008 commented Nov 22, 2017 •

edited

Loading

melvin0008 commented Nov 30, 2017 •

edited

Loading

IanBellomy commented Jul 11, 2019 •

edited

Loading