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

Support using theme icons in webview iconPath #92122

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mjbvz
Copy link
Collaborator

@mjbvz mjbvz commented Mar 5, 2020

Fixes #90616

Enables using theme icons in the webview.iconPath

Screen Shot 2020-03-05 at 3 12 03 PM

This fixes the problem in a very ugly way by inlining all the codicon css classes. We should look into a proper fix

Fixes #90616

Enables using theme icons in the webview.iconPath

This fixes the problem in a very ugly way by inlining all the codicon css classes. We should look into a proper fix
@mjbvz mjbvz requested review from bpasero and miguelsolorio March 5, 2020 23:16
@mjbvz mjbvz self-assigned this Mar 5, 2020
@miguelsolorio
Copy link
Contributor

Is there no way to have the icons use the codicon classes then? Because we have certain icons that use certain colors/styles and will be overwritten by the file icon theme and looks odd:

image

@bpasero
Copy link
Member

bpasero commented Mar 6, 2020

@mjbvz what do you want me to review here?

@mjbvz
Copy link
Collaborator Author

mjbvz commented Mar 6, 2020

@bpasero I updated the change to enable codicons in tab labels:

Screen Shot 2020-03-06 at 2 43 18 PM

This works but I feel we should probably push the ThemeIcons logic into the ResourceLabel class. What do you think? Any thoughts on what that change would involve?

Also @misolori, once we figure out what we want to do, I'll likely need you help tweaking some of the css.

try {
const webviewSelector = `.show-file-icons .webview-${key}-name-file-icon`;
if (ThemeIcon.isThemeIcon(value)) {
// Theme icons are handled by EditorInput so hide the standard file icons.
cssRules.push(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It would be much nicer of ResourceLabel could handle this automatically

@@ -963,8 +966,9 @@ export class TabsTitleControl extends TitleControl {
tabContainer.title = title;

// Label
const displayName = editor.iconPath ? `$(${editor.iconPath.id}) ${escapeCodicons(name)}` : escapeCodicons(name);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This logic is ugly and has to duplicated. We should look into pushing the icon logic into ResourceLabel

@bpasero
Copy link
Member

bpasero commented Mar 10, 2020

@mjbvz yes, having this in resource labels seems better to me as probably most of these areas use it under the hood.

I do not fully understand what this PR is trying to solve:

  • having an explicit optional icon property on editors makes sense to me and then should probably be wired in properly in those places where we show an editor
  • allowing codicons as part of label or description does NOT make sense to me

I fear that escaping codicons in places like update-tabs could be a perf hit because this method may be called a ton of times. We should only change logic if the optional icon property is set on the editor input imho and should not require any escaping. The icon should just appear as part of the resource label before the text.

@miguelsolorio
Copy link
Contributor

cc @aeschli if you're able to help out with this

@miguelsolorio miguelsolorio requested a review from aeschli March 20, 2020 16:24
@mjbvz
Copy link
Collaborator Author

mjbvz commented Mar 20, 2020

@isidorn do you own ResourceLabel? If so, can we add this work to the April plan

@isidorn
Copy link
Contributor

isidorn commented Mar 23, 2020

@mjbvz @bpasero , @jrieken and me did work in the ResourceLabel. Nobody really owns it as far as I know, so feel free to jump on it.

@jrieken
Copy link
Member

jrieken commented Mar 23, 2020

@mjbvz For a start, there is ThemIcon.asClassName which is doing some of the CSS work for you. That's what I have used for the toolbar

@jrieken
Copy link
Member

jrieken commented Mar 23, 2020

Generally, and swe-wise, it seems that we should have some kind of Icon-type which encapsulated all those different sources (theme icon, file icon, file icon pair for light and dark theme). A similar struggle exists with customs tree and so

@mjbvz
Copy link
Collaborator Author

mjbvz commented Mar 23, 2020

I've struggled to figure out how file icons work in the workbench, especially the CSS part. Can someone please help with the workbench side of things for April?

@bpasero
Copy link
Member

bpasero commented Mar 24, 2020

@mjbvz for file icons specifically, I would connect with @aeschli. For general label widget issues, I am mainly the author of that.

@aeschli
Copy link
Contributor

aeschli commented Mar 24, 2020

We suggest we distinguish between file icons and 'product' icons.

  • ResourceLabel is for file icons (and labels). The input is a resource URI and the ResourceLabel knows how how to show the associated file icon
  • we need a similar helper class for product icons. Input is a product icon name (aka ThemeIcon) or a icon path (or light/dark paths).
    I'm working on the icon registry for the product icons along with a icon contribution point and theming support for all product icons. [themes] Add product icon themes #92791

@mjbvz If you want to extract what you have to a IconWidget, that would be a great starting point. Otherwise I can do this once I find time. @bpasero Please let me know if we already have something like that.

@bpasero
Copy link
Member

bpasero commented Mar 24, 2020

@aeschli yes we do, it is called IconLabel.

We might want to reuse parts of ResourceLabels in case we expect many product icons (e.g. in trees) that are updated frequently. The idea was to remove listeners from the single resource label instance and move it to its parent.

@kieferrm kieferrm added this to the May 2020 milestone May 10, 2020
@bpasero bpasero removed their request for review May 22, 2020 17:10
@mjbvz mjbvz modified the milestones: May 2020, June 2020 Jun 16, 2020
@kieferrm kieferrm modified the milestones: June 2020, July 2020 Jul 6, 2020
@mjbvz mjbvz modified the milestones: July 2020, August 2020 Jul 28, 2020
@mjbvz mjbvz modified the milestones: August 2020, On Deck Sep 8, 2020
@joaomoreno joaomoreno changed the base branch from master to main February 15, 2021 08:51
@ghost
Copy link

ghost commented Sep 27, 2021

What's the progress on this? Is there a reason for help-wanted on the ticket?

@vivodi
Copy link

vivodi commented Dec 29, 2024

Is this still relevant?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow ThemeIcon in WebviewPanel.iconPath
8 participants