Subscribes to the property chain upto the application for theme binding. - #8957
AmrAlSayed0 wants to merge 4 commits into
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
Thanks for your contribution @AmrAlSayed0 ! I see there are some build errors for this one unfortunately:
|
|
"never used" is usually a warning not an error so warning as errors is probably turned on but anyway, I will repush another change fixing this soon. |
|
@jfversluis Done. |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
Thank you for your pull request. We are auto-formating your source code to follow our code guidelines. |
StephaneDelcroix
left a comment
StephaneDelcroix
left a comment
There was a problem hiding this comment.
Could you please add a unit test that'd fail without your changes, and pass with it? Thx
|
@AmrAlSayed0 I know it's been a while so just let us know if you'd like us to take this PR over from you or if you still would like to work on it. |
|
@PureWeen Yes, I submitted this at a time when I had some free time but now, sadly, I don't. Please tell me if there is anything I can do to give you the access or something. I don't know what taking over actually involve 😅 |
| } | ||
|
|
||
| void OnRequestedThemeChanged(object sender, AppThemeChangedEventArgs e) | ||
| => ApplyCore(true); |
There was a problem hiding this comment.
Does this generate on background threads? I'm wondering if we should have a version of this that always fires on the UIThread. Otherwise, every single AppThemeBinding is going to dispatch changes individually. If we just ensure that on Window there's a version of this (or maybe this one already does) that fires on the UIThread then all the AppThemeBindings changes can just happen at once.
|
|
||
| void OnWindowChanging(object sender, PropertyChangingEventArgs e) | ||
| { | ||
| if (string.Equals(e.PropertyName, VisualElement.WindowProperty.PropertyName, StringComparison.Ordinal)) |
There was a problem hiding this comment.
Should we call DetachEvents from WindowChanging and ApplicationChanging? Or does Unapply run when that happens?
|
@AmrAlSayed0 sounds good! Thank you for the PR and apologies it took so long to get back to you on this. We can just take it over. No need to do anything. |
|
Closing in favor of #19931 |
6 participants
Description of Change
Changed AppThemeBinding to subscribe to the changes happening to the property chain up to the parent application so that the target theme is always in sync with the current AppTheme
Issues Fixed
Fixes #8713