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

plugin: ThemeIcon Support in SCM #12187

Merged
merged 2 commits into from
Jun 1, 2023

Conversation

FernandoAscencio
Copy link
Contributor

@FernandoAscencio FernandoAscencio commented Feb 15, 2023

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.

TheiaTest

How to test

  1. Delete "@theia/git": "1.38.0" from the examples\browser\package.json and examples\electron\package.json files.
  2. Run Theia without plugins.
  3. Install the modified git-base and git files from here
  4. Reload if necessary.
  5. The SCM should display at least two files that have been modified: the package.json files.
  6. Check the icons display to the right.
  7. 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

Reminder for reviewers

@FernandoAscencio FernandoAscencio force-pushed the fa/ThemeIconSupport branch 2 times, most recently from 8371377 to d7f2df0 Compare March 24, 2023 13:04
@vince-fugnitto vince-fugnitto modified the milestone: 1.37.0 Apr 27, 2023
packages/plugin-ext/src/common/plugin-api-rpc.ts Outdated Show resolved Hide resolved
packages/plugin-ext/src/main/browser/scm-main.ts Outdated Show resolved Hide resolved
packages/plugin-ext/src/plugin/plugin-icon-path.ts Outdated Show resolved Hide resolved
packages/plugin-ext/src/plugin/scm.ts Show resolved Hide resolved
packages/plugin-ext/src/plugin/scm.ts Outdated Show resolved Hide resolved
packages/plugin-ext/src/plugin/scm.ts Outdated Show resolved Hide resolved
packages/plugin-ext/src/plugin/scm.ts Outdated Show resolved Hide resolved
@@ -58,6 +58,8 @@ export interface ScmResource {
}

export interface ScmResourceDecorations {
icon?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

tsmaeder commented May 8, 2023

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

@FernandoAscencio what do you mean by that?

@FernandoAscencio
Copy link
Contributor Author

  1. 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
Copy link
Contributor

@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
Copy link
Contributor Author

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

Copy link
Contributor

@tsmaeder tsmaeder left a comment

Choose a reason for hiding this comment

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

That looks much nicer!

packages/plugin-ext/src/plugin/plugin-icon-path.ts Outdated Show resolved Hide resolved
packages/plugin-ext/src/plugin/scm.ts Show resolved Hide resolved
@@ -511,6 +513,15 @@ class SsmResourceGroupImpl implements theia.SourceControlResourceGroup {
return rawResourceSplices;
}

private getThemableIcon(icon: UriComponents | ThemeIcon | undefined): IconUrl | ThemeIcon | undefined {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree.

packages/scm/src/browser/scm-tree-widget.tsx Show resolved Hide resolved
@tsmaeder
Copy link
Contributor

@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 <[email protected]>
Finishes Estelle's changes to support Themables.

Signed-off-by: FernandoAscencio <[email protected]>
Co-Authored-By: Estelle
@FernandoAscencio
Copy link
Contributor Author

FernandoAscencio commented May 31, 2023

@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.

Copy link
Contributor

@tsmaeder tsmaeder left a comment

Choose a reason for hiding this comment

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

Looks good to me now.

@tsmaeder
Copy link
Contributor

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

@FernandoAscencio
Copy link
Contributor Author

@tsmaeder
I think that is it for this PR.

@tsmaeder tsmaeder merged commit b59581b into eclipse-theia:master Jun 1, 2023
@vince-fugnitto vince-fugnitto added this to the 1.39.0 milestone Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

vscode: add full support for SourceControlResourceThemableDecorations.iconPath
3 participants