Skip to content

Conversation

@zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft commented Aug 24, 2022

While debugging #13694, we discovered a very subtle bug we had accidentally introduced in a few places. ViewModelHelper defines a PropertyChanged event, backed by a _propertyChangedHandlers event. All opbservable properties in the viewmodels are supposed to run through that event. However, if you do WINRT_CALLBACK(PropertyChanged, Windows::UI::Xaml::Data::PropertyChangedEventHandler) in a derived class, it'll override that original method with the new one. XAML will subscribe to the second one, which is backed by _PropertyChangedHandlers, but the properties will still raise notifications on the callbacks registered to _propertyChangedHandlers.

This change makes it more explicit in these derived classes, that the PropertyChanged method exposed by these classes is indeed the one that's implemented in the base class.

This is a bit of a footgun, for sure. AuditMode would have apparently caught this, as we'd be overriding a method without using the override keyword.

Unblocks #13694.

@DHowett
Copy link
Member

DHowett commented Aug 24, 2022

@carlos-zamora you will need to propagate some of these changes to the new ColorScheme page!

@DHowett DHowett marked this pull request as draft August 24, 2022 22:36
@DHowett
Copy link
Member

DHowett commented Aug 24, 2022

I converted this to a draft so that it would not merge without a description

Editor::ColorTableEntry ColorEntryAt(uint32_t index);

WINRT_CALLBACK(PropertyChanged, Windows::UI::Xaml::Data::PropertyChangedEventHandler);
// DON'T YOU DARE ADD A `WINRT_CALLBACK(PropertyChanged` TO A CLASS DERIVED FROM ViewModelHelper. Do this instead:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(This should be "why" and not "what".)

Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once this merges, I'll merge it into #13269 and update that PR accordingly

@zadjii-msft
Copy link
Member Author

@msftbot merge this in 2 minutes

@ghost ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Aug 25, 2022
@ghost
Copy link

ghost commented Aug 25, 2022

Hello @zadjii-msft!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

  • I won't merge this pull request until after the UTC date Thu, 25 Aug 2022 13:59:01 GMT, which is in 2 minutes

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

@zadjii-msft zadjii-msft added Issue-Bug It either shouldn't be doing this or needs an investigation. Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Area-SettingsUI Anything specific to the SUI labels Aug 25, 2022
@ghost ghost merged commit 94339c1 into main Aug 25, 2022
@ghost ghost deleted the dev/migrie/b/subtle-propertychanged-on-main branch August 25, 2022 14:00
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Area-SettingsUI Anything specific to the SUI AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants