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.

Sign up for GitHub

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

Use problem manager to handle markers from plugins #4178

Merged
merged 1 commit into from
Feb 27, 2019
Merged

Use problem manager to handle markers from plugins #4178

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/markers/src/browser/problem/problem-widget.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ export class ProblemWidget extends TreeWidget {
<i className={severityClass}></i>
</div>
<div className='owner'>
{'[' + problemMarker.owner + ']'}
{'[' + (problemMarker.data.source || problemMarker.owner) + ']'}
</div>
<div className='message'>{problemMarker.data.message}
{
Expand Down
1 change: 1 addition & 0 deletions packages/plugin-ext/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
"@theia/editor": "^0.3.19",
"@theia/file-search": "^0.3.19",
"@theia/filesystem": "^0.3.19",
"@theia/markers": "^0.3.19",
"@theia/messages": "^0.3.19",
"@theia/monaco": "^0.3.19",
"@theia/navigator": "^0.3.19",
Expand Down
2 changes: 1 addition & 1 deletion packages/plugin-ext/src/api/model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ export interface MarkerData {
}

export interface RelatedInformation {
resource: UriComponents;
resource: string;
message: string;
startLineNumber: number;
startColumn: number;
Expand Down
2 changes: 1 addition & 1 deletion packages/plugin-ext/src/api/plugin-api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -900,7 +900,7 @@ export interface LanguagesMain {
$registerDocumentHighlightProvider(handle: number, selector: SerializedDocumentFilter[]): void;
$registerQuickFixProvider(handle: number, selector: SerializedDocumentFilter[], codeActionKinds?: string[]): void;
$clearDiagnostics(id: string): void;
$changeDiagnostics(id: string, delta: [UriComponents, MarkerData[]][]): void;
$changeDiagnostics(id: string, delta: [string, MarkerData[]][]): void;
$registerDocumentFormattingSupport(handle: number, selector: SerializedDocumentFilter[]): void;
$registerRangeFormattingProvider(handle: number, selector: SerializedDocumentFilter[]): void;
$registerOnTypeFormattingProvider(handle: number, selector: SerializedDocumentFilter[], autoFormatTriggerCharacters: string[]): void;
Expand Down
86 changes: 51 additions & 35 deletions packages/plugin-ext/src/main/browser/languages-main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,25 +27,29 @@ import {
ResourceFileEditDto,
} from '../../api/plugin-api';
import { interfaces } from 'inversify';
import { SerializedDocumentFilter, MarkerData, Range, WorkspaceSymbolProvider } from '../../api/model';
import { SerializedDocumentFilter, MarkerData, Range, WorkspaceSymbolProvider, RelatedInformation, MarkerSeverity } from '../../api/model';
import { RPCProtocol } from '../../api/rpc-protocol';
import { fromLanguageSelector } from '../../plugin/type-converters';
import { UriComponents } from '../../common/uri-components';
import { LanguageSelector } from '../../plugin/languages';
import { DocumentFilter, MonacoModelIdentifier, testGlob, getLanguages } from 'monaco-languageclient/lib';
import { DisposableCollection, Emitter } from '@theia/core';
import { MonacoLanguages } from '@theia/monaco/lib/browser/monaco-languages';
import URI from 'vscode-uri/lib/umd';
import CoreURI from '@theia/core/lib/common/uri';
import { ProblemManager } from '@theia/markers/lib/browser';
import * as vst from 'vscode-languageserver-types';

export class LanguagesMainImpl implements LanguagesMain {

private ml: MonacoLanguages;
private problemManager: ProblemManager;

private readonly proxy: LanguagesExt;
private readonly disposables = new Map<number, monaco.IDisposable>();
constructor(rpc: RPCProtocol, container: interfaces.Container) {
this.proxy = rpc.getProxy(MAIN_RPC_CONTEXT.LANGUAGES_EXT);
this.ml = container.get(MonacoLanguages);
this.problemManager = container.get(ProblemManager);
}

$getLanguages(): Promise<string[]> {
Expand Down Expand Up @@ -158,24 +162,15 @@ export class LanguagesMainImpl implements LanguagesMain {
}

$clearDiagnostics(id: string): void {
const markers = monaco.editor.getModelMarkers({ owner: id });
const clearedEditors = new Set<string>(); // uri to resource
for (const marker of markers) {
const uri = marker.resource;
const uriString = uri.toString();
if (!clearedEditors.has(uriString)) {
const textModel = monaco.editor.getModel(uri);
monaco.editor.setModelMarkers(textModel, id, []);
clearedEditors.add(uriString);
}
for (const uri of this.problemManager.getUris()) {
this.problemManager.setMarkers(new CoreURI(uri), id, []);
}
}

$changeDiagnostics(id: string, delta: [UriComponents, MarkerData[]][]): void {
for (const [uriComponents, markers] of delta) {
const uri = monaco.Uri.revive(uriComponents);
const textModel = monaco.editor.getModel(uri);
monaco.editor.setModelMarkers(textModel, id, markers.map(reviveMarker));
$changeDiagnostics(id: string, delta: [string, MarkerData[]][]): void {
for (const [uriString, markers] of delta) {
const uri = new CoreURI(uriString);
this.problemManager.setMarkers(uri, id, markers.map(reviveMarker));
}
}

Expand Down Expand Up @@ -698,35 +693,56 @@ export class LanguagesMainImpl implements LanguagesMain {
}
}

function reviveMarker(marker: MarkerData): monaco.editor.IMarkerData {
const monacoMarker: monaco.editor.IMarkerData = {
function reviveMarker(marker: MarkerData): vst.Diagnostic {
const monacoMarker: vst.Diagnostic = {
code: marker.code,
severity: marker.severity,
severity: reviveSeverity(marker.severity),
range: reviveRange(marker.startLineNumber, marker.startColumn, marker.endLineNumber, marker.endColumn),
message: marker.message,
source: marker.source,
startLineNumber: marker.startLineNumber,
startColumn: marker.startColumn,
endLineNumber: marker.endLineNumber,
endColumn: marker.endColumn,
relatedInformation: undefined
};

if (marker.relatedInformation) {
monacoMarker.relatedInformation = [];
for (const ri of marker.relatedInformation) {
monacoMarker.relatedInformation.push({
resource: monaco.Uri.revive(ri.resource),
message: ri.message,
startLineNumber: ri.startLineNumber,
startColumn: ri.startColumn,
endLineNumber: ri.endLineNumber,
endColumn: ri.endColumn
});
}
monacoMarker.relatedInformation = marker.relatedInformation.map(reviveRelated);
}

return monacoMarker;
}

function reviveSeverity(severity: MarkerSeverity): vst.DiagnosticSeverity {
switch (severity) {
case MarkerSeverity.Error: return vst.DiagnosticSeverity.Error;
case MarkerSeverity.Warning: return vst.DiagnosticSeverity.Warning;
case MarkerSeverity.Info: return vst.DiagnosticSeverity.Information;
case MarkerSeverity.Hint: return vst.DiagnosticSeverity.Hint;
}
}

function reviveRange(startLine: number, startColumn: number, endLine: number, endColumn: number): vst.Range {
// note: language server range is 0-based, marker is 1-based, so need to deduct 1 here
tsmaeder marked this conversation as resolved.
Show resolved Hide resolved
return {
start: {
line: startLine - 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

If possible please use https://github.com/theia-ide/theia/blob/master/packages/plugin-ext/src/plugin/type-converters.ts for such conversions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, nope, this is on the "main" side of the API. The conversions in type-converter.ts are in the other direction.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

src/main subtree can import anything from src/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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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

Copy link
Member

@akosyakov akosyakov Feb 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

character: startColumn - 1
},
end: {
line: endLine - 1,
character: endColumn - 1
}
};
}

function reviveRelated(related: RelatedInformation): vst.DiagnosticRelatedInformation {
return {
message: related.message,
location: {
uri: related.resource,
range: reviveRange(related.startLineNumber, related.startColumn, related.endLineNumber, related.endColumn)
}
};
}

function reviveRegExp(regExp?: SerializedRegExp): RegExp | undefined {
if (typeof regExp === 'undefined' || regExp === null) {
return undefined;
Expand Down
10 changes: 5 additions & 5 deletions packages/plugin-ext/src/plugin/languages/diagnostics.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ export class DiagnosticCollection implements theia.DiagnosticCollection {
if (this.has(uri)) {
this.fireDiagnosticChangeEvent(uri);
this.diagnostics.delete(uri.toString());
this.proxy.$changeDiagnostics(this.name, [[uri, []]]);
this.proxy.$changeDiagnostics(this.name, [[uri.toString(), []]]);
}
}

Expand Down Expand Up @@ -204,7 +204,7 @@ export class DiagnosticCollection implements theia.DiagnosticCollection {
}

private sendChangesToEditor(uris: URI[]): void {
const markers: [URI, MarkerData[]][] = [];
const markers: [string, MarkerData[]][] = [];
nextUri:
for (const uri of uris) {
const uriMarkers: MarkerData[] = [];
Expand All @@ -224,18 +224,18 @@ export class DiagnosticCollection implements theia.DiagnosticCollection {
endLineNumber: lastMarker.endLineNumber,
endColumn: lastMarker.endColumn
});
markers.push([uri, uriMarkers]);
markers.push([uri.toString(), uriMarkers]);
continue nextUri;
}
}
}
}
} else {
uriDiagnostics.forEach(diagnostic => uriMarkers.push(convertDiagnosticToMarkerData(diagnostic)));
markers.push([uri, uriMarkers]);
markers.push([uri.toString(), uriMarkers]);
}
} else {
markers.push([uri, []]);
markers.push([uri.toString(), []]);
}
}

Expand Down
2 changes: 1 addition & 1 deletion packages/plugin-ext/src/plugin/type-converters.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,7 +347,7 @@ function convertRelatedInformation(diagnosticsRelatedInformation: theia.Diagnost
const relatedInformation: model.RelatedInformation[] = [];
for (const item of diagnosticsRelatedInformation) {
relatedInformation.push({
resource: item.location.uri,
resource: item.location.uri.toString(),
message: item.message,
startLineNumber: item.location.range.start.line + 1,
startColumn: item.location.range.start.character + 1,
Expand Down