SCMViewlet creates many, many action items

[jrieken]

With the new listener leak limit being 100 and without 0e86dd6 you can see the following warnings on the console while scrolling up/down in a full SCM viewlet

at ContextAwareMenuItemActionItem.MenuItemActionItem.render (file:///Users/jrieken/Code/vscode/out/vs/platform/actions/browser/menuItemActionItem.js:168:50)
    at file:///Users/jrieken/Code/vscode/out/vs/base/browser/ui/actionbar/actionbar.js:505:22
    at Array.forEach (<anonymous>)
    at ActionBar.push (file:///Users/jrieken/Code/vscode/out/vs/base/browser/ui/actionbar/actionbar.js:487:21)
    at updateActions (file:///Users/jrieken/Code/vscode/out/vs/workbench/parts/scm/electron-browser/scmViewlet.js:415:36)
    at Emitter.fire (file:///Users/jrieken/Code/vscode/out/vs/base/common/event.js:223:38)
    at Menu.<anonymous> (file:///Users/jrieken/Code/vscode/out/vs/platform/actions/common/menu.js:33:40)
    at Emitter.fire (file:///Users/jrieken/Code/vscode/out/vs/base/common/event.js:226:41)
    at file:///Users/jrieken/Code/vscode/out/vs/base/common/event.js:441:37

[jrieken]

Once you have scrolled up and down as fast as you can, then scrolling just 1 row will create 180 new menu items

[joaomoreno]

@jrieken Good catch. This is a consequence of menus having coarse "I just changed" events. I then create the action array for that menu and from there create the matching action item array. As the fix, I introduce an action cache, so I can do array.equals and avoid creating action items for those actions if they are the same as before. I wonder if that caching could be pushed further down and have the "alive" menu compute that difference itself and not emit that event if nothing has changed?

On top of that there were two places in which I simply wasn't disposing disposables. 🤦‍♂️

[jrieken]

As the fix, I introduce an action cache, so I can do array.equals and avoid creating action items for those actions if they are the same as before.

Menus can fire events when the actions change (hide/show) and when some state (checked, precondition) changes. The latter two aren't exposed to extensions so it shouldn't be thing for the SCM viewlet... @joaomoreno I wonder what events you are receiving here so that the cache will ever catch something?

[joaomoreno]

I pay attention to the menu change event for when context keys change, eg git contributes different actions depending on the state of the repo.

[jrieken]

I pay attention to the menu change event for when context keys change,

You mean you only listen to the menu change but not to context key changes yourselves, right?

[joaomoreno]

Yes. I listen to the menu changes, which fires when context keys change.

[mjbvz]

I'm still seeing the following potential leak in latest master:

   at file:///Users/matb/projects/vscode/out/vs/base/common/event.js:506:20
    at Object.onFirstListenerAdd (file:///Users/matb/projects/vscode/out/vs/base/common/event.js:429:32)
    at _event (file:///Users/matb/projects/vscode/out/vs/base/common/event.js:164:44)
    at new Menu (file:///Users/matb/projects/vscode/out/vs/platform/actions/common/menu.js:31:164)
    at MenuService.createMenu (file:///Users/matb/projects/vscode/out/vs/platform/actions/common/menuService.js:23:20)
    at ResourceRenderer.renderElement (file:///Users/matb/projects/vscode/out/vs/workbench/parts/scm/electron-browser/scmViewlet.js:417:41)
    at PipelineRenderer.renderElement (file:///Users/matb/projects/vscode/out/vs/base/browser/ui/list/listWidget.js:632:26)
    at ListView.insertItemInDOM (file:///Users/matb/projects/vscode/out/vs/base/browser/ui/list/listView.js:266:22)
    at ListView.render (file:///Users/matb/projects/vscode/out/vs/base/browser/ui/list/listView.js:230:26)
    at ListView.onScroll (file:///Users/matb/projects/vscode/out/vs/base/browser/ui/list/listView.js:426:22)
    at Emitter.fire (file:///Users/matb/projects/vscode/out/vs/base/common/event.js:226:41)
    at file:///Users/matb/projects/vscode/out/vs/base/browser/ui/scrollbar/scrollableElement.js:123:33
    at Emitter.fire (file:///Users/matb/projects/vscode/out/vs/base/common/event.js:223:38)
    at Scrollable._setState (file:///Users/matb/projects/vscode/out/vs/base/common/scrollable.js:227:28)
    at Scrollable.setScrollPositionNow (file:///Users/matb/projects/vscode/out/vs/base/common/scrollable.js:160:18)
    at ScrollableElement.AbstractScrollableElement._onMouseWheel (file:///Users/matb/projects/vscode/out/vs/base/browser/ui/scrollbar/scrollableElement.js:278:42)
    at HTMLDivElement.onMouseWheel (file:///Users/matb/projects/vscode/out/vs/base/browser/ui/scrollbar/scrollableElement.js:233:27)

This was after staging node_modules and scrolling through the changes list

[mjbvz]

Actually, I'll open a new issue since I don't see the action item problem anymore