Submenu contribution to editor/title and view/title not working #12706

[jfaltermeier]

What it does

The menu registered at the registry is correctly registered (Context menu > Sub Menu > Child Entries).

In this case the renderer should skip rendering the sub menu however.
The sub menu is represented by the toolbar button and its tooltip already.
So we just want to see the child entries in the context menu.

This PR introduces a helper method to adjust an existing menu-model to be rendered without a single root-submenu. Renderers may use this helper as required.

• introduce option in RenderContextMenuOptions to ask renderer to not render a menu with single submenu. Instead render only the children
• add helper method to menu-model-registry to remove the parent nodes from a menu node as described above
• adjust renderers and callers accordingly
• fix icons in toolbar

fixes #12706

How to test

See the reproducer on #12706

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

[JonasHelming]

@jonah-iden You mentioned yesterday that you miss some menus, is this maybe related?

[jonah-iden]

@JonasHelming thanks for the info, but the not notebook specific menus im missing are:
file/newFile from both the python and ipynb extension and editor/content which only the python extension is missing.
All other missing menus are notebook specific, so i have to add them myself

[JonasHelming]

#12755

[martin-fleck-at]

Code looks good to me in general and as far as I can see, all the menus are showing their content as they do in VS Code, even if the location within the context menu sometimes varies a bit. So great work, Johannes!

The only issue I could detect was with the icon. For some reason, it does not show up for me. I looked a bit into this and the image resource is properly hosted and loaded so it seems more of a CSS thing. The CSS is applied in plugin-shared-style.ts and I noticed that if I change the display property from the default inline to something else, the icon shows up. However, I'm not sure if this might have some side effects on other icons. Could you please have another look?

[jfaltermeier]

Code looks good to me in general and as far as I can see, all the menus are showing their content as they do in VS Code, even if the location within the context menu sometimes varies a bit. So great work, Johannes!

The only issue I could detect was with the icon. For some reason, it does not show up for me. I looked a bit into this and the image resource is properly hosted and loaded so it seems more of a CSS thing. The CSS is applied in plugin-shared-style.ts and I noticed that if I change the display property from the default inline to something else, the icon shows up. However, I'm not sure if this might have some side effects on other icons. Could you please have another look?

Thanks, I can reproduce it when I merge the current master into my branch. I think you mentioned that you've rebased before testing. Looks like #12827 causes the issue for my previous fix. The ::before causes that a different style is applied now. I'll have a look.

[jfaltermeier]

I've rebased and added a dedicated css entry that sets display to flex. With this it seems to work.

I also noticed an issue with the removeSingleRootNode method while retesting. It did not detect submenus that are not rendered (because they have no children) as something that can be skipped as well. This was reproducible immediately after installing the test vsix.
Please have a look at the implementation again.

[martin-fleck-at]

Hello Johannes, thank you for the quick update! Everything looks good to me now, I just have a minor nitpick regarding private methods in classes that may be re-used by adopters. Please let me know what you think.

[jfaltermeier]

Hello Johannes, thank you for the quick update! Everything looks good to me now, I just have a minor nitpick regarding private methods in classes that may be re-used by adopters. Please let me know what you think.

Thanks for the review. I've made the methods protected, although I would prefer them to be private. I understand why you prefer it the other way, but those methods are only existing, because we use them as helper methods from one the the actual API methods to make the code nicer. If this implementation has to change at some point, we still have the burden of having those additional API methods that can't be removed/adapted without a major API break. So I'm not a fan.

[martin-fleck-at]

Thank you Johannes, everything looks good to me now!