Fix event leak on reuse of touchbar item#12527
Conversation
nornagon
left a comment
nornagon
left a comment
There was a problem hiding this comment.
It's confusing that there are two things called changeListener, but it looks like you got the right one. 👍
There was a problem hiding this comment.
I think this is undoing the item.on('change', this.changeListener) in registerItem(), but the next line there is to also register each of item.child.ordereredItems iff item.child is a TouchBar. Do we need to undo that here too?
If we don't also need that, then I'm 👍 on this PR
There was a problem hiding this comment.
edit: and walking recursively down all TouchBar subtrees
There was a problem hiding this comment.
edit 2: do we also need to remove the escapeItem changeListener too?
There was a problem hiding this comment.
I now clean up recursively and deal with the escapeItem listener
| window.removeListener('closed', removeListeners) | ||
| window._touchBar = null | ||
| delete this.windowListeners[id] | ||
| for (const item of this.ordereredItems) { |
There was a problem hiding this comment.
'ordereredItems'? not new to this PR, but... 🙄 😄
There was a problem hiding this comment.
items is a map of id --> item
orderedItems is an array giving you the left->right order of those items containing references to the objects in the items map
There was a problem hiding this comment.
Right, I get it. Just amused by the spelling ordereredItems 🥈
There was a problem hiding this comment.
Oh, wow, ok. That's a typo I never noticed 😆
|
We have automatically backported this PR to "1-8-x", please check out #12623 |
3 participants
Fixes #12508