Replace icons with codicons

[msujew]

What it does

Closes #6115

Does a lot of css work:

• Replaces all internally used icons (fontawesome, custom svg files) with codicons
• Adds a new utility method for codicons, namely codicon and codiconArray.
• Aligns button-icon css with vscode: Buttons in toolbars, etc. are now highlighted with a backdrop

I decided to leave the old svg icons in the codebase for backwards compatibility. I also couldn't notice any breaking change, but please keep an eye out for them.

How to test

Before starting to test I would recommend to start a second Theia instance to compare visual changes. The following testing steps are only the most important parts that have changed. As every icon in the app has changed, please refer to the code to gather a list of everything that has changed. Also, change the icon theme to seti at appropriate times to check for tab-bar items, etc.

• Check toolbar icons, every icon should show a backdrop on hover.
• Check tabbar close icons, every close icon should show a backdrop on hover.
• Check status bar icons
• Check notification widget icons
• Check debug view widget icons
• Check scm view widget icons
• Check problems view widget icons
• Check extensions view widget icons

Review checklist

• as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• as a reviewer, I agree to behave in accordance with the review guidelines

[vince-fugnitto]

@msujew I believe we should also update the widget icons themselves (ex: explorer, scm, ...), any reason we wanted to omit it initially?

[msujew]

any reason we wanted to omit it initially?

Actually no, I just stopped thinking about it, once I noticed that the explorer icon is not in the codicon collection. But I just saw that every other icon is in there. So I'll replace those. What is your opinion on replacing the default folder/file icon in the explorer? Currently it still uses the fontawesome icons.

[vince-fugnitto]

Actually no, I just stopped thinking about it, once I noticed that the explorer icon is not in the codicon collection. But I just saw that every other icon is in there. So I'll replace those. What is your opinion on replacing the default folder/file icon in the explorer? Currently it still uses the fontawesome icons.

I think the idea for consistency with vscode and what plugins use we should aim to update all icons (including font-awesome icons) to their codicon counterparts. This includes widgets when docked to sidepanels. It will create a uniform look and feel across the framework.

[vince-fugnitto]

This reminds me of the following issue #9768, perhaps it's something we can easily implement in the future.

[vince-fugnitto]

If we previously had layout data saved (ex: local-storage), then starting the app may cause icons to be non-existent:

We may want to introduce a layout-migration:

theia/packages/core/src/browser/shell/application-shell.ts

Lines 51 to 62 in 1dba014

	export type ApplicationShellLayoutVersion =
	/** layout versioning is introduced, unversioned layout are not compatible */
	2.0 |
	/** view containers are introduced, backward compatible to 2.0 */
	3.0 |
	/** git history view is replaced by a more generic scm history view, backward compatible to 3.0 */
	4.0;
	/**
	* When a version is increased, make sure to introduce a migration (ApplicationShellLayoutMigration) to this version.
	*/
	export const applicationShellLayoutVersion: ApplicationShellLayoutVersion = 4.0;

theia/packages/core/src/browser/shell/shell-layout-restorer.ts

Lines 87 to 110 in 1dba014

	export const ApplicationShellLayoutMigration = Symbol('ApplicationShellLayoutMigration');
	export interface ApplicationShellLayoutMigration {
	/**
	* A target migration version.
	*/
	readonly layoutVersion: ApplicationShellLayoutVersion;
	/**
	* A migration can transform layout before it will be inflated.
	*
	* @throws `ApplicationShellLayoutMigrationError` if a layout cannot be migrated,
	* in this case the default layout will be initialized.
	*/
	onWillInflateLayout?(context: ApplicationShellLayoutMigrationContext): MaybePromise<void>;
	/**
	* A migration can transform the given description before it will be inflated.
	*
	* @returns a migrated widget description, or `undefined`
	* @throws `ApplicationShellLayoutMigrationError` if a widget description cannot be migrated,
	* in this case the default layout will be initialized.
	*/
	onWillInflateWidget?(desc: WidgetDescription, context: ApplicationShellLayoutMigrationContext): MaybePromise<WidgetDescription | undefined>;
	}

[msujew]

We may want to introduce a layout-migration

Alright, done. It's a bit crude, but I found the string replacement method to be sufficient.

[vince-fugnitto]

@msujew do you mind rebasing the pull-request when you get a chance? I'd be happy to take a look at it afterwards :)

[vince-fugnitto]

I really love the new icons and behavior for toolbar items 👍
I noticed two things when testing:

• I believe the bottom sidebar (settings icon) may need to be adjusted to have the same font-size as the rest of the sidebar
• I believe the search-in-workspace widget needs to be updated to take into account the new chevron (left is the pr, and right is master), the input fields are no longer aligned:

• dirty editors are no longer marked as dirty:

[msujew]

@vince-fugnitto Thanks for the feedback, I think I addressed everything 👍

[vince-fugnitto]

@msujew do you mind rebasing when you get a chance? I finally have a bit of time to help with the review :)

[vince-fugnitto]

@msujew I would be fine to merge the changes early in the month and iteratively fix issues if we find them.
One such issue I noticed is the alignment of some icons and the close button as compared to master (below):

We would also probably want more padding (like master) when docking widgets out of the sidepanels:

master:

[msujew]

@vince-fugnitto Sure sounds good to me. I'm still willing to fix any issues that we find before we merge :)

[vince-fugnitto]

@vince-fugnitto Sure sounds good to me. I'm still willing to fix any issues that we find before we merge :)

You can fix the last two I found, I wasn’t able to see any other regressions when testing 👍

[vince-fugnitto]

I'd be fine to merge and fix issues throughout the month if necessary as I do not want to hold back the pull-request 👍

I still notice some UI issues, perhaps one of the bigger ones (that may be caused by a bigger issue) is that the plugin-views sometimes do not display their proper icons:

I also noticed that the alignment of toolbar items contributed by plugins are not necessarily aligned properly as well (compared to builtin views like the explorer):

A very minor bug is how the scm arrow is not aligned properly (for CHANGES) compared to other trees:

[msujew]

I still notice some UI issues, perhaps one of the bigger ones (that may be caused by a bigger issue) is that the plugin-views sometimes do not display their proper icons

This seems to be unrelated to my changes, see the current master:

I noticed that the instanceof check in the plugin host fails:

theia/packages/plugin-ext/src/plugin/tree/tree-views.ts

Line 372 in 7442150

} else if (iconPath instanceof ThemeIcon) {

I assume this is due to the ES2017 upgrade and the @es5Compatibility annotation (#9436). instanceof checks are not a valid operation on es5 classes as far as I can tell.

I'll add an is check for ThemeIcon users instead.

[vince-fugnitto]

@msujew sorry I had thought I confirmed against master that it was a regression (turns out the code I confirmed against was older) but you're right that its not related to your changes and we can omit the fix for now 👍

[vince-fugnitto]

The changes look good to me 👍

I think there is a leftover file:

[msujew]

I'll merge this Tuesday or Wednesday, if no one else has any issues with my changes.