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

Allow extensions to specify custom tree view resoure type #43261

Merged
merged 4 commits into from
Feb 26, 2018

Conversation

vbfox
Copy link
Contributor

@vbfox vbfox commented Feb 8, 2018

The FileKind was previously guessed from the collapsability of the item,
extensions now have the capability to set it manually.

Here the top level item come from vscode great icons theme for .fsproj files:
2018-02-09 22_11_07- extension development host - welcome - iotestxxx - code - oss dev

Fixes #43216

@msftclas
Copy link

msftclas commented Feb 8, 2018

CLA assistant check
All CLA requirements met.

@vbfox vbfox closed this Feb 8, 2018
@vbfox vbfox reopened this Feb 9, 2018
@vbfox
Copy link
Contributor Author

vbfox commented Feb 9, 2018

@sandy081 here is the PR I was speaking about in #43216 I think it's ready for review

@sandy081 sandy081 self-requested a review February 12, 2018 16:57
@sandy081 sandy081 added the tree-views Extension tree view issues label Feb 12, 2018
@sandy081 sandy081 self-assigned this Feb 12, 2018
@sandy081 sandy081 added this to the February 2018 milestone Feb 12, 2018
@sandy081
Copy link
Member

@vbfox Thanks for the PR. Why not using the trailing slash to determine if the resource is a directory or not?

@vbfox
Copy link
Contributor Author

vbfox commented Feb 12, 2018

I preferred to be explicit as the extension often know what type of resource it is and also to avoid breaking extensions that would have started to depend on the current behavior.

But I can change it, It simplify the code a lot.

@sandy081
Copy link
Member

I see your point, but we already have FileType in our proposed API here. Re-using that type might be right approach.

@bpasero May I know if we can reuse FileType here ?

@bpasero
Copy link
Member

bpasero commented Feb 14, 2018

@sandy081 that is @jrieken area

@sandy081
Copy link
Member

@vbfox I do not think using a new property which is file specific is good idea as the tree is generic. Another idea is, since this is related to icon, overload iconPath property to determine the resource type.

iconPath?: string | Uri | { light: string | Uri; dark: string | Uri } | 'file' | 'folder' ;

@sandy081
Copy link
Member

Another idea is to define a ThemeIcon and use it for icon.

export namespace ThemeIcon {
	const file: ThemeIcon;
	const folder: ThemeIcon;
}

export class ThemeIcon {
         readonly id: string;
}

iconPath?: string | Uri | { light: string | Uri; dark: string | Uri } | ThemeIcon;

@vbfox
Copy link
Contributor Author

vbfox commented Feb 20, 2018

I think I like the 'file' | 'folder' in iconPath solution the most, it's explicit simple and widely useful.

Especially if it also style as a file or folder without resource URI (In this case using the generic icons for file or folder)

I'll take a shoot at implementing that

@sandy081
Copy link
Member

@vbfox Thanks for considering that.

I would prefer last solution of using ThemeIcon. This will be more explicit that given resource URI, icon for the tree item should be File theme icon or Folder them icon.

The FileKind was previously guessed from the collapsability of the item,
extensions now have the capability to set it manually.

Fixes microsoft#43216
@vbfox vbfox force-pushed the file_as_folder_in_custom_tree branch from 45227f2 to 03caa41 Compare February 23, 2018 12:38
@vbfox
Copy link
Contributor Author

vbfox commented Feb 23, 2018

@sandy081 I did the changes, choose the name ThemeIconCategory instead of ThemeIcon because I felt that a ThemeIcon would designate a single icon like a ThemeColor point to a single color and it's not what's happening here. But I can change it.

The multi process / saniziting was the most foreign thing for me so I hope I did it correctly:

  • ThemeIconCategory is the type sent by the extension and is transformed to and IThemeIconCategory keeping only the id field
  • The object matching IThemeIconCategory is passed to the main process and detected in customView.ts via it's id property

@sandy081
Copy link
Member

@aeschli Would like to know your opinion on the name ThemeIconCategory?

@aeschli
Copy link
Contributor

aeschli commented Feb 23, 2018

Should be ThemeIcon, like ThemeColor.
ThemeIconCategory doesn't fit.
The idea is to give our icons an identifier ('folder-icon', 'file-icon', 'refresh-button'...), and let icon themes provide a different icon

Copy link
Member

@sandy081 sandy081 left a comment

Choose a reason for hiding this comment

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

Changes look good to me. There are some minor comments about the API documentation and little tweaks in passing theme icon. Since today is the code freeze, I will merge this PR and do the little tweaks myself. I hope you do not mind with that.

Thanks

@sandy081 sandy081 merged commit d2470e2 into microsoft:master Feb 26, 2018
@github-actions github-actions bot locked and limited conversation to collaborators Mar 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
tree-views Extension tree view issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Icons for TreeViewItems fallbacking to default folder item if file uri is used.
6 participants