Tree : Use of dot decorations on folders (colored according to the priority) by OmarSdt-EC · Pull Request #9473 · eclipse-theia/theia

OmarSdt-EC · 2021-05-12T17:57:20Z

What it does

Fixes #6904
The following commit updates the rendering of tail decorations for
composite tree nodes (nodes that contain children nodes like folders).

The following updates the rendering of tail decorations for composite
tree nodes (nodes that contain children nodes like folders). The
updates include :

rendering a generic icon decoration (dot) for composite nodes with children with decoration data.
updates to the rendering logic to only render the decoration data with the highest priority so no duplicate generic icons are present.

How to test

Start the Theia application
Open a project that is under version control
Try to modify some files in it, you should see a dot decorator on the parent folder
Try to create an error in one of the children files, you should see the color of the dot change (it should be red)
If you commit and push your changes in your remote repository, you shouldn't see the tail decorators anymore

Review checklist

as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

as a reviewer, I agree to behave in accordance with the review guidelines

vince-fugnitto · 2021-05-13T15:19:10Z

The second commit ea23664 should be dropped before merging.
The commit is used for test purposes to confirm if multiple tailDecorations for a given composite node is present that we do not render multiple generic icons.

packages/core/src/browser/tree/tree-widget.tsx

colin-grant-work · 2021-05-13T15:07:56Z

packages/markers/src/browser/problem/problem-decorator.ts

            },
