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

[themes] Add ability to reload themes without restarting the editor #66115

Merged
merged 4 commits into from Jan 21, 2019

Conversation

Projects
None yet
2 participants
@JimiC
Copy link
Contributor

JimiC commented Jan 6, 2019

Resolves #45963

This PRs adds the ability to reload a theme (color or icon) without the need to restart the editor.

Replaces #60136

@JimiC JimiC force-pushed the JimiC:reload_theme branch from 734432d to e0ca716 Jan 6, 2019

@egamma egamma requested a review from aeschli Jan 6, 2019

@aeschli

This comment has been minimized.

Copy link
Contributor

aeschli commented Jan 7, 2019

made some changes.

@aeschli aeschli added this to the December/January 2019 milestone Jan 7, 2019

@JimiC

This comment has been minimized.

Copy link
Contributor Author

JimiC commented Jan 7, 2019

Fine by me as long as it brings us one step closer to merging. 😄

@JimiC

This comment has been minimized.

Copy link
Contributor Author

JimiC commented Jan 7, 2019

Looks like createUnloadedTheme requires undefined for settingsId and extensionData.

@aeschli aeschli changed the title Add ability to reload themes without restarting the editor [themes] Add ability to reload themes without restarting the editor Jan 21, 2019

@aeschli aeschli merged commit c596383 into Microsoft:master Jan 21, 2019

1 check passed

license/cla All CLA requirements met.
Details
@aeschli

This comment has been minimized.

Copy link
Contributor

aeschli commented Jan 21, 2019

We had long discussions in the team about this feature. By design, the content of extensions are expected to be immutable. Only our extension management code should make changes.
Adding a watcher is the wrong approach. Instead we should add a icon/color theme provider API in code that would allow you to implement a more powerful icon theme.
But that API isn't going to happen very soon, so I added a hidden/ & in-official flag for you where you can turn on a watcher for your theme.

		"iconThemes": [
			{
				"id": "vs-minimal",
				"label": "Minimal (Visual Studio Code)",
				"path": "./fileicons/vs_minimal-icon-theme.json",
				"_watch": true
			}
		]

No warranties on that flag and it might also be removed any time if necessary. But I will let you know once we have the proper API in place.

Thanks for your help!

aeschli added a commit that referenced this pull request Jan 21, 2019

@JimiC

This comment has been minimized.

Copy link
Contributor Author

JimiC commented Jan 21, 2019

So that's what _watch is all about.
Sorry if I'm wrong but I only see this being declared for color themes and not icon themes (ColorThemeData).
Discard the above, I just saw 24e8857

Just for the record, I proposed an API but it was rejected for not fitting to the current API (#60136 (comment)). I'm totally in favor of an API point and that was my original approach. But in order to see the reloading feature happening, I went with whatever was suggested.

I don't want to sound bitter but a temp solution is like no solution at all. Never the less, I'll try to see the cup half-full and not half-empty. Some will say that from not having this at all, at least we can have it as it is.

@JimiC JimiC deleted the JimiC:reload_theme branch Jan 21, 2019

@aeschli

This comment has been minimized.

Copy link
Contributor

aeschli commented Jan 22, 2019

Yes, sorry, my fault that we went down that pass. But also a reload API doesn't make any sense given that extensions are not supposed to make changes to their content. You guys also only do that to work around limitations on the icon theme syntax.
What you want is a file icon theme provider that lets you return an icon for a given file path. However, that's quite a lot of changes and probably requires us to refactor the way icon associations are made.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment