Fixes #44396 - cancel alt up after mouse down on menu#44397
Fixes #44396 - cancel alt up after mouse down on menu#44397isidorn merged 1 commit intomicrosoft:masterfrom
Conversation
|
@isidorn not sure if you or Joh, but I remember you were in this code for Alt support in menus. |
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.
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. |
4 participants
Fixes #44396