Use problem manager to handle markers from plugins #4178
Conversation
| @@ -104,7 +104,7 @@ export class ProblemWidget extends TreeWidget { | |||
| <i className={severityClass}></i> | |||
| </div> | |||
| <div className='owner'> | |||
| {'[' + problemMarker.owner + ']'} | |||
| {'[' + problemMarker.data.source + ']'} | |||
There was a problem hiding this comment.
problemMarker.data.source || problemMarker.owner in the case if source is not provided
There was a problem hiding this comment.
I don't agree here: if I am correct that "owner" is a technical Identifier, we should not show it to the user. In that case "<none>" or the empty string would be appropriate.
We're mapping the id of the diagnostic collection to "owner": that is a technical id.
There was a problem hiding this comment.
"owner": that is a technical id.
Where did you get that? Before this change it was not used as a technical identifier, but as source, so there are client expecting it to be used.
I like the change, since it can show now more refined source, but if it is not present the PR should not break existing behaviour.
There was a problem hiding this comment.
"owner": that is a technical id.
Where did you get that? Before this change it was not used as a technical identifier, but as
source, so there are client expecting it to be used.
Owner is filled from the id of the diagnostics collection (when going through the plugin API). The diagnostics id can be generated, and as such is a technical thing (think "diagnostic_collection_id_12709fajkö")
Therefore I think that the behaviour before was actually wrong and anyone relying on it should fix.
There was a problem hiding this comment.
The plugin extension is not only client. There are other theia extensions which are using ProblemManager and they provide proper owner. They should work as before even if the plugin extension is not installed at all.
There was a problem hiding this comment.
anyway, I've tested with existing languages and it seems to work, let's give it a try
There was a problem hiding this comment.
I've changed my mind on this one: if a contribution does not set the "source" on the diagnostics, he might very well have interpreted the API in the sense that Theia did. So I think it's actually a good fallback
| // note: language server range is 0-based, marker is 1-based, so need to deduct 1 here | ||
| return { | ||
| start: { | ||
| line: startLine - 1, |
There was a problem hiding this comment.
If possible please use https://github.com/theia-ide/theia/blob/master/packages/plugin-ext/src/plugin/type-converters.ts for such conversions.
There was a problem hiding this comment.
Again, nope, this is on the "main" side of the API. The conversions in type-converter.ts are in the other direction.
There was a problem hiding this comment.
again, yes
type-converter is converting both ways (and the name is generic enough and is in an upper package to be widely used)
example for Position: https://github.com/theia-ide/theia/blob/master/packages/plugin-ext/src/plugin/type-converters.ts#L96-L102
it has from and to
There was a problem hiding this comment.
Sorry, but no: the file you refer to is in the "plugin" subtree and converts between plugin API objects and the main/ext API. The conversion here happens on the other side of the main/ext API and converts to objects from the module 'vscode-languageserver-types'.
Also, the this file already contains a number of functions that convert from main/ext types to monaco or theia internal types. So my change here conforms to the already existing code.
There was a problem hiding this comment.
type-converter is in plug-in subtree and you're converting stuff in the file packages/plugin-ext/src/main/browser/languages-main.ts so it's the same subtree.
It's converting types coming from `vscode-languageserver-types'
you introduced a local scope
import * as vst from 'vscode-languageserver-types'; to convert data to the same origin,
so it should be in https://github.com/theia-ide/theia/blob/master/packages/plugin-ext/src/plugin/type-converters.ts
There was a problem hiding this comment.
Yes, technically, it can, but if the type converter provides services to both the plugin and main side of things, it should be moved to the "common" folder, otherwise, why do we even have the subfolders? I feel that is outside of the scope of this PR.
Anyway, we're having a stylistic difference here we're not going to resolve amicably. How should we proceed?
There was a problem hiding this comment.
src/mainsubtree can import anything fromsrc/plugin
I agree that managing conversions in one place would be good. But I did not think that the code from main module can access VS Code types, or need to load them at runtime. Does it?
What about getting rid of MarkerData? Theia expect LSP diagnostics which are JSON already, so VS Code diagnostics can be converted to LSP in the host plugin process. No need for conversions in the main module.
There was a problem hiding this comment.
@akosyakov you mean this line https://github.com/theia-ide/theia/blob/master/packages/plugin-ext/src/plugin/type-converters.ts#L27 ? (imports of vscode-types ) ?
or that https://github.com/theia-ide/theia/blob/a83fb71067315159706ab274f31e2fa5c4393644/packages/plugin-ext/src/main/browser/languages-main.ts (language main) can't access type-converter ?
I'm also fine with your proposal
There was a problem hiding this comment.
or that https://github.com/theia-ide/theia/blob/a83fb71067315159706ab274f31e2fa5c4393644/packages/plugin-ext/src/main/browser/languages-main.ts (language main) can't access
type-converter?
I meant that when we import type-converters in main, it pulls JS files like types-impl and other internal code of the host plugin process (even implementations of *Ext interfaces) which gets bundled by webpack or loaded by main server node process. Is it really necessary, expected?
I believe it is not caused by Thomas changes and quite a big separate issue.
There was a problem hiding this comment.
I've opened an issue for it - #4271
Thinking more about it, I am not sure that we can do all conversions in one file. There are internal VS Code types for plugin module and internal Monaco types for main module, trying to make conversion between them in one file will always cause such issues. Common converter can only depend on theia namespace, LSP and plugin DTO types.
| // note: language server range is 0-based, marker is 1-based, so need to deduct 1 here | ||
| return { | ||
| start: { | ||
| line: startLine - 1, |
There was a problem hiding this comment.
again, yes
type-converter is converting both ways (and the name is generic enough and is in an upper package to be widely used)
example for Position: https://github.com/theia-ide/theia/blob/master/packages/plugin-ext/src/plugin/type-converters.ts#L96-L102
it has from and to
|
@akosyakov no, type converter should be at a single location and not dispatched across all the code. |
|
@akosyakov and most of the time, I never see these follow-up PR |
Signed-off-by: Thomas Mäder <[email protected]>
tsmaeder
force-pushed
the
4097_plugin_diagnostics
branch
from
a158cb5 to
8c82eda
Compare
This is a fix for #4097. The problem was that markers from plugins were not pushed through the problem manager, but into the Monaco editor directly.
My test case for this change is installing vscode-java via "Deploy plugin by id" and I used the typescript extension to make sure that theia extensions are not broken.
The problems view used to show the name of the diagnostics collections, but IMO this was wrong, since that seems to be a technical name: the plugin API even allows for it to be generated (that is what vscode-java does). Now the problems view shows the "source" attribute of the Diagnostic.