-
Notifications
You must be signed in to change notification settings - Fork 315
[Do not merge till RNW 0.82 is released] Picker - Upgrade Windows to New Arch using XAML Islands #664
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
base: master
Are you sure you want to change the base?
[Do not merge till RNW 0.82 is released] Picker - Upgrade Windows to New Arch using XAML Islands #664
Conversation
|
@Naturalclar Please review - windows new arch picker implementation |
| if (m_updating) { | ||
| return; | ||
| } |
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.
Why is m_updating needed? I mean, when control is in UpdateProps can this code path be hit? That shouldn't be a possibility in a single-threaded app. Do the event handlers get called in a different thread?
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.
Great question! You're right that this is a single-threaded app, but m_updating is still needed for a different reason.
When we programmatically set properties like SelectedIndex, Items, etc. in UpdateProps these calls synchronously trigger the event handlers on the same thread. So m_updating distinguishes between:
Programmatic changes (syncing props from JS -> native): We should not emit events back to JS
User interactions (clicking dropdown): We should emit events to JS
Let me know your thoughts...
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.
A better option would be to temporarily detach and attach handler as it's localized i.e. the change, and its repercussions are visible in the same place. OTOH a member variable is globalized i.e. a state that is visible across functions and confuse people (like I got and asked a question around it). Generally state variables are error-prone, needs consistent updating and fiddly.
// Revoke (detach) - just reset the revoker
m_sliderValueChangedRevoker. revoke();
// Set the value without triggering the handler
MySlider().Value(newValue);
// Re-register the event handler
m_sliderValueChangedRevoker = MySlider().ValueChanged(
winrt::auto_revoke,
{ this, &MyPage:: OnSliderValueChanged });You can use RAII to never forget to attach the handler back:
// Runs code with auto detach and attach of event handler.
static
template<typename TRevoker, typename TSetup, typename TAction>
void WithEventSuspended(TRevoker& revoker, TSetup setup, TAction action)
{
revoker.revoke();
action();
revoker = setup();
}
// Usage:
void MyPage::UpdateSliderProgrammatically(double newValue)
{
WithEventSuspended(
m_sliderValueChangedRevoker,
[this]() {
return MySlider().ValueChanged(
winrt::auto_revoke,
{ this, &MyPage::OnSliderValueChanged });
},
[this, newValue]() {
MySlider().Value(newValue);
});
}I've shown a slider value changed example for simplicity. You can adapt it to your needs.
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.
I see this is resolved but not changes were made nor a comment was left. I'm confused. Do you agree and forgot to make changes? You disagree and forgot to give reasons?
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.
This was resolved on my first comment and never unresolved on your second comment. I have unresolved now and will work on the fix you suggested.
sundaramramaswamy
left a comment
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.
Signing off with some comments. Please address them. LGTM otherwise!
| if (m_updating) { | ||
| return; | ||
| } |
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.
A better option would be to temporarily detach and attach handler as it's localized i.e. the change, and its repercussions are visible in the same place. OTOH a member variable is globalized i.e. a state that is visible across functions and confuse people (like I got and asked a question around it). Generally state variables are error-prone, needs consistent updating and fiddly.
// Revoke (detach) - just reset the revoker
m_sliderValueChangedRevoker. revoke();
// Set the value without triggering the handler
MySlider().Value(newValue);
// Re-register the event handler
m_sliderValueChangedRevoker = MySlider().ValueChanged(
winrt::auto_revoke,
{ this, &MyPage:: OnSliderValueChanged });You can use RAII to never forget to attach the handler back:
// Runs code with auto detach and attach of event handler.
static
template<typename TRevoker, typename TSetup, typename TAction>
void WithEventSuspended(TRevoker& revoker, TSetup setup, TAction action)
{
revoker.revoke();
action();
revoker = setup();
}
// Usage:
void MyPage::UpdateSliderProgrammatically(double newValue)
{
WithEventSuspended(
m_sliderValueChangedRevoker,
[this]() {
return MySlider().ValueChanged(
winrt::auto_revoke,
{ this, &MyPage::OnSliderValueChanged });
},
[this, newValue]() {
MySlider().Value(newValue);
});
}I've shown a slider value changed example for simplicity. You can adapt it to your needs.
sundaramramaswamy
left a comment
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.
Please reply to comments.
|
Is there a rough estimate for when RNW 0.82 will be released? |
Yes around Feb'26 or end of Jan'26 |
Co-authored-by: Sundaram Ramaswamy <suramaswamy@microsoft.com>
Co-authored-by: Sundaram Ramaswamy <suramaswamy@microsoft.com>
|
All comments are resolved. Now only waiting for RNW 0.82 Stable version to be released to merge this PR. |
Overview
This PR upgrades the Windows implementation of the Picker component to use the new architecture based on XAML Islands. The goal is to modernize the component, improve compatibility with recent React Native Windows changes, and ensure better long-term support.
Design and Implementation
XAML Islands Integration
Architecture Changes
Spec File Changes
I have removed the old implementation of windows in “js” directory for Picker.
Testing Instructions
Note
Screenshots and videos
picker_3p_windows.mp4