plugin: ThemeIcon Support in SCM #12187

FernandoAscencio · 2023-02-15T15:58:04Z

What it does

Closes: #11829
Finishes Estelle's changes to support Themables from here.
Allows SourceControlResourceThemableDecorations.iconPath to accept ThemeIcon and modifies the SCM supporting architecture to accept the change.

How to test

Delete "@theia/git": "1.38.0" from the examples\browser\package.json and examples\electron\package.json files.
Run Theia without plugins.
Install the modified git-base and git files from here
Reload if necessary.
The SCM should display at least two files that have been modified: the package.json files.
Check the icons display to the right.
Change the Theme and verify the icon changes when changing from dark to light themes or vice-versa.

This package currently only tests for 'Light' and "Dark" theme changes as well as URI and ThemeIcon inputs.

Possible tests:

Check for different icons for different Status (Untracked, Modified, Staged) if possible.

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

packages/plugin-ext/src/common/plugin-api-rpc.ts

packages/plugin-ext/src/main/browser/scm-main.ts

packages/plugin-ext/src/plugin/plugin-icon-path.ts

packages/plugin-ext/src/plugin/scm.ts

tsmaeder · 2023-05-08T09:24:18Z

packages/scm/src/browser/scm-provider.ts

@@ -58,6 +58,8 @@ export interface ScmResource {
 }

 export interface ScmResourceDecorations {
+    icon?: string;


Not sure whether it would be a better idea to keep the conditional structure (either a ThemeIcon or a { dark: ..., light...} structure until we render the whole thing in the UI? It kinda doesn't make sense to have a dark and light ThemeIcon, as mentioned elsewhere, IMO.

tsmaeder · 2023-05-08T09:30:24Z

Modify the git extension files to pass a ThemeIcon (URI or codicon) as a decoration

@FernandoAscencio what do you mean by that?

FernandoAscencio · 2023-05-09T14:56:29Z

Modify the git extension files to pass a ThemeIcon (URI or codicon) as a decoration

@FernandoAscencio what do you mean by that?

It has been a while so I have no idea. I'll change the steps so they make more sense.

tsmaeder · 2023-05-11T09:14:29Z

@FernandoAscencio having done a bit more thinking about the general approach of this PR. In plugin-icon-path.ts, you import from VS Code in order to be able to resolve ThemeIcons in the plugin host process. Using stuff from VS Code via import in the plugin host or the back-end in general is unusual. I think the right way to to this is to transfer the ThemeIcon as a typed object to the front end and to do the conversion to CSS there. As we do, for example, for tree items in tree-views.ts.

FernandoAscencio · 2023-05-15T17:03:35Z

@tsmaeder
I was not sure on that part, thank you for the guidance.
I'll make changes based on tree-views.ts

tsmaeder

That looks much nicer!

packages/plugin-ext/src/plugin/plugin-icon-path.ts

packages/plugin-ext/src/plugin/scm.ts

tsmaeder · 2023-05-30T08:56:58Z

packages/plugin-ext/src/plugin/scm.ts

@@ -511,6 +513,15 @@ class SsmResourceGroupImpl implements theia.SourceControlResourceGroup {
        return rawResourceSplices;
    }

+    private getThemableIcon(icon: UriComponents | ThemeIcon | undefined): IconUrl | ThemeIcon | undefined {


I think string | ThemaIcon | undefined would be a more precise return type.

packages/scm/src/browser/scm-tree-widget.tsx

tsmaeder · 2023-05-30T09:25:55Z

@FernandoAscencio about the "how to test": what are the changes you did to the git and git-base extensions? We already could have dark and light icons before, how does the testing procedure test the use of ThemeIcon?

fixes small typo `SsmResourceGroupImpl` -> `ScmResourceGroupImpl` Signed-Off-By: FernandoAscencio <fernando.ascencio.cama@ericsson.com>

Finishes Estelle's changes to support Themables. Signed-off-by: FernandoAscencio <fernando.ascencio.cama@ericsson.com> Co-Authored-By: Estelle

FernandoAscencio · 2023-05-31T02:41:58Z

@tsmaeder
Yes, dark and light icons were available before. This test makes it possible to test for the added ThemeIcon which is displayed as a globe codicon. URI is displayed as the blue M icon. These do not display in the current master when using the vsix files.

tsmaeder

Looks good to me now.

tsmaeder · 2023-05-31T09:05:43Z

@FernandoAscencio unless you have more changes coming, I'll merge this one.

FernandoAscencio · 2023-05-31T16:53:30Z

@tsmaeder
I think that is it for this PR.

FernandoAscencio force-pushed the fa/ThemeIconSupport branch 2 times, most recently from 8371377 to d7f2df0 Compare March 24, 2023 13:04

FernandoAscencio force-pushed the fa/ThemeIconSupport branch from d7f2df0 to 06da84a Compare March 29, 2023 17:11

vince-fugnitto modified the milestone: 1.37.0 Apr 27, 2023

tsmaeder reviewed May 8, 2023

View reviewed changes

FernandoAscencio force-pushed the fa/ThemeIconSupport branch from c582211 to 002f719 Compare May 9, 2023 14:41

FernandoAscencio force-pushed the fa/ThemeIconSupport branch from 002f719 to 70db92d Compare May 9, 2023 16:10

FernandoAscencio force-pushed the fa/ThemeIconSupport branch from 70db92d to e2ae178 Compare May 17, 2023 18:02

FernandoAscencio force-pushed the fa/ThemeIconSupport branch from 4e7161c to 9780fe5 Compare May 24, 2023 22:37

FernandoAscencio requested a review from tsmaeder May 26, 2023 22:38

tsmaeder reviewed May 30, 2023

View reviewed changes

FernandoAscencio added 2 commits May 30, 2023 22:31

typo: scm.ts typo

e5fa9d8

fixes small typo `SsmResourceGroupImpl` -> `ScmResourceGroupImpl` Signed-Off-By: FernandoAscencio <fernando.ascencio.cama@ericsson.com>

plugin: ThemeIcon Support in SCM

8836ce4

Finishes Estelle's changes to support Themables. Signed-off-by: FernandoAscencio <fernando.ascencio.cama@ericsson.com> Co-Authored-By: Estelle

FernandoAscencio force-pushed the fa/ThemeIconSupport branch from 9780fe5 to 8836ce4 Compare May 31, 2023 02:41

tsmaeder approved these changes May 31, 2023

View reviewed changes

tsmaeder merged commit b59581b into eclipse-theia:master Jun 1, 2023

vince-fugnitto added this to the 1.39.0 milestone Jun 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

plugin: ThemeIcon Support in SCM #12187

plugin: ThemeIcon Support in SCM #12187

FernandoAscencio commented Feb 15, 2023 •

edited

Loading

tsmaeder May 8, 2023

tsmaeder commented May 8, 2023

FernandoAscencio commented May 9, 2023

tsmaeder commented May 11, 2023

FernandoAscencio commented May 15, 2023

tsmaeder left a comment

tsmaeder May 30, 2023

FernandoAscencio May 31, 2023

tsmaeder commented May 30, 2023

FernandoAscencio commented May 31, 2023 •

edited

Loading

tsmaeder left a comment

tsmaeder commented May 31, 2023

FernandoAscencio commented May 31, 2023

plugin: ThemeIcon Support in SCM #12187

plugin: ThemeIcon Support in SCM #12187

Conversation

FernandoAscencio commented Feb 15, 2023 • edited Loading

What it does

How to test

Review checklist

Reminder for reviewers

tsmaeder May 8, 2023

Choose a reason for hiding this comment

tsmaeder commented May 8, 2023

FernandoAscencio commented May 9, 2023

tsmaeder commented May 11, 2023

FernandoAscencio commented May 15, 2023

tsmaeder left a comment

Choose a reason for hiding this comment

tsmaeder May 30, 2023

Choose a reason for hiding this comment

FernandoAscencio May 31, 2023

Choose a reason for hiding this comment

tsmaeder commented May 30, 2023

FernandoAscencio commented May 31, 2023 • edited Loading

tsmaeder left a comment

Choose a reason for hiding this comment

tsmaeder commented May 31, 2023

FernandoAscencio commented May 31, 2023

FernandoAscencio commented Feb 15, 2023 •

edited

Loading

FernandoAscencio commented May 31, 2023 •

edited

Loading