[NET10.0] Removed the MessagingCenter from the RadioButtonGroup#29366
[NET10.0] Removed the MessagingCenter from the RadioButtonGroup#29366rmarinho merged 10 commits intodotnet:net10.0from
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR removes the MessagingCenter-based implementation from the RadioButtonGroup and replaces it with a direct approach using parent hierarchy traversal and a ConditionalWeakTable for efficient controller management. Key changes include:
- Removing the obsolete MessagingCenter classes and related message constants.
- Refactoring the RadioButtonGroupController to manage state updates directly and clean up controller associations on child removal.
- Updating RadioButton and RadioButtonGroup logic to call the new controller methods.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Controls/tests/Core.UnitTests/RadioButtonTests.cs | Added a grid layout to group radio buttons in tests. |
| src/Controls/src/Core/RadioButton/RadioButtonGroupSelectionChanged.cs | Removed obsolete MessagingCenter message classes. |
| src/Controls/src/Core/RadioButton/RadioButtonGroupController.cs | Refactored to use ConditionalWeakTable and direct event handling for state updates. |
| src/Controls/src/Core/RadioButton/RadioButtonGroup.cs | Updated grouping logic to uncheck buttons via direct hierarchy traversal. |
| src/Controls/src/Core/RadioButton/RadioButton.cs | Modified property change handlers to use the new controller methods. |
Comments suppressed due to low confidence (1)
src/Controls/src/Core/RadioButton/RadioButtonGroupController.cs:89
- [nitpick] The variable name 'radioButton1' could be more descriptive. Consider renaming it to something like 'childRadioButton' for clarity.
if (element is RadioButton radioButton1)
|
|
||
| _layout = (Element)layout; | ||
| _layout.ChildAdded += ChildAdded; | ||
| _layout.ChildRemoved += ChildRemoved; |
There was a problem hiding this comment.
Consider adding an unsubscribe mechanism or disposing pattern for the ChildRemoved (and ChildAdded) event handlers to prevent potential memory leaks when the controller is no longer needed.
There was a problem hiding this comment.
@Tamilarasan-Paranthaman should we indeed also add a -= somewhere?
There was a problem hiding this comment.
Seems the ChildAdded was also not unsubscribe.
Should use the DisconnectHandler to remove these events?
There was a problem hiding this comment.
Shall we implement IDisposable to unhook these events? Although there doesn’t seem to be any noticeable leak caused by this.
|
Azure Pipelines successfully started running 3 pipeline(s). |
| { | ||
| internal class RadioButtonGroupController | ||
| { | ||
| static readonly ConditionalWeakTable<RadioButton, RadioButtonGroupController> groupControllers = new(); |
There was a problem hiding this comment.
Are there a couple tests, that check this works as we'd expect?
The cases would be:
- I setup a group of
RadioButtonon screen in a group, several GCs occur, are they still in the group? - I remove a group of
RadioButtonsfrom the screen, they go away and do not leak
It reminds me of tests like:
The main goal here was to implement something that doesn't leak, so I think we should also add tests for these cases. Thanks!
There was a problem hiding this comment.
@jonathanpeppers, Okay, thanks for sharing the reference. I have checked it along with other memory leak related cases, and based on that, I have added some test cases to the UI, Unit, and Device tests. Could you please take a look and let me know if you have any concerns?
|
/azp run |
jonathanpeppers
left a comment
There was a problem hiding this comment.
If CI ends up green, this looks good to me. The new tests are good. 👍
|
Azure Pipelines successfully started running 3 pipeline(s). |
src/Controls/src/Core/RadioButton/RadioButtonGroupController.cs
Outdated
Show resolved
Hide resolved
|
/rebase |
0e1913f to
b1ec095
Compare
b1ec095 to
62a97d7
Compare
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Description of Change
Issues Fixed
Fixes #24264
Tested the behaviour in the following platforms
Screenshot
Same-Group-Diff-Parent.mov
Different-Groups.mov