+            tailDecorations: [
+                {
+                    data: 'allo',


Is this data field meaningful?

Please remove this extraneous data that I assume was for testing purposes.

Sorry Colin for not answering your question, I completely forgot. Indeed, it was for testing purposes

Me and Vince planned to remove it before merging, we used it only to check if the icon would have the color of the highest priority. Problem markers shouldn't be in my PR since Vince had a commit about them. It should be dropped before merging mine.

packages/core/src/browser/tree/tree-widget.tsx

OmarSdt-EC · 2021-05-20T11:35:20Z

@colin-grant-work Hi, I think that all the changes are done and that I addressed all the comments. Can you do a re-review ?

vince-fugnitto

Thinking about the feature a bit further I'm wondering if we should override renderTailDecorations solely in the navigator-widget so we do not potentially break other trees, or introduce an unwanted behavior. I think the behavior of displaying a generic tail decoration for composite nodes (folders) can be exclusive to the navigator.

Reasoning:

the navigator will still properly display the generic decoration for composite nodes.
other trees are left untouched, which means no unwanted default behavior will be present.
downstream extensions can still provide composite tailDecorations to other trees if they choose to do so.

colin-grant-work

This appears to be working well for me. Just a couple of very minor comments on the code.

colin-grant-work · 2021-06-16T20:22:09Z

packages/core/src/browser/tree/tree-widget.tsx

-            // eslint-disable-next-line no-null/no-null
-            return null;
+    protected decorateIcon(node: TreeNode, icon: React.ReactNode): React.ReactNode {
+        // icon can be null or undefined


I don't think this comment is necessary - the !icon check makes it clear you're checking for all falsey values, and no one who isn't checking this diff will know that the code used to assume only null was a possibility. :-)

I was surprised and I asked Omar to add that comment... ReactNode is a weird type, but I assume you are right: just need to Go To Type Definition and we can see that it includes null.

I will remove the comment thanks !

colin-grant-work · 2021-06-16T20:25:04Z

packages/markers/src/browser/problem/problem-decorator.ts

            },
+            tailDecorations: [
+                {
+                    data: 'allo',


Please remove this extraneous data that I assume was for testing purposes.

colin-grant-work · 2021-06-16T20:26:44Z

packages/navigator/src/browser/navigator-widget.tsx

+        const { tooltip } = decoration as TreeDecoration.TailDecoration;
+        const { fontData } = decoration as TreeDecoration.TailDecoration;


Suggested change

const { tooltip } = decoration as TreeDecoration.TailDecoration;

const { fontData } = decoration as TreeDecoration.TailDecoration;

const { tooltip, fontData } = decoration as TreeDecoration.TailDecoration;

colin-grant-work · 2021-06-21T20:17:05Z

It looks like this PR may also fix #6904.

vince-fugnitto

The changes work well for me 👍
@colin-grant-work did you have any additional feedback?

colin-grant-work · 2021-07-26T17:13:46Z

@colin-grant-work did you have any additional feedback?

Sorry for the delay. Looks good to me!

The following commit updates the rendering of tail decorations for composite tree nodes (nodes that contain children nodes like folders). The following updates the rendering of tail decorations for composite tree nodes (nodes that contain children nodes like folders). The updates include : - rendering a generic icon decoration (dot) for composite nodes with children with decoration data. - updates to the rendering logic to only render the decoration data with the highest priority so no duplicate generic icons are present.

OmarSdt-EC force-pushed the os/fix-scmStateMarkers branch 2 times, most recently from 7657b66 to 01e816d Compare May 12, 2021 19:27

OmarSdt-EC changed the title ~~Os/fix scm state markers~~ Tree : Use of dot decorations on folders that are colored according to the priority of the modifications May 13, 2021

OmarSdt-EC changed the title ~~Tree : Use of dot decorations on folders that are colored according to the priority of the modifications~~ Tree : Use of dot decorations on folders (colored according to the priority) May 13, 2021

colin-grant-work self-requested a review May 13, 2021 14:00

vince-fugnitto force-pushed the os/fix-scmStateMarkers branch 2 times, most recently from ac3bd7e to ea23664 Compare May 13, 2021 15:15

vince-fugnitto added tree issues related to the tree (ex: tree widget) ui/ux issues related to user interface / user experience labels May 13, 2021

colin-grant-work reviewed May 13, 2021

View reviewed changes

OmarSdt-EC force-pushed the os/fix-scmStateMarkers branch 3 times, most recently from 035e10a to 7d25474 Compare May 13, 2021 22:43

paul-marechal reviewed May 13, 2021

View reviewed changes

OmarSdt-EC force-pushed the os/fix-scmStateMarkers branch 2 times, most recently from c0c3f14 to 7ebcb33 Compare May 13, 2021 23:31

OmarSdt-EC closed this May 20, 2021

OmarSdt-EC reopened this May 20, 2021

vince-fugnitto reviewed May 21, 2021

View reviewed changes

OmarSdt-EC force-pushed the os/fix-scmStateMarkers branch 3 times, most recently from 0388a2b to 8e9e879 Compare May 25, 2021 15:16

colin-grant-work self-requested a review June 15, 2021 16:39

colin-grant-work approved these changes Jun 16, 2021

View reviewed changes

OmarSdt-EC force-pushed the os/fix-scmStateMarkers branch 3 times, most recently from 6f445c8 to 6dd275a Compare July 21, 2021 15:30

OmarSdt-EC force-pushed the os/fix-scmStateMarkers branch from 6dd275a to e15e0d0 Compare July 21, 2021 15:36

vince-fugnitto approved these changes Jul 21, 2021

View reviewed changes

OmarSdt-EC force-pushed the os/fix-scmStateMarkers branch from e15e0d0 to b9d18a8 Compare August 26, 2021 18:09

paul-marechal merged commit b330ab7 into eclipse-theia:master Aug 26, 2021

paul-marechal added this to the 1.17.0 milestone Aug 26, 2021

colin-grant-work mentioned this pull request Mar 1, 2022

Use tail decorations for problems in TreeWidget #10820

Merged

1 task

vince-fugnitto mentioned this pull request Apr 13, 2022

Symlink Decorator on Folders Appearing as Bubble #11024

Closed

		const { tooltip } = decoration as TreeDecoration.TailDecoration;
		const { fontData } = decoration as TreeDecoration.TailDecoration;

	const { tooltip } = decoration as TreeDecoration.TailDecoration;
	const { fontData } = decoration as TreeDecoration.TailDecoration;
	const { tooltip, fontData } = decoration as TreeDecoration.TailDecoration;

Conversation

OmarSdt-EC commented May 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What it does

How to test

Review checklist

Reminder for reviewers

Uh oh!

vince-fugnitto commented May 13, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

OmarSdt-EC commented May 20, 2021

Uh oh!

vince-fugnitto left a comment

Choose a reason for hiding this comment

Uh oh!

colin-grant-work left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paul-marechal Jun 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

colin-grant-work commented Jun 21, 2021

Uh oh!

vince-fugnitto left a comment

Choose a reason for hiding this comment

Uh oh!

colin-grant-work commented Jul 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

OmarSdt-EC commented May 12, 2021 •

edited

Loading

paul-marechal Jun 17, 2021 •

edited

Loading