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

Add indent guidelines to all trees #7194

Closed
wants to merge 1 commit into from
Closed

Add indent guidelines to all trees #7194

wants to merge 1 commit into from

Conversation

kaiyue0329
Copy link
Contributor

@kaiyue0329 kaiyue0329 commented Feb 21, 2020

What it does

Fixes #6586

  • Add preference to set 'workbench.tree.renderIndentGuides' to 'onHover' (default), 'none' or 'always'
  • When nodes are selected, indent guidelines are rendered for all the sibling nodes
  • When parent node is selected, indent guidelines are rendered for all the child nodes
  • When hovering over a tree, indent guidelines are displayed for all expanded nodes
  • Selecting multiple nodes works in a similar fashion

How to test

  • Set the workspace preference to 'onHover', 'none', 'always' and the rendering of the indent guidelines should change accordingly.
  • In different locations in Theia where the tree is being used (for example, in navigator, search in workspace, outline, type hierarchy), select one/more nodes to test if the indent guidelines are rendered correctly.

Review checklist

Reminder for reviewers

@paul-marechal paul-marechal added help wanted issues meant to be picked up, require help tree issues related to the tree (ex: tree widget) ui/ux issues related to user interface / user experience labels Feb 21, 2020
@akosyakov akosyakov self-requested a review February 25, 2020 09:24
@paul-marechal
Copy link
Member

@akosyakov I looked through VS Code's implementation together with @kaiyue0329, and it is not trivial at all. Something that doesn't help is the fact that we are using React, whereas they are using raw DOM manipulations. Porting the code is a bit of a headache but I think we found a way to get it working.

We will need to keep some state within the widget as some kind of cache though, because it will be a bit expensive to re-compute for each node on each frame. I don't see how we can properly avoid this...

@akosyakov
Copy link
Member

We will need to keep some state within the widget as some kind of cache though, because it will be a bit expensive to re-compute for each node on each frame. I don't see how we can properly avoid this...

I would need sometime to understand whether it cause race conditions and performance implications for big trees of it. I see that we do traversing again and so on, plus mutate instance state during rendering of a particular state. It can cause perf issues and state inconsistencies. Sorry I don't have time to dive into it right now. You can try to test for performance by generating many errors markers for instance with Java. I think there were a PR we fixed it with steps to reproduce.

@paul-marechal
Copy link
Member

@kaiyue0329 you might want to rebase once you are happy with your changes.

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 functionality looks really good! Very nice change 👍
I tested using a few trees (explorer, search, outline, problems) and it all worked as intended.

packages/core/src/browser/tree/tree-widget.tsx Outdated Show resolved Hide resolved
@kaiyue0329
Copy link
Contributor Author

kaiyue0329 commented Mar 3, 2020

The performance looks good from a user's perspective. However, I was wondering if anyone knows a good way to measure/benchmark the time that it takes to render the indent guideline? It would be nice to see if rendering the indent guideline slows things down performance-wise.

A good place to put the methods for measuring time would be around:

       const content = <div className={TREE_NODE_CONTENT_CLASS}>
            <div className={TREE_NODE_INDENT_CLASS}>
                {this.renderIndent(node, props.depth)}
            </div>
            {this.renderExpansionToggle(node, props)}
            {this.decorateIcon(node, this.renderIcon(node, props))}
            {this.renderCaptionAffixes(node, props, 'captionPrefixes')}
            {this.renderCaption(node, props)}
            {this.renderCaptionAffixes(node, props, 'captionSuffixes')}
            {this.renderTailDecorations(node, props)}
        </div>;

Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

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

LGTM, performance doesn't feel worse on my end.

@paul-marechal paul-marechal requested a review from akosyakov March 9, 2020 19:42
@vince-fugnitto
Copy link
Member

@kaiyue0329 can you please rebase to fix the merge conflicts?

- Add preference to set 'workbench.tree.renderIndentGuides' to 'onHover' (default), 'none' or 'always'
- When nodes are selected, indent guidelines are rendered for all the sibling nodes
- When parent node is selected, indent guidelines are rendered for all the child nodes
- Selecting multiple nodes works in a similar fashion

Signed-off-by: Kaiyue Pan <[email protected]>
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 will review it later. Sorry, trees are too important.

@akosyakov
Copy link
Member

I will try to look into it after this release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted issues meant to be picked up, require help tree issues related to the tree (ex: tree widget) ui/ux issues related to user interface / user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ui/ux][tree] Help the tree indentation with vertical lines, mimic VS Code
4 participants