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

Multi Diff Editor menus: Submenu icon not rendered #203792

Closed
alexandruavadanii opened this issue Jan 30, 2024 · 4 comments · Fixed by #204148
Closed

Multi Diff Editor menus: Submenu icon not rendered #203792

alexandruavadanii opened this issue Jan 30, 2024 · 4 comments · Fixed by #204148
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug diff-editor Diff editor issues insiders-released Patch has been released in VS Code Insiders verified Verification succeeded
Milestone

Comments

@alexandruavadanii
Copy link

Type: Bug

An extension that contributes a submenu with a valid icon to multiDiffEditor/resource/title is rendered without the icon (screenshot made on mouse hover, without hovering it's not even visible):
image

For comparison, the same submenu icon shows fine as a contribution to view/title:
image

To reproduce this, I created an empty extension with a simple set of menu contributions.

[1] https://github.com/alexandruavadanii/vscode-submenu-icon
[2] alexandruavadanii/vscode-submenu-icon@098d2bb

VS Code version: Code - Insiders 1.86.0-insider (9c5eabb, 2024-01-30T03:56:13.017Z)
OS version: Windows_NT x64 10.0.18362
Modes:
Remote OS version: Linux x64 4.18.0-372.19.1.el8_6.x86_64

System Info
Item Value
CPUs Intel(R) Core(TM) i7-8665U CPU @ 1.90GHz (8 x 2112)
GPU Status 2d_canvas: enabled
canvas_oop_rasterization: enabled_on
direct_rendering_display_compositor: disabled_off_ok
gpu_compositing: enabled
multiple_raster_threads: enabled_on
opengl: enabled_on
rasterization: enabled
raw_draw: disabled_off_ok
skia_graphite: disabled_off
video_decode: enabled
video_encode: enabled
vulkan: disabled_off
webgl: enabled
webgl2: enabled
webgpu: enabled
Load (avg) undefined
Memory (System) 15.81GB (6.04GB free)
Process Argv
Screen Reader no
VM 0%
Item Value
Remote SSH: sestolabccv8
OS Linux x64 4.18.0-372.19.1.el8_6.x86_64
CPUs Intel(R) Xeon(R) Silver 4216 CPU @ 2.10GHz (8 x 2095)
Memory (System) 31.21GB (16.98GB free)
VM 100%
Item Value
Remote SSH: sestolabccv8
OS Linux x64 4.18.0-372.19.1.el8_6.x86_64
CPUs Intel(R) Xeon(R) Silver 4216 CPU @ 2.10GHz (8 x 2095)
Memory (System) 31.21GB (16.98GB free)
VM 100%
Item Value
Remote SSH: sestolabccv8
OS Linux x64 4.18.0-372.19.1.el8_6.x86_64
CPUs Intel(R) Xeon(R) Silver 4216 CPU @ 2.10GHz (8 x 2095)
Memory (System) 31.21GB (16.98GB free)
VM 100%
@hediet
Copy link
Member

hediet commented Jan 31, 2024

Can you look into why this happens?

@hediet hediet added bug Issue identified by VS Code Team member as probable bug diff-editor Diff editor issues labels Jan 31, 2024
@hediet hediet added this to the Backlog milestone Jan 31, 2024
@alexandruavadanii
Copy link
Author

I will give it a try.

@alexandruavadanii
Copy link
Author

Since I'm not familiar with either TypeScript or the vscode structure, I don't have a fix, only some random findings.

Inspecting the 2 HTML elements (the view/title submenu which does have an icon and the multiDiffEditor/resource/title which does not):

  • the working one has an a element with class="action-label codicon-squirrel codicon";
  • the broken one has an a element with class="action-label submenu codicon" (note the submenu instead of codicon-...);

For both of these, we eventually construct a DropdownMenuActionViewItem, but the call trace for getting there is a bit different:

  1. view/title rendering (working fine)
  • WorkbenchToolBar in created here with an actionViewItemProvider that calls getActionViewItem;
  • getActionViewItem calls createActionViewItem here
  • createActionViewItem creates an instance of SubmenuEntryActionViewItem here
  • classNames is set to the expected value based on the item.icon object before calling the super (DropdownMenuActionViewItem) constructor:
    classNames: options?.classNames ?? (ThemeIcon.isThemeIcon(action.item.icon) ? ThemeIcon.asClassName(action.item.icon) : undefined),
  1. multiDiffEditor/resource/title (not working fine)
  • MenuWorkbenchToolBar is created here
  • MenuWorkbenchToolBar constructor calls the constructor of the super (WorkbenchToolBar) here
  • WorkbenchToolBar constructor calls the constructor of the super (ToolBar) here
  • the ToolBar constructor can set a classname for the "More" codicon here, but doesn't know what to do for our submenu:
    classNames: action.class,
  • so we end up with an untouched class name ("submenu") that was set at the beginning:
    super(`submenuitem.${item.submenu.id}`, typeof item.title === 'string' ? item.title : item.title.value, actions, 'submenu');

The difference between the two is that the first one creates an instance of SubmenuEntryActionViewItem (which extends DropdownMenuActionViewItem), while the second creates directly a DropdownMenuActionViewItem.

Now, given the separation between base and platform, I'm not sure whether we should just handle the classes for the submenu along the calls in platform or not. But once we get to the ToolBar class in base, we no longer hold a reference to the item that contains the item.icon we are interested in.

What does work and is probably not how we should handle this is setting the class name from the start in

super(`submenuitem.${item.submenu.id}`, typeof item.title === 'string' ? item.title : item.title.value, actions, 'submenu');

-super(`submenuitem.${item.submenu.id}`, typeof item.title === 'string' ? item.title : item.title.value, actions, 'submenu');
+super(`submenuitem.${item.submenu.id}`, typeof item.title === 'string' ? item.title : item.title.value, actions, ThemeIcon.isThemeIcon(item?.icon) ? ThemeIcon.asClassName(item.icon) : 'submenu');

Like I said, I'm a bit out of my depth here, I'm not familiar with the design and how this should be handled, I'm just writing this down in case it helps.

@hediet
Copy link
Member

hediet commented Feb 2, 2024

Thank you, that is a very detailed analysis! Will look into it

@hediet hediet modified the milestones: Backlog, February 2024 Feb 2, 2024
hediet added a commit that referenced this issue Feb 2, 2024
@hediet hediet mentioned this issue Feb 2, 2024
hediet added a commit that referenced this issue Feb 2, 2024
@VSCodeTriageBot VSCodeTriageBot added unreleased Patch has not yet been released in VS Code Insiders insiders-released Patch has been released in VS Code Insiders and removed unreleased Patch has not yet been released in VS Code Insiders labels Feb 2, 2024
@TylerLeonhardt TylerLeonhardt added the verified Verification succeeded label Feb 22, 2024
@microsoft microsoft locked and limited conversation to collaborators Jun 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug diff-editor Diff editor issues insiders-released Patch has been released in VS Code Insiders verified Verification succeeded
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants