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

String DocumentFilter should only match scheme 'file' and 'untitled' #21886

Closed
dbaeumer opened this issue Mar 3, 2017 · 19 comments
Closed

String DocumentFilter should only match scheme 'file' and 'untitled' #21886

dbaeumer opened this issue Mar 3, 2017 · 19 comments
Assignees
Labels
api feature-request Request for new features or functionality on-testplan under-discussion Issue is under discussion for relevance, priority, approach
Milestone

Comments

@dbaeumer
Copy link
Member

dbaeumer commented Mar 3, 2017

  • VSCode Version: 1.10.x

Document filters that are only string based should only match scheme file and untitled. So if an extension registers a feature provider for 'bat' for example the provider should only be invoked for 'file' and 'untitled' scheme and not for example for 'git' scheme.

@dbaeumer dbaeumer added this to the March 2017 milestone Mar 3, 2017
@dbaeumer
Copy link
Member Author

dbaeumer commented Mar 3, 2017

@egamma FYI.

@jrieken jrieken added api feature-request Request for new features or functionality labels Mar 3, 2017
jrieken added a commit that referenced this issue Mar 7, 2017
@jrieken
Copy link
Member

jrieken commented Mar 7, 2017

@dbaeumer Pushed this to master, please verify with the language servers.

@aeschli You might need to adjust some selectors for html, json, etc that work off the virtual state

@jrieken jrieken closed this as completed Mar 7, 2017
@dbaeumer
Copy link
Member Author

dbaeumer commented Mar 7, 2017

@jrieken will do. Thanks!

@jrieken jrieken added the verification-needed Verification of issue is requested label Mar 27, 2017
@chrmarti
Copy link
Contributor

@jrieken This breaks the interactive playground. Adding walkThroughSnippet to the _defaultSchemes in languageSelector.ts appears to fix it. Please advise.

@chrmarti chrmarti reopened this Mar 27, 2017
@jrieken
Copy link
Member

jrieken commented Mar 28, 2017

Please share more details about your problem? Are you registering your document as walkThroughSnippet and what extension do you expect to work with that? The consequence of our change is that those extensions must not opt in and adjust their language selector

@jrieken jrieken closed this as completed Mar 28, 2017
@chrmarti
Copy link
Contributor

The embedded editors use models with walkThroughSnippet as their URL scheme. These URLs no longer match the registered languages, they did before. As a consequence editorModelConext.ts deactivates all contexts and actions like Format and Rename are no longer available in the embedded editors, that's a regression.

@chrmarti chrmarti reopened this Mar 28, 2017
@jrieken
Copy link
Member

jrieken commented Mar 28, 2017

Yes, that is the new design. It's not enough for the languages to just register with 'langId' anymore but they must spell out all schemes they should support (file, untitled, walkThroughSnippet ).

@jrieken
Copy link
Member

jrieken commented Mar 28, 2017

fyi @dbaeumer

@dbaeumer
Copy link
Member Author

The consequences of the old design was that the walkThroughSnippets were honored by a lot of extensions although they can't actually do something meaningful with them. With the new design extension by default participate in file and untitled (what most extensions thought happens anyways) and you must explicitly opt in into more schemes. This is breaking by design.

@chrmarti
Copy link
Contributor

Chatted with @jrieken, he suggests to add scheme: * to the languages participating in the playground, effectively reversing the effect of this change. It would be easier and avoid matching all schemes to handle walkThroughSnippet like untitled. The original change meant to address issues with git and similar, not walkThroughSnippet, if I understand correctly.

@jrieken
Copy link
Member

jrieken commented Mar 29, 2017

We will revert the changes for March to give us more time for this

@jrieken jrieken removed the verification-needed Verification of issue is requested label Mar 29, 2017
@jrieken jrieken modified the milestones: April 2017, March 2017 Mar 29, 2017
jrieken added a commit that referenced this issue Mar 29, 2017
@jrieken
Copy link
Member

jrieken commented Mar 29, 2017

Moving to April for further discussion

@chrmarti
Copy link
Contributor

Another way to look at it is that the Playground document is at odds with the layering, because it comes with languages that are not in the core, but in extensions. Longer term we could move the document out of the core, while keeping the Playground URI scheme and editor as language-agnostic support for playgrounds in the core. That would allow for special-casing the Playground scheme in the core similarly to untitled. /cc @kieferrm

@dbaeumer
Copy link
Member Author

dbaeumer commented Apr 3, 2017

IMO we should not have any playground specific URI knowledge in the core. A Playground should be contributable by an extension without it being necessary to modify the core.

I know that due to limitation we need to, but we should push hard to improve our system to not require thus.

@jrieken
Copy link
Member

jrieken commented Apr 3, 2017

IMO we should not have any playground specific URI knowledge in the core.

Agree

A Playground should be contributable by an extension without it being necessary to modify the core.

Disagree. The extension API is there to enrich the core development cycle not these scenarios. IMO the playground should use the standalone editor with its language service implementation. It's problematic that playground samples bleed into the extension host and causing all kinds issues, like markers we don't really want.

@jrieken jrieken modified the milestones: Backlog, April 2017 Apr 5, 2017
@jrieken
Copy link
Member

jrieken commented Nov 17, 2017

This feature request will not be considered in the next 6-12 months roadmap and as such will be closed to keep the number of issues we have to maintain actionable. Thanks for understanding and happy coding!

@jrieken jrieken closed this as completed Nov 17, 2017
@vscodebot vscodebot bot locked and limited conversation to collaborators Jan 1, 2018
@jrieken jrieken modified the milestones: Backlog, April 2018 Apr 6, 2018
@jrieken jrieken reopened this Apr 6, 2018
@jrieken
Copy link
Member

jrieken commented Apr 6, 2018

Revisiting this for April...

@microsoft microsoft unlocked this conversation Apr 9, 2018
@jrieken jrieken added the under-discussion Issue is under discussion for relevance, priority, approach label Apr 12, 2018
jrieken added a commit that referenced this issue Apr 12, 2018
@jrieken
Copy link
Member

jrieken commented Apr 12, 2018

Idea is to educate because it's hard to make the right (breaking) decision for every extension. As a first step, I have added an info-message that gets logged once for extensions that are under development and that use a lax-selector. Next step is a page with more documentation/information that we can link to.

@jrieken
Copy link
Member

jrieken commented Apr 18, 2018

@gregvanl Can you please review the text here: https://go.microsoft.com/fwlink/?linkid=872305. We should also find a place in the docs for this.

/cc @egamma

@jrieken jrieken closed this as completed Apr 20, 2018
@vscodebot vscodebot bot locked and limited conversation to collaborators Jun 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api feature-request Request for new features or functionality on-testplan under-discussion Issue is under discussion for relevance, priority, approach
Projects
None yet
Development

No branches or pull requests

3 participants