Fixes #44396 - cancel alt up after mouse down on menu #44397
Conversation
|
@isidorn not sure if you or Joh, but I remember you were in this code for Alt support in menus. |
| const altKey = AltKeyEmitter.getInstance(this._contextMenuService); | ||
| if (altKey.isPressed) { | ||
| altKey.suppressAltKeyUp(); | ||
| } |
There was a problem hiding this comment.
@eamodio Why is this logic only needed in one place and not in the other?
There was a problem hiding this comment.
@jrieken I'm not sure I'm following — which other place? Do you mean in ContextAwareMenuItemActionItem? If so it might need to be there too, but I can't get any alt stuff to work with any context menus — if you hold alt and try to open a context menu it opens and quickly disappears, and if you hit alt while a context menu is open it just disappears too.
There was a problem hiding this comment.
We have two references in the same file... But then I haven't been in this code for long. @isidorn I leave the review to you.
| suppressAltKeyUp() { | ||
| this._suppressAltKeyUp = true; | ||
| } | ||
|
|
||
| @memoize | ||
| static getInstance(contextMenuService: IContextMenuService) { |
There was a problem hiding this comment.
@isidorn unrelated, related but using @memoize is risky because I am pretty sure it memoizes without looking at the arguments and I am not sure there is only one contextMenuService around (i might be mistaken...)
There was a problem hiding this comment.
@jrieken yes it ignores the arguments. Thanks for letting me know, will try to fix.
|
This week we are in endgame and on thursday I go on vacation -> pushing to next milestone when I will have time to review. |
|
@isidorn let me know if you need anything else on this. |
|
@eamodio sorry for the slow response I came back from vacation a couple of days ago. Thus pushing this to may. I also see conficlits with the master. Can you please resolve those conflicts so I can review this some time next week. Thanks |
|
@isidorn no worries, sorry for the delay in re-syncing this 😄 |
|
@eamodio I tried this out and it works expected. I will just add some comments since this is a workaround so we remember what it was for. |
Fixes #44396