Prevent Tab Switcher from increasing Tab refcount #8653
Conversation
| _tabChangedRevoker = tab.PropertyChanged(winrt::auto_revoke, [weakThis{ get_weak() }](auto& sender, auto& e) { | ||
| auto item{ weakThis.get() }; | ||
| auto senderTab{ sender.try_as<TabBase>() }; | ||
|
|
||
| if (item && senderTab) | ||
| { | ||
| auto changedProperty = e.PropertyName(); | ||
| if (changedProperty == L"Title") | ||
| { | ||
| item->Name(senderTab.Title()); | ||
| } | ||
| else if (changedProperty == L"Icon") | ||
| { | ||
| item->Icon(senderTab.Icon()); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
are we going to be upset that we don't have this?
There was a problem hiding this comment.
@DHowett - happy new year! Missed you! 😊
Yes, we will be a upset a bit, as if the switcher is already visible the change in the tab's name / icon won't get reflected immediately. The update will become visible only the next time the tab switcher is open.
We definitely need to find a solution for this, but I assumed this one is urgent enough to restore the functionality in some follow up.
@zadjii-msft - know what.. I only read about this.. but never tried not to remove. I will check and update you. UPDATE: It works.. 🤦 I think I was confused.. as I started with making FilteredCommand::Item to be weak ref. And in this case I could not make items implement INotifyPropertyChanged, as the binding will increase the refcount (it needs to store it). However, it is irrelevant here as actually the Item remained strong-ref and only the Tab is weak. |
ghost
added
Area-User Interface
|
I love this. Thanks so much. Happy new year indeed! |
DHowett
added
the
AutoMerge
|
Hello @DHowett! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
ghost
merged commit
|
🎉 Handy links: |
ghost
mentioned this pull request
Fix `TabPaletteItem` to hold only a weak reference to a tab. This way we guarantee that the refcount of the closed tab gets to 0 immediately (and that command palette cannot "raise it from the dead"). While this seems a correct thing to do, it is still not clear why the `FilteredCommand` itself (the one holding the `TabPaletteItem`) doesn't get released until the UI is refreshed. There is an impact of not registering to PropertyChanged event: if the tab title changes during Tab Switcher navigation the Tab Switcher item won't be updated immediately (the change will apply next time the Tab Switcher is open). Due to this change we need to make sure that the tabs binding in microsoft#8427 doesn't break the title / icon update. ## Validation Steps Performed * Manual testing Closes microsoft#8651
Fix
TabPaletteItemto hold only a weak reference to a tab.This way we guarantee that the refcount of the closed tab
gets to 0 immediately
(and that command palette cannot "raise it from the dead").
While this seems a correct thing to do,
it is still not clear why the
FilteredCommanditself(the one holding the
TabPaletteItem) doesn't get releaseduntil the UI is refreshed.
There is an impact of not registering to PropertyChanged event:
if the tab title changes during Tab Switcher navigation
the Tab Switcher item won't be updated immediately
(the change will apply next time the Tab Switcher is open).
Due to this change we need to make sure that the tabs binding
in #8427
doesn't break the title / icon update.
Validation Steps Performed
Closes #8651