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

material-icon-theme: change folder color command does not work when the extension is run as a workspace extension #2343

Closed
egamma opened this issue Feb 12, 2020 · 7 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug extension-issue remote Issues in the code server support upstream Issue identified as 'upstream' component related (exists outside of VS Code Remote)

Comments

@egamma
Copy link
Member

egamma commented Feb 12, 2020

Background: the popular theme PKief.material-icon-theme does not work in VSO. One reason is that the extension is configured to be a strict ui extension "extensionKind": ["ui"].

When configuring the extension so that it can also run as a workspace extension: "extensionKind": ["ui", "workspace"] then the material icon theme is properly applied and rendered. However, the dynamic theme customizations (change folder, change opacity) have no effect. These dynamic theme customization modify the icon definition file and the svgs for the extension. Reloading the window has no effect and these customizations are not honored.

To reproduce the problem:

  • configure the material icon theme as a workspace extensions in the user setting:
	"remote.extensionKind": {
		"pkief.material-icon-theme": "workspace"
	}
  • open a dev container (e.g. https://github.com/microsoft/vscode-remote-try-node)
  • install the material theme
  • reload the window and verify that the extension is run as a workspace extension using 'show running extensions'
  • run the extension command 'Material Icons: Change Folder Color'. This command changes the color of the default folder.
  • the extension detects the changes to setting and updates the icon file in the extension in dist/material-icons.json and regenerates the corresponding folder SVG icons in the icons folder (e.g. folder.svg). It then informs the user to reload the window.

🐛 the change has no effect and the folder color falls back to the default color.

@egamma egamma added bug Issue identified by VS Code Team member as probable bug remote Issues in the code server support extension-issue labels Feb 12, 2020
@aeschli
Copy link
Contributor

aeschli commented Feb 17, 2020

The extension modifies the icon definition theme file but also the svg-icons it provides (it changes the fill color).
The problem is that for remote extension resources we set the cache header public, max-age=31536000 as we assume that extension resources only change when an extension updates.
The caching helps with for a good window reload experience.

Some ideas:

  • ask the author to use a dynamic icon name e.g. encode the background color in the file name
  • add a new flag to package.json no_cache or allow to add a query component or a path segment to signal that the resource is 'non-static'
  • have a proper icons provider API

@aeschli
Copy link
Contributor

aeschli commented Feb 17, 2020

@PKief Would you consider using a different file name per generated folder icon? E.g. folder-00ff00.svg. That way icons can still be cached when the extension is set to remote.

@aeschli aeschli added this to the February 2020 milestone Feb 17, 2020
@PKief
Copy link

PKief commented Feb 17, 2020

@aeschli I was just elaborating if this would be technically possible. So right now, I do not see any hurdles except the fact that I need to touch various lines of code in my extension. I could definitely try that if it saves you some time ;)

@aeschli
Copy link
Contributor

aeschli commented Feb 18, 2020

Sorry for the work on your side. I think it's the most pragmatic change. Officially, resources in extensions are supposed to be static so what your extension does (others do it as well, I know) is not supported . I understand why you do it and there's currently no alternative for you. The proper solution would be a new 'Icon Provider` API, but that's not very high on our backlog.

When you make the suggested change, as an additional benefit I believe the 'reload window' is no longer necessary.

@PKief
Copy link

PKief commented Feb 18, 2020

No worries, I can understand that. I put this task on my agenda to work on that soon. And if the 'reload window' is no longer necessary with this approach, it will improve the experience of my extension as well.

@PKief
Copy link

PKief commented Feb 25, 2020

@egamma It is now working with this PR PKief/vscode-material-icon-theme#649 in a remote container:

demo

It is also working for the settings 'material-icon-theme.opacity' and 'material-icon-theme.saturation'. The 'reload.window' is not needed anymore, which is really nice!

I'll merge it to my master branch and push an updated version of the extension to the marketplace in the next days. @aeschli Is there anything else that should be done from my side?

@aeschli
Copy link
Contributor

aeschli commented Feb 26, 2020

@PKief Thaks a lot.
You can now also update your package.json to:

"extensionKind": ["ui", "workspace"]

That way your extension will run properly in remote windows as well as in the web without the need for us to override extensionKind.

@aeschli aeschli closed this as completed Feb 26, 2020
@aeschli aeschli added the upstream Issue identified as 'upstream' component related (exists outside of VS Code Remote) label Feb 28, 2020
@aeschli aeschli removed this from the February 2020 milestone Feb 28, 2020
@vscodebot vscodebot bot locked and limited conversation to collaborators Apr 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug extension-issue remote Issues in the code server support upstream Issue identified as 'upstream' component related (exists outside of VS Code Remote)
Projects
None yet
Development

No branches or pull requests

3 participants