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

Fixed #1278: implemented tab bar decorator & supported error marker in editor tabs #5845

Merged
merged 1 commit into from
Aug 22, 2019

Conversation

fangnx
Copy link

@fangnx fangnx commented Aug 2, 2019

What it does

Fixed #1278.

  • Implemented TabBarDecorator that provides tabs with decorations, similar to what we already had for tree nodes.
  • Supported diagnostic problem markers (error, warning, ...) in editor tabs in the main area. Tabs in side bars can be decorated as well in the future using the same code.
  • Refactored TreeDecoration to a more generic NodeDecoration, which is currently used for decorating tree nodes and tabbar tabs.

How to test

  • Open editors with errors/warnings identified. If they are in the main tab bar (horizontal), small error/warning icons should be seen overlaying on top of the file icon.
  • Once problems in a file are resolved, the error/warning decorator should disappear.

Review checklist

Reminder for reviewers

@akosyakov akosyakov added editor issues related to the editor markers issues related to problem markers shell issues related to the core shell labels Aug 3, 2019
@vince-fugnitto
Copy link
Member

@fangnx ping me when the PR is ready for a review 😃

@fangnx fangnx force-pushed the fnx-fix-1278 branch 2 times, most recently from 4d083e2 to d7c75da Compare August 5, 2019 20:16
@fangnx fangnx force-pushed the fnx-fix-1278 branch 3 times, most recently from 8361c65 to 9633236 Compare August 8, 2019 18:04
@fangnx fangnx changed the title WIP: Fixed #1278: implemented error marker for editor tabs Fixed #1278: implemented tabbar decorator & supported error marker in editor tabs Aug 8, 2019
@fangnx fangnx marked this pull request as ready for review August 8, 2019 18:05
@fangnx fangnx force-pushed the fnx-fix-1278 branch 2 times, most recently from dc8fa17 to 1d366e5 Compare August 8, 2019 18:13
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.

Few minor comments, the code is still untested on my end.

packages/core/src/browser/shell/tab-bar-decorator.ts Outdated Show resolved Hide resolved
packages/core/src/browser/shell/tab-bar-decorator.ts Outdated Show resolved Hide resolved
packages/markers/src/browser/problem/problem-decorator.ts Outdated Show resolved Hide resolved
packages/core/src/browser/shell/tab-bar-decorator.ts Outdated Show resolved Hide resolved
packages/core/src/browser/shell/tab-bar-decorator.ts Outdated Show resolved Hide resolved
packages/core/src/browser/shell/tab-bar-decorator.ts Outdated Show resolved Hide resolved
packages/core/src/browser/shell/tab-bar-decorator.ts Outdated Show resolved Hide resolved
@akosyakov
Copy link
Member

akosyakov commented Aug 9, 2019

@fangnx please update the description according the project template: https://github.com/theia-ide/theia/blob/master/doc/pull-requests.md#pr-template

May I ask why did you delete in the first place? Do you use some client which does not propose it?

@fangnx fangnx force-pushed the fnx-fix-1278 branch 2 times, most recently from 10d05fd to aec4bca Compare August 13, 2019 18:32
@vince-fugnitto
Copy link
Member

@fangnx there still seems to be an issue regarding CI, something concerning illegal characters in the method getDecorations.

@fangnx fangnx force-pushed the fnx-fix-1278 branch 3 times, most recently from cf582fb to a63f7c5 Compare August 15, 2019 18:55
@akosyakov
Copy link
Member

akosyakov commented Aug 16, 2019

I think code-wise it looks good. I've tried changes and it behaves well in the main area. I have following concerns:

  • why don't we do it in the side-bars as well? It will be consistent and we will be able to reuse to show count of changes for the scm title.
  • do we a need a preference to disable decorations? It can be useful if it would cause some perf issues or a user just does not want it. Is there such preferences in VS Code? Maybe we can have a preference for problems at least, something like problem.decorations.enabled by analogy with the git.decorations.enabled.

@fangnx fangnx force-pushed the fnx-fix-1278 branch 2 times, most recently from 5970863 to befcddd Compare August 20, 2019 18:29
@fangnx
Copy link
Author

fangnx commented Aug 20, 2019

I think code-wise it looks good. I've tried changes and it behaves well in the main area. I have following concerns:

  • why don't we do it in the side-bars as well? It will be consistent and we will be able to reuse to show count of changes for the scm title.
  • do we a need a preference to disable decorations? It can be useful if it would cause some perf issues or a user just does not want it. Is there such preferences in VS Code? Maybe we can have a preference for problems at least, something like problem.decorations.enabled by analogy with the git.decorations.enabled.

Just did it in the side bars in the latest push, with larger problem decorators shown. I also added a preference that can turn on/off the display of problem decorators in the tab bars, as you suggested.

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

It works very well for me and code looks good, although pay attention to the last comment.

@vince-fugnitto Would you be fine to merge?

@vince-fugnitto
Copy link
Member

It works very well for me and code looks good, although pay attention to the last comment.

@vince-fugnitto Would you be fine to merge?

Sure, I'll merge once your last comment has been addressed and the CI re-passes :)

@vince-fugnitto
Copy link
Member

@fangnx can you please rebase the PR?

@fangnx
Copy link
Author

fangnx commented Aug 21, 2019

@fangnx can you please rebase the PR?

Done ;)

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.

Please address my comments before we can successfully merge :)

CHANGELOG.md Outdated Show resolved Hide resolved
packages/core/src/browser/shell/tab-bar-decorator.ts Outdated Show resolved Hide resolved
packages/core/src/browser/shell/tab-bar-decorator.ts Outdated Show resolved Hide resolved
packages/core/src/browser/shell/tab-bars.ts Show resolved Hide resolved
packages/core/src/browser/shell/tab-bars.ts Outdated Show resolved Hide resolved
packages/core/src/browser/style/tree-decorators.css Outdated Show resolved Hide resolved
…rror marker in editor tabs

- Implemented `TabBarDecorator` that provides tabs with decorations, similar to what we already had for tree nodes.
- Supported diagnostic problem markers (error, warning, ...) in editor tabs in the main area. Tabs in side bars can be decorated as well in the future using the same code.
- Refactored `TreeDecoration` to a more generic `NodeDecoration`, which is currently used for decorating tree nodes and tabbar tabs.

Signed-off-by: fangnx <[email protected]>
@fangnx fangnx changed the title Fixed #1278: implemented tabbar decorator & supported error marker in editor tabs Fixed #1278: implemented tab bar decorator & supported error marker in editor tabs Aug 22, 2019
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.

It works very well, really nice work :) 👍

I tested using different file types, in both the main, bottom, left and right side bars, and tested whether the preference is properly emitted and listened to.

@vince-fugnitto vince-fugnitto merged commit 06346e5 into eclipse-theia:master Aug 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editor issues related to the editor markers issues related to problem markers shell issues related to the core shell
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[editor][diagnostics] Support error/warning marker in the editor tabs
3 participants