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

[markers] Sync problem markers with active editor #8172

Merged
merged 1 commit into from
Jul 23, 2020

Conversation

Anasshahidd21
Copy link
Contributor

What it does

Fixes: #7436

Reveals and expands the markers for the active editor.
Added a preference problems.autoReveal to control this behavior.

ezgif com-resize

Signed-off-by: Muhammad Anas Shahid muhammad.shahid@ericsson.com

How to test

  1. Open multiple editors with Problem markers ( one can create multiple .json files with dummy text)
  2. Change the active editor and check to see if, in the problems view, the active editor markers are selected.
  3. Collapse all markers, then change the active editor, check to see if the markers for active editor are expanded.
  4. From the preferences, goto problems.autoReveal uncheck it, and check to see if while changing the active editors the markers are expanded or selected. (in ideal case, they should not be expanded or selected)

Review checklist

Reminder for reviewers

@vince-fugnitto vince-fugnitto added the markers issues related to problem markers label Jul 14, 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.

I verified the changes and they work well 👍

problems.autoReveal = true:

  • the marker info nodes are correctly expanded and revealed (even when the tree is collapsed)

problems.autoReveal = false:

  • the behavior is like today, there is no reveal present when switching active editors

The only drawback is that preview editors are not revealed initially (likely since they are not active) until the editor is focused. I'm fine with this approach but just wanted to mention it works correctly in vscode.

@Anasshahidd21
Copy link
Contributor Author

Thank you for your review @vince-fugnitto.

The only drawback is that preview editors are not revealed initially (likely since they are not active) until the editor is focused. I'm fine with this approach but just wanted to mention it works correctly in vscode.

I have updated the code to make sure the behavior is consistent with editor-preview as well.

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.

I wonder why don't we follow on initial opening, but only a change?

packages/markers/src/browser/problem/problem-widget.tsx Outdated Show resolved Hide resolved
packages/markers/src/browser/problem/problem-widget.tsx Outdated Show resolved Hide resolved
@Anasshahidd21 Anasshahidd21 force-pushed the problemsSync branch 3 times, most recently from 0b58e8f to 8799906 Compare July 15, 2020 16:52
Copy link
Contributor

@kaiyue0329 kaiyue0329 left a comment

Choose a reason for hiding this comment

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

The changes work well. The correct problem markers are selected and expanded (if they are previously collapsed) when we change active editor. I also verify that unchecking problems.autoReveal preference works correctly.

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.

The changes work well 👍

I verified the behavior when:

  • switching the preference applies the following logic correctly
  • problemsAutoReveal: on:
    • The problem marker nodes are correctly expanded and selected when their corresponding editor or editor-preview are opened. The behavior also works well for new markers, and when the problems-view is collapsed.
  • problemsAutoReveal: off:
    • The problem marker nodes behave like master - they are not expanded and selected when their corresponding editors are opened. End-users must explicitly expand the nodes in the tree.

Fixes: eclipse-theia#7436

Reveals and expands the markers for the active editor.
Added a preference `problems.autoReveal` to control this behavior.

Signed-off-by: Muhammad Anas Shahid <muhammad.shahid@ericsson.com>
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.

code-wise looks good

@vince-fugnitto vince-fugnitto merged commit fd23988 into eclipse-theia:master Jul 23, 2020
@vince-fugnitto vince-fugnitto deleted the problemsSync branch July 23, 2020 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
markers issues related to problem markers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

markers: better sync between editor and problems-view
4 participants