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

[output] add severity/color to appendLine method #7549

Merged
merged 1 commit into from
Apr 20, 2020

Conversation

amiramw
Copy link
Member

@amiramw amiramw commented Apr 11, 2020

Signed-off-by: Amiram Wingarten amiram.wingarten@sap.com

Fixes #7504

What it does

Add severity to append methods of output channel. This allow to color output lines for errors and warnings.

How to test

For some output channel call appendLine with severities info, warning and error.
The output lines should color accordingly, warning in yellow and error in red.

Call append method - it should behave as info.

See example examples/api-samples/src/browser/output/sample-output-channel-with-severity.ts

Also test application can be found in #7600

const channel = this.outputChannelManager.getChannel('my test channel');
channel.appendLine('hello info1');
channel.appendLine('hello info2', OutputChannelSeverity.info);
channel.appendLine('hello error', OutputChannelSeverity.Error);
channel.appendLine('hello warning', OutputChannelSeverity.Warning);
channel.append('append info1 ');
channel.append('append info2');

The expected output channel "my test channel" should look like this after theia start:

image

Review checklist

Reminder for reviewers

@vince-fugnitto vince-fugnitto added the output issues related to the output label Apr 13, 2020
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

@amiramw do you mind updating the pull-request's 'how to test' so a reviewer can verify the output for both errors and warnings? I was unable to do so with the description provided.

const lines = outputChannelLine.text.split(/[\n\r]+/);
let className;
if (outputChannelLine.severity === OutputChannelSeverity.Error) {
className = 'theia-console-error';
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure these are the correct classes to use as they are contributed by console.
It is possible to build an application without console, and in this scenario, coloring present in the output view will not work.

Copy link
Member Author

Choose a reason for hiding this comment

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

I created new classes in output package styles

@akosyakov
Copy link
Member

Would it be possible to keep it after we switch to use Monaco editor for the output to align with VS Code and get features like search and so on?

@amiramw
Copy link
Member Author

amiramw commented Apr 15, 2020

@akosyakov sure. But will colors be possible after the move? VS code doesn't have this feature as far as I know.

@akosyakov
Copy link
Member

@amiramw It was my question :) But thinking now, Monaco editor has decorators API to color text. So it should be.

Please address @vince-fugnitto comment: #7549 (comment)

The output extension should not have implicit dependencies on the console extension via css classes.

@amiramw
Copy link
Member Author

amiramw commented Apr 17, 2020

@akosyakov i wasn't aware that vscode uses monaco editor component also for the output.

Anyway for now I'm addressing CR comments...

@amiramw amiramw changed the title [output] add severity/color to append API [output] add severity/color to appendLine method Apr 17, 2020
@amiramw
Copy link
Member Author

amiramw commented Apr 17, 2020

@vince-fugnitto I updated the how to test section. There is another PR with test example.

@amiramw amiramw force-pushed the output branch 2 times, most recently from a567ee7 to 88ae8e8 Compare April 18, 2020 20:23
@amiramw
Copy link
Member Author

amiramw commented Apr 20, 2020

Any clue why the build is failing now?

@akosyakov
Copy link
Member

@amiramw I'm looking into the build failure.

@kittaakos
Copy link
Contributor

Adding a new dependency to @theia/api-sampl should update the examples/api-samples/compile.tsconfig.json with the path too @theia/output when you run the build. Did you commit all the file changes?

@amiramw
Copy link
Member Author

amiramw commented Apr 20, 2020

I didn't, thought it was generated wrongly. I'll check again.

Signed-off-by: Amiram Wingarten <amiram.wingarten@sap.com>
@amiramw
Copy link
Member Author

amiramw commented Apr 20, 2020

@kittaakos Seems that build is fine now and i fixed your comment.

Copy link
Contributor

@kittaakos kittaakos left a comment

Choose a reason for hiding this comment

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

I like the idea, the implementation will probably change as soon as we switch to the editor. 👍

I have verified it with the API sample:
Screen Shot 2020-04-20 at 17 21 38

@kittaakos
Copy link
Contributor

@amiramw, ping me if you cannot do the merge.

@amiramw
Copy link
Member Author

amiramw commented Apr 20, 2020

Thansks @kittaakos! Merging.

@amiramw amiramw merged commit 63138d3 into eclipse-theia:master Apr 20, 2020
@amiramw amiramw deleted the output branch April 20, 2020 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
output issues related to the output
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support colors for logs in OutputChannel
4 participants