New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix event leak on reuse of touchbar item #12527
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -118,6 +118,9 @@ class TouchBar extends EventEmitter { | |
window.removeListener('closed', removeListeners) | ||
window._touchBar = null | ||
delete this.windowListeners[id] | ||
for (const item of this.ordereredItems) { | ||
item.removeListener('change', this.changeListener) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is undoing the If we don't also need that, then I'm 👍 on this PR There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. edit: and walking recursively down all TouchBar subtrees There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. edit 2: do we also need to remove the escapeItem changeListener too? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I now clean up recursively and deal with the |
||
} | ||
} | ||
window.once('closed', removeListeners) | ||
this.windowListeners[id] = removeListeners | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'ordereredItems'? not new to this PR, but... 🙄 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
items
is a map of id --> itemorderedItems
is an array giving you the left->right order of those items containing references to the objects in theitems
mapThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I get it. Just amused by the spelling
ordereredItems
🥈There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, wow, ok. That's a typo I never noticed 😆