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

fix(preference-tree-widget): sync. Theia and VSCode behavior about extensions' node naming #12929

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

VGRSTM
Copy link

@VGRSTM VGRSTM commented Sep 22, 2023

What it does

Fix #12928

Contributed by STMicroelectronics
Signed-off-by: Vincent GRENET vincent.grenet@st.com

How to test

  1. Hack package.json of your preferred VSCode extension
  2. Install it within Theia
  3. Just check configuration.title is now considered asking for Settings widget

Follow-ups

Review checklist

Reminder for reviewers

…tensions' node naming

Contributed by STMicroelectronics
Signed-off-by: Vincent GRENET <vincent.grenet@st.com>
@VGRSTM VGRSTM changed the title Pr/vgrstm bugfix 12928 fix(preference-tree-widget): sync. Theia and VSCode behavior about extensions' node naming Sep 22, 2023
@VGRSTM VGRSTM marked this pull request as ready for review September 22, 2023 16:22
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Alright, behavior looks mostly good, I just have a few remarks on the code and one on the behavior: The TypeScript extension provides preferences starting with typescript.* and javascript.*. Since both use the same title, but with a different group ID, these get listed twice as separate groups in the preferences:

image

VSCode shows them as expected only as a single group.

}

export namespace CompositeTreeNode {
export const is = (node: BaseTreeNode): node is CompositeTreeNode => 'depth' in node && 'title' in node;
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: Since title is an optional property, using 'title' in node can lead to false negative results.

Suggested change
export const is = (node: BaseTreeNode): node is CompositeTreeNode => 'depth' in node && 'title' in node;
export const is = (node: BaseTreeNode): node is CompositeTreeNode => 'depth' in node;

Copy link
Author

Choose a reason for hiding this comment

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

Good to me. I'll integrate this suggestion.

Comment on lines +168 to +169
const keys = Object.keys(config.properties);
keys.forEach(key => { config.properties[key].title = config.title; });
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: We don't need to make it so complicated by first getting all the keys.

Suggested change
const keys = Object.keys(config.properties);
keys.forEach(key => { config.properties[key].title = config.title; });
Object.values(config.properties).forEach(property => property.title = config.title);

Copy link
Author

Choose a reason for hiding this comment

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

Good to me. I'll integrate this suggestion.

@VGRSTM
Copy link
Author

VGRSTM commented Sep 26, 2023

Alright, behavior looks mostly good, I just have a few remarks on the code and one on the behavior: The TypeScript extension provides preferences starting with typescript.* and javascript.*. Since both use the same title, but with a different group ID, these get listed twice as separate groups in the preferences:

Ok got it. Good point. I'll work a bit more on tree generator to get proper "merge" of such nodes sticking VSCode rendering.
Thanks for this first review !

@msujew msujew added preferences issues related to preferences vscode issues related to VSCode compatibility labels Sep 28, 2023
@JonasHelming
Copy link
Contributor

@VGRSTM Do you plan to update this PR?

@VGRSTM
Copy link
Author

VGRSTM commented Dec 19, 2023

@VGRSTM Do you plan to update this PR?

Have got no time to allocate to up to now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preferences issues related to preferences vscode issues related to VSCode compatibility
Projects
Status: Waiting on author
Development

Successfully merging this pull request may close these issues.

Preference tree widget is failing to name extensions' nodes the same than VSCode
3 participants