Skip to content
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

Disable snippets in extension search (when not in suggest dropdown) #55281

Conversation

JacksonKearl
Copy link
Contributor

@JacksonKearl JacksonKearl commented Jul 27, 2018

This closes #55278, but a full solution is contingent on #55280.

This closes #55280 with 5a32484

@@ -342,7 +343,7 @@ export class ExtensionsViewlet extends ViewContainerViewlet implements IExtensio
this.monacoStyleContainer = append(header, $('.monaco-container'));
this.searchBox = this.instantiationService.createInstance(CodeEditorWidget, this.monacoStyleContainer,
mixinHTMLInputStyleOptions(getSimpleEditorOptions(), localize('searchExtensions', "Search Extensions in Marketplace")),
getSimpleCodeEditorWidgetOptions());
{ isSimpleWidget: true, contributions: [SuggestController] });
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree we don't need SnippetController2 and TabCompletionController, but have you gone through what the other contributions (MenuPreventer, SelectionClipboard, ContextMenuController) of the simple editor have to offer to ensure we are not missing out on anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • MenuPreventer stops Alt+click from toggling the menu bar visibility. We should have that.
  • SelectionClipboard is a Linux specific thing that I can't seem to get to trigger on Windows. From what I can tell, it automatically adds whatever you select to your clipboard and pastes it on middle click. Not sure if this is something we want, given we select the search text for the user occasionally and this might overwrite their clipboard
  • ContextMenuController allows you to right click in the search box. We should have that.

@JacksonKearl
Copy link
Contributor Author

Looks like removing SnippetController2 is breaking the ability to insert text. Error shows up: Cannot read property "insert" of null. investigating.

@JacksonKearl
Copy link
Contributor Author

JacksonKearl commented Jul 30, 2018

Looks like the SuggestController delegates the actual insertion of text to the SnippetController2. So removing SnippetController2 breaks the SuggestContoller. Not sure what the best way to proceed from here is...

  • we can change the suggest controller to not depend on a SnippetController2 existing. This makes sense, but its a bit big of a change to make in the endgame.
  • we can add back the SnippetController2. This isn't a great solution, because it means global snippets will show up on type.
  • we can add back the SnippetController2, and also get "editor.snippetSuggestions": "none" to have an effect in monaco simple widgets. This makes sense as well, and is something we wanted to do anyways, at Internal monaco editors do not respect editor.snippetSuggestions = 'none' #55280.

@JacksonKearl JacksonKearl force-pushed the disable-snippits-in-extensions-search branch from 8494f50 to b0af12c Compare July 30, 2018 17:13
@JacksonKearl
Copy link
Contributor Author

Needed for #55386.

@ramya-rao-a ramya-rao-a merged commit 28bfd5b into microsoft:master Jul 30, 2018
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants