New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for macOS globalFindClipboard #11233 #28896 #35956

Merged
merged 9 commits into from Nov 30, 2017

Conversation

Projects
None yet
9 participants
@melvin0008
Contributor

melvin0008 commented Oct 10, 2017

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

This comment has been minimized.

Show comment
Hide comment
@msftclas

msftclas Oct 10, 2017

CLA assistant check
All CLA requirements met.

msftclas commented Oct 10, 2017

CLA assistant check
All CLA requirements met.

@melvin0008

This comment has been minimized.

Show comment
Hide comment
@melvin0008

melvin0008 Oct 11, 2017

Contributor

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

Contributor

melvin0008 commented Oct 11, 2017

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

@melvin0008

This comment has been minimized.

Show comment
Hide comment
@melvin0008

melvin0008 Oct 19, 2017

Contributor

@alexandrudima I would love some help with this.

Contributor

melvin0008 commented Oct 19, 2017

@alexandrudima I would love some help with this.

@melvin0008

This comment has been minimized.

Show comment
Hide comment
@melvin0008

melvin0008 Oct 21, 2017

Contributor

@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

Contributor

melvin0008 commented Oct 21, 2017

@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

@alexandrudima
  • 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.
Show outdated Hide outdated src/vs/editor/common/commonCodeEditor.ts Outdated
Show outdated Hide outdated src/vs/editor/contrib/find/browser/find.ts Outdated
Show outdated Hide outdated src/vs/editor/contrib/find/common/findController.ts Outdated
Show outdated Hide outdated src/vs/editor/contrib/find/common/findController.ts Outdated
Show outdated Hide outdated src/vs/editor/contrib/find/common/findController.ts Outdated
Add support for macOS globalFindClipboard #11233
TODO: Add test and restrict it to OSX
@melvin0008

This comment has been minimized.

Show comment
Hide comment
@melvin0008

melvin0008 Oct 26, 2017

Contributor

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.

Contributor

melvin0008 commented Oct 26, 2017

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.

@alexandrudima

This comment has been minimized.

Show comment
Hide comment
Member

alexandrudima commented Oct 26, 2017

@liyanage

This comment has been minimized.

Show comment
Hide comment
@liyanage

liyanage Oct 26, 2017

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?

liyanage commented Oct 26, 2017

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?

@melvin0008

This comment has been minimized.

Show comment
Hide comment
@melvin0008

melvin0008 Oct 28, 2017

Contributor

@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.

Contributor

melvin0008 commented Oct 28, 2017

@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 melvin0008 changed the title from Add support for macOS globalFindClipboard #11233 to Add support for macOS globalFindClipboard #11233 #28896 Oct 29, 2017

@melvin0008

This comment has been minimized.

Show comment
Hide comment
@melvin0008

melvin0008 Oct 30, 2017

Contributor

@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

Contributor

melvin0008 commented Oct 30, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@liyanage

liyanage Oct 30, 2017

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!

liyanage commented Oct 30, 2017

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

This comment has been minimized.

Show comment
Hide comment
@melvin0008

melvin0008 Oct 30, 2017

Contributor

@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

Contributor

melvin0008 commented Oct 30, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@melvin0008

melvin0008 Nov 7, 2017

Contributor

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

Contributor

melvin0008 commented Nov 7, 2017

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

@alexandrudima

This comment has been minimized.

Show comment
Hide comment
@alexandrudima

alexandrudima Nov 14, 2017

Member

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

Member

alexandrudima commented Nov 14, 2017

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

Forwarding the PR to @rebornix

@rebornix

This comment has been minimized.

Show comment
Hide comment
@rebornix

rebornix Nov 22, 2017

Member

@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.

Member

rebornix commented Nov 22, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@melvin0008

melvin0008 Nov 22, 2017

Contributor

@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.

Contributor

melvin0008 commented Nov 22, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@melvin0008

melvin0008 Nov 23, 2017

Contributor

Where should I add the summary?

Contributor

melvin0008 commented Nov 23, 2017

Where should I add the summary?

@rebornix

This comment has been minimized.

Show comment
Hide comment
@rebornix

rebornix Nov 27, 2017

Member

@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!

Member

rebornix commented Nov 27, 2017

@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!

melvin0008 added a commit to melvin0008/vscode-docs that referenced this pull request Nov 28, 2017

Add support for macOS NSFindPboard as an action
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

This comment has been minimized.

Show comment
Hide comment
@melvin0008

melvin0008 Nov 28, 2017

Contributor

@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

Contributor

melvin0008 commented Nov 28, 2017

@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 rebornix merged commit dec4686 into Microsoft:master Nov 30, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details
@rebornix

This comment has been minimized.

Show comment
Hide comment
@rebornix

rebornix Nov 30, 2017

Member

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

Member

rebornix commented Nov 30, 2017

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

@melvin0008

This comment has been minimized.

Show comment
Hide comment
@melvin0008

melvin0008 Nov 30, 2017

Contributor

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

Thanks for vscode :)

Contributor

melvin0008 commented Nov 30, 2017

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

Thanks for vscode :)

@melvin0008 melvin0008 deleted the melvin0008:melvin0008/global_find_11233 branch Nov 30, 2017

octref added a commit to Microsoft/vscode-docs that referenced this pull request Dec 2, 2017

Add support for macOS NSFindPboard as an action
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

This comment has been minimized.

Show comment
Hide comment
@wprater

wprater Dec 3, 2017

Contributor

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

Contributor

wprater commented Dec 3, 2017

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

@melvin0008

This comment has been minimized.

Show comment
Hide comment
@melvin0008

melvin0008 Dec 4, 2017

Contributor

Sure.

Contributor

melvin0008 commented Dec 4, 2017

Sure.

@bradkemper

This comment has been minimized.

Show comment
Hide comment
@bradkemper

bradkemper Dec 20, 2017

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

bradkemper commented Dec 20, 2017

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

@JasonHise

This comment has been minimized.

Show comment
Hide comment
@JasonHise

JasonHise Jan 24, 2018

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?

JasonHise commented Jan 24, 2018

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment