Pass correct "effectiveMenuPath" to menu actions. #16148
Conversation
Fixes eclipse-theia#16132, eclipse-theia#16082 Contributed on behalf of STMicroelectronics Signed-off-by: Thomas Mäder <[email protected]>
Fixes eclipse-theia#16148 Contributed on behalf of STMicroelectronics Signed-off-by: Thomas Mäder <[email protected]>
|
Hi @tsmaeder, @colin-grant-work, @martin-fleck-at, Thanks everyone for the effort and discussion around the three PRs addressing the issue. I would like to get to a decision and get the fix in for the upcoming release (next week). From the feedback, it seems there is a preference towards avoiding unnecessary breaking changes while still improving the API surface. Given that, and unless there are strong objections, I would propose to move ahead with this current PR for this release. It provides a reasonable compromise between stability and cleanup, and ensures we have a solution ready in time. If further refinements (e.g. along the lines of #16149) are still desired, we can continue iterating in a follow-up PR. Does that sound acceptable to everyone? |
colin-grant-work
left a comment
colin-grant-work
left a comment
There was a problem hiding this comment.
Solves the immediate problem. 👍
| /* | ||
| return this.menuNode.children.map(child => { | ||
| if (CompoundMenuNode.is(child)) { | ||
| return new ToolbarItemAsSubmenuWrapper(child, [...this.effectiveMenuPath, child.id]); | ||
| } else if (CommandMenu.is(child)) { | ||
| return new ToolbarItemAsCommandMenuWrapper(child, [...this.effectiveMenuPath, child.id]); | ||
| } else { | ||
| throw new Error('should not happen'); | ||
| } | ||
| }).filter(node => node !== undefined).filter(node => node as MenuNode);*/ |
| * This class wraps a menu node, but replaces the effective menu path. Command parameters need to be mapped | ||
| * for commands contributed by extension and this mapping is keyed by the menu path |
There was a problem hiding this comment.
It probably would have been helpful if I'd made a similar comment back in #11290
github-project-automation
bot
moved this from Waiting on reviewers
to Needs merge
in PR Backlog
|
@ndoschek, I'm ok moving forward with this fix for now. |
|
Hi @tsmaeder, do you have time to update this PR so we can get it in by Thursday for the release? |
|
Probably no, quite busy this week. |
|
Ok thanks! No worries, we'll take care of it then! |
ndoschek
left a comment
ndoschek
left a comment
There was a problem hiding this comment.
Thanks for the update @colin-grant-work, I tested the mentioned commands and menus and works as expected 👍
|
Hi @colin-grant-work, I re-triggered the ECA validation today, and it was successful now 👍 |
colin-grant-work
merged commit 9ce14ab
into
eclipse-theia:master
3 participants
Fixes #16132, #16082
What it does
How to test
Make sure the various "commit" actions work now as described in the issue. Test with both "native" and "custom" title bar styles.
Make sure other menus still work fine (e.g. "Explorer" context menu, toolbar actions, etc.)
Follow-ups
Breaking changes
Attribution
Contributed on behalf of STMicroelectronics
Review checklist
Reminder for reviewers