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

Support VS Code Default Language Icons #13014

Merged
merged 2 commits into from Nov 8, 2023
Merged

Support VS Code Default Language Icons #13014

merged 2 commits into from Nov 8, 2023

Conversation

martin-fleck-at
Copy link
Contributor

@martin-fleck-at martin-fleck-at commented Oct 20, 2023

What it does

Add support for default language icons similar to VS Code:

  • Extend language service with necessary methods
  • Create CSS rules for contributed language icons
  • Use language icons if theme sets 'showLanguageModeIcons'
  • Ensure custom theme can override language icons

Fixes #13013

How to test

  • Provide a plugin with a language contribution that adds a custom default language icon, ideally a "known" language like python
  • Install Material Icon Theme which already provides custom icons for the same language.
  • Switch file icon themes to see that everything is working as expected:
    • None: no icons shown
    • File Icons (Theia): uses our default language icon
    • Material Icon Theme: uses icon from the theme

Follow-ups

Review checklist

Reminder for reviewers

@msujew msujew self-requested a review October 23, 2023 13:21
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Provide a plugin with a language contribution that adds a custom default language icon, ideally a "known" language like python

Can you provide such a plugin? As far as I can tell, the builtin plugins don't contribute such an icon to any language.

packages/monaco/src/browser/monaco-languages.ts Outdated Show resolved Hide resolved
packages/plugin-ext/src/common/plugin-protocol.ts Outdated Show resolved Hide resolved
@martin-fleck-at
Copy link
Contributor Author

martin-fleck-at commented Oct 23, 2023

@msujew Thank you for having a look! I pushed an example to https://github.com/eclipse-theia/theia/tree/issues/13013_sample or you can download the zipped vsix:

  • Add custom light and dark icon for Python files

language-icon

@tsmaeder
Copy link
Contributor

@msujew are you reviewing this one?

- Extend language service with necessary methods
- Create CSS rules for contributed language icons
- Use language icons if theme sets 'showLanguageModeIcons'
- Ensure custom theme can override language icons
@martin-fleck-at
Copy link
Contributor Author

I rebased the branch onto main as there was a conflict in scanner-theia.ts.

@msujew msujew added the theming issues related to theming label Nov 6, 2023
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Thanks for the example plugin. This change works well for me 👍

I've encountered a very minor issue during testing. After installing the extension (but before changing the color/icon theme), any open editor keeps the old icon (while it correctly updates in the navigator). After changing to another theme, everything is updated as expected. This shouldn't stop this change from getting merged though.

@martin-fleck-at
Copy link
Contributor Author

@msujew Very good catch! I added a commit that should fix that problem. It was just adding an event so I thought it would be faster than to create a follow up issue ;-)

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for the update @martin-fleck-at 👍

The new changes look good as well. Now without any further critique :)

@martin-fleck-at martin-fleck-at merged commit 882e125 into master Nov 8, 2023
13 checks passed
@martin-fleck-at martin-fleck-at deleted the issues/13013 branch November 8, 2023 15:40
@github-actions github-actions bot added this to the 1.44.0 milestone Nov 8, 2023
Comment on lines +128 to +129
width: ${size}'px';
height: ${size}'px';
Copy link
Contributor

@bvenreply bvenreply Nov 25, 2023

Choose a reason for hiding this comment

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

Hello @martin-fleck-at, what is the intent behind the addition of these single quotes? I don't think this is valid syntax, these properties are just being discarded from the css stylesheet at the moment.

I'm taking a look in the context of #13096 btw.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @bvenreply, you are definitely right that they are not producing valid CSS syntax. My best guess is that I did it by mistake and for some reason just didn't notice. I don't see any reason why it would be like that as it is also not like that for any other pixel value. Thank you for discovering this! If you are already working on an issue I'd be grateful if you could take care of this but I can also create a separate change if necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you @martin-fleck-at, I believe I'll be able to take care of this myself in the course of #13096. If not I'll let you know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theming issues related to theming
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Support VS Code Default Language Icons
4 participants