RadioButton #5349

andreinitescu · 2019-02-22T18:47:07Z

Description of Change

RadioButton implementation.

Issues Resolved

fixes [F100] RadioButton #2404

API Changes

Added:

public class RadioButton : IToggleButtonElement, ToggleButtonElement, ...
{
      public event EventHandler Checked;
      public event EventHandler Unchecked;

      public bool IsChecked {get; set; }  //BP
      public string GroupName { get; set; } //BP
}

interface IToggleButtonElement
{
      event EventHandler Checked;
      event EventHandler Unchecked;
}

static class ToggleButtonElement
{
     public static bool IsChecked { get; set; }  // BP
}

Platforms Affected

Core/XAML (all platforms)
iOS
Android
UWP
MacOS

Behavioral/Visual Changes

GroupName property

The "RadioButton group Gallery - Legacy" has three tabs which show group behavior.
Radio-buttons with group name null are grouped by their parent.
Radio-buttons with same group name, no matter the hierarchy, are grouped at page level.

Video: https://www.youtube.com/watch?v=fwYSmvjCfbw

Before/After Screenshots

Testing Procedure

PR Checklist

Has automated tests
Rebased on top of the target branch at time of PR
Changes adhere to coding standard

andreinitescu · 2019-02-22T18:58:49Z

Some thoughts:

ToggleButtonElement can be used as a base class for all kind of toggle buttons, RadioButton, CheckBox, or any custom drawn toggle buttons, etc.
On Android, I think RadioButtonRenderer could derive from ButtonFastRenderer because android.widget.RadioButton indirectly derives from android.widget.Button. Right now there's a lot of code duplication across the RadioButtonRenderer and ButtonFastRenderer.
Also, I'd like to use ButtonLayoutManager for padding and text, but it's meant to deal with Button's ContentLayout. I'd have to refactor ButtonLayoutManager, maybe a BaseTextButtonLayoutManager which deals with padding and text?
On iOS, it's a similar situation, much of the code in RadioButtonRenderer and ButtonRenderer is duplicated. I think it makes sense here to have a BaseButtonRenderer too.

I could have done all that already by I'm waiting for your input in order to plan my effort efficiently, thanks.

rookiejava · 2019-03-04T01:41:31Z

I'll add the Tizen implementation through the subsequent PR. 😉

samhouts · 2019-04-10T00:32:14Z

We also have #4369, and as you mentioned, there can be some shared code there. I wonder how we do that without causing a tangled mess.

StephaneDelcroix

reviewed the Core part only.

Thanks for this, I'm quite happy with what I'm seeing. Could you please review and discuss naming and code sharing strategies (with Switch), and also review the groupName scoping ? Thanks

Xamarin.Forms.Core/PlatformConfiguration/AndroidSpecific/RadioButton.cs

Xamarin.Forms.Core/ITextButtonElement.cs

Xamarin.Forms.Core/ITextElement.cs

Xamarin.Forms.Core/RadioButton.cs

Xamarin.Forms.Core/ToggleButton.cs

andreinitescu · 2019-05-21T05:12:46Z

This is very different than how the CheckBox works. I wish there was some feedback.

I put a considerable amount of time on this PR, I'm not sure how to continue with this now.

PureWeen · 2019-05-23T19:23:46Z

This is very different than how the CheckBox works.

Can you elaborate on this a bit? Are you saying that your approach of an implementation is different than how we did CheckBox? Or that fundamentally the two don't align so nothing should be shared betewen them?

ToggleButtonElement can be used as a base class for all kind of toggle buttons, RadioButton, CheckBox, or any custom drawn toggle buttons, etc.

I feel like the mileage here of a common base class wouldn't give us too much. I've found that just using interfaces to illustrate shared property is enough and then just reusing the bindable properties via the classes like ButtonElement

On Android, I think RadioButtonRenderer could derive from ButtonFastRenderer because android.widget.RadioButton indirectly derives from android.widget.Button. Right now there's a lot of code duplication across the RadioButtonRenderer and ButtonFastRenderer.
Also, I'd like to use ButtonLayoutManager for padding and text, but it's meant to deal with Button's ContentLayout. I'd have to refactor ButtonLayoutManager, maybe a BaseTextButtonLayoutManager which deals with padding and text?

Don't we want to add something like (or just exactly) ContentLayout to Checkbox and Radiobutton as well so you can position the text to the right or left of the checkbox and also effect the spacing? I hadn't dove into it yet but my plan for checkbox was to just pass the checkbox into buttonlayoutmanager, initialize it the same way it's initialized on material, and then just hope it all works :-)

Checkbox, RadioButton, and Button with an Image Source set
Are all basically the same thing right? The only difference being that CB and RB have state so the Drawable gets toggled based on that state. Otherwise they all just implement button on the native platforms and it's all just the same code we already use for Button + ImageSource + Text

On iOS, it's a similar situation, much of the code in RadioButtonRenderer and ButtonRenderer is duplicated. I think it makes sense here to have a BaseButtonRenderer too.

The implementation of ButtonRenderer on ios is super thin. It's more just passing things to Managers. I think just following the same strategy with the checkbox renderer of passing to the managers first is a good idea. Then once it's done and working we can look at refactoring to a base class. Either way I feel like this is a minor detail as most of the core shared code here was already refactored to managers

andreinitescu · 2019-05-25T09:25:27Z

Can you elaborate on this a bit? Are you saying that your approach of an implementation is different than how we did CheckBox?

My RadioButton impl. is very different than the CheckBox impl. I wanted the RadioButton to be a button , with text, pressed states. After all on native platforms which have a CheckBox (and all have it except iOS), the native check-box control is actually a button.

Or that fundamentally the two don't align so nothing should be shared betewen them?

They should be aligned I think, they are both toggle buttons and share similar properties (same IsChecked property, same CheckChanged event, and same visual states, same border properties), just with a different UI, and the RadioButton has some extra logic for grouping.

I feel like the mileage here of a common base class wouldn't give us too much. I've found that just using interfaces to illustrate shared property is enough and then just reusing the bindable properties via the classes like ButtonElement

A toggle button base class would have important advantages in my opinion:

Makes it very clear the responsibility of derived classes.
The implementation for CheckBox and RadioButton would become very short and concise, which is much easier to understand and maintain than having a lot of code duplicated.
I'm aware adding a base class must always be planned carefully because of single inheritance in C#, but for this specific purpose of a toggle button I think it should be OK.
Makes it extensible for custom controls.
If I want to create a custom drawn toggle button, I shouldn't have to create a class from scratch (IsChecked property, CheckedChanged event, border properties, visual state logic, etc.).

However, if you still think it doesn't make sense to have a common class, that's OK for me.

If you'd like me to still work on this control, please let me know feedback on the existing PR.
@StephaneDelcroix already placed feedback, but I'm confused. He is suggesting to have Switch as a base class. But the CheckBox doesn't have Switch as base class... Shouldn't CheckBox and RadioButton be aligned? Also, I'm not sure about having Switch as base class would be OK.
With little planning and guidance, I think I can work on refactoring the RadioButton to align with how you want it.

Thanks!

PureWeen · 2019-06-02T18:50:48Z

My RadioButton impl. is very different than the CheckBox impl. I wanted the RadioButton to be a button , with text, pressed states. After all on native platforms which have a CheckBox (and all have it except iOS), the native check-box control is actually a button.

I feel like very different is a bit of an overstatement :-) Currently I see CheckBox as a subset to your vision. CheckBox and RadioButton are effectively the same controls just with different images and different state behavior when you click on them. So I see whatever you are doing for RB as something that's just going to be symmetrically applied to CB by whatever means of sharing we come up with

The sharing at the cross platform level

Still not sure. Switch has IsToggled and CheckBox has IsChecked. Checkbox hasn't been released so should we call it IsToggled instead and create common base class? In a perfect world I'd say we go the android route and just create a CompoundButton that inherits from Button then just make switch, radio, checkbox all inherit from that because even at the platform level they are all just buttons with states so literally all the code we have that operates on buttons should just be applied to cb, rb, switch. Let me bounce this off a few people then get back to you.

PureWeen

Compilation error

Xamarin.Forms.Platform.iOS\Renderers\RadioButtonRenderer.cs(82,34): Error CS0311: The type 'Xamarin.Forms.RadioButton' cannot be used as type parameter 'T' in the generic type or method 'VisualElementExtensions.UseLegacyColorManagement<T>(T)'. There is no implicit reference conversion from 'Xamarin.Forms.RadioButton' to 'Xamarin.Forms.IElementConfiguration<Xamarin.Forms.RadioButton>'.
Xamarin.Forms.Platform.UAP\RadioButtonRenderer.cs(69,8): Error CS0311: The type 'Xamarin.Forms.RadioButton' cannot be used as type parameter 'T' in the generic type or method 'VisualElementExtensions.UseFormsVsm<T>(T)'. There is no implicit reference conversion from 'Xamarin.Forms.RadioButton' to 'Xamarin.Forms.IElementConfiguration<Xamarin.Forms.RadioButton>'.
Process 'msbuild.exe' exited with code '1'.

On iOS I can "uncheck" the RadioButton if I click it twice.
UWP isn't using the ContentLayout property
On iOS can we also refactor that to use the ButtonLayoutManager?

Xamarin.Forms.Platform.Android/IButtonLayoutRenderer.cs

PureWeen · 2019-08-05T20:52:46Z

Xamarin.Forms.Platform.Android/AppCompat/RadioButtonRenderer.cs

+			Tag = this;
+		}
+
+		void UpdateFont()


Can we just move all of this code into ButtonLayoutManager and anything else that's the same among the buttons? How much of the identical code can we just move into ButtonLayoutManager?

StephaneDelcroix

only re-reviewed the Core, we're not there yet (but making progress)

StephaneDelcroix · 2019-08-06T09:21:04Z

Xamarin.Forms.Core/RadioButton.cs

+			set { SetValue(IsCheckedProperty, value); }
+		}
+
+		public string GroupName


you have

StackLayout Frame RadioButton Frame RadioButton

so the 2 radio buttons aren't in the same group ?

StephaneDelcroix · 2019-08-06T09:21:23Z

Xamarin.Forms.Core/RadioButton.cs

+		public static readonly BindableProperty ButtonSourceProperty = BindableProperty.Create(
+			nameof(ButtonSource), typeof(ImageSource), typeof(RadioButton), null);
+
+		public event EventHandler<CheckedChangedEventArgs> CheckedChanged;


I don't see how this is resolved

StephaneDelcroix · 2019-08-06T09:22:58Z

Xamarin.Forms.Core/RadioButton.cs

+	[RenderWith(typeof(_RadioButtonRenderer))]
+	public partial class RadioButton : IToggleButtonElement, IToggleButtonController, ITextButtonElement
+	{
+		static Dictionary<string, List<WeakReference<RadioButton>>> _groupNameToElements;


the problem with this approach is the List keeps getting populated, and so the performance degrades over time

StephaneDelcroix · 2019-08-06T09:25:21Z

Xamarin.Forms.Core/RadioButton.cs

+			set { SetValue(IsCheckedProperty, value); }
+		}
+
+		public string GroupName


also, you say the scope is the parent, but looking at GetVisualRoot, the scope is the page. what if I have this

Page StackLayout YesNoMaybeControl YesNoMaybeControl YesNoMaybeControl

StephaneDelcroix · 2019-08-06T09:25:36Z

Xamarin.Forms.Core/RadioButton.cs

+			}
+		}
+
+		static Element GetVisualRoot(Element element)


StephaneDelcroix · 2019-08-06T09:26:11Z

Xamarin.Forms.Core/RadioButton.cs

+	[RenderWith(typeof(_RadioButtonRenderer))]
+	public partial class RadioButton : IToggleButtonElement, IToggleButtonController, ITextButtonElement
+	{
+		static Dictionary<string, List<WeakReference<RadioButton>>> _groupNameToElements;


I've seen PurgeDeads, but we probably can do better

StephaneDelcroix · 2019-08-06T09:29:39Z

Xamarin.Forms.Core/RadioButton.cs

+
+		public event EventHandler<CheckedChangedEventArgs> CheckedChanged;
+
+		public bool IsChecked


Checked is probably a better name. We try to avoid property names containing multiple verbs. The IsXxxx construct is valid when Xxxx is an adjective. We can debate that in this case Checked is used as an adjective or not, I still think the name is overloaded

this is not a big deal, I know we have IsEnabled already, and might want to reserve the Checked name for the event. so it's fine to leave it as is, and rename the event

StephaneDelcroix · 2019-08-06T09:34:05Z

Xamarin.Forms.Core/RadioButton.cs

+namespace Xamarin.Forms
+{
+	[RenderWith(typeof(_RadioButtonRenderer))]
+	public partial class RadioButton : Button


drop the partial. where is the IToggleElement interface you use to have ?

StephaneDelcroix · 2019-08-06T09:35:08Z

Xamarin.Forms.Core/RadioButton.cs

+
+		public const string IsCheckedVisualState = "IsChecked";
+
+		public static readonly BindableProperty IsCheckedProperty = BindableProperty.Create(


this isn't 2-Way, but should

StephaneDelcroix · 2019-08-06T09:36:14Z

Xamarin.Forms.Core/RadioButton.cs

+		public static readonly BindableProperty GroupNameProperty = BindableProperty.Create(
+			nameof(GroupName), typeof(string), typeof(RadioButton), null, propertyChanged: (b, o, n) => ((RadioButton)b).OnGroupNamePropertyChanged((string)o, (string)n));
+
+		public static readonly BindableProperty ButtonSourceProperty = BindableProperty.Create(


how does this works ? is this a spriteMap ? don't we need a source for both on and off ?

jsuarezruiz · 2019-08-12T10:56:20Z

@andreinitescu There is some feedback (and also conflicts). Let us know if we can help you with something ;)

* RadioButton * Removed unused files * Rebase and make it run * First round of feedback * Revert AppCompatButton -> AButton * Cleaned minor usings * Fix unselecting radiobutton on iOS * Fixed Mac OS grouping * [Android] Fix API29 usages Co-authored-by: Andrei Nitescu <nitescua@yahoo.com> Co-authored-by: Rui Marinho <me@ruimarinho.net>

SagarPanwala · 2020-04-22T10:41:00Z

How end user can differentiate between RadioButton and Checkbox in iOS ?
Current implementation of checkbox in iOS is in Round Shape, which identically says user that if there are group of checkboxes, they can select only 1.
Can we make Square shape for checkbox in iOS as well or give a property for shape?

andreinitescu requested a review from StephaneDelcroix as a code owner February 22, 2019 18:47

andreinitescu force-pushed the RadioButton branch from d23c9af to e882632 Compare February 22, 2019 19:05

samhouts added the t/enhancement ➕ label Feb 22, 2019

andreinitescu force-pushed the RadioButton branch 2 times, most recently from 3e9f3d3 to 9497861 Compare March 8, 2019 06:45

samhouts self-requested a review April 8, 2019 16:40

samhouts assigned samhouts and StephaneDelcroix Apr 8, 2019

samhouts added the API-change Heads-up to reviewers that this PR may contain an API change label Apr 10, 2019

StephaneDelcroix suggested changes Apr 16, 2019

View reviewed changes

samhouts added p/iOS 🍎 p/Android p/UWP p/macOS DO-NOT-MERGE-!!! 🛑 This is in progress and needs to be updated before it can be merged. labels Apr 24, 2019

samhouts added roadmap F100 labels Jun 3, 2019

andreinitescu mentioned this pull request Jul 26, 2019

[Bug] [Android] CheckBox tab order #6955

Closed

PureWeen mentioned this pull request Jul 26, 2019

[Bug][iOS] CheckBox accessibility: no tab selection #6954

Open

PureWeen suggested changes Aug 5, 2019

View reviewed changes

StephaneDelcroix suggested changes Aug 6, 2019

View reviewed changes

samhouts added Core ControlGallery labels Sep 5, 2019

samhouts removed the retarget-branch-required PR or associated issues target a milestone. Please target this PR to the matching branch. label Dec 3, 2019

jfversluis mentioned this pull request Dec 16, 2019

RadioButton Implementation #8910

Merged

4 tasks

samhouts closed this Jan 17, 2020

samhouts added the proposal-open label Feb 7, 2020

samhouts added this to the 4.6.0 milestone Feb 28, 2020

samhouts added the a/radiobutton 🔘 label Apr 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RadioButton #5349

RadioButton #5349

andreinitescu commented Feb 22, 2019 •

edited

Loading

andreinitescu commented Feb 22, 2019 •

edited

Loading

rookiejava commented Mar 4, 2019

samhouts commented Apr 10, 2019

StephaneDelcroix left a comment

andreinitescu commented May 21, 2019 •

edited

Loading

PureWeen commented May 23, 2019 •

edited

Loading

andreinitescu commented May 25, 2019 •

edited

Loading

PureWeen commented Jun 2, 2019

PureWeen left a comment

PureWeen Aug 5, 2019

StephaneDelcroix left a comment

StephaneDelcroix Aug 6, 2019

StephaneDelcroix Aug 6, 2019

StephaneDelcroix Aug 6, 2019

StephaneDelcroix Aug 6, 2019

StephaneDelcroix Aug 6, 2019

StephaneDelcroix Aug 6, 2019

StephaneDelcroix Aug 6, 2019

StephaneDelcroix Aug 6, 2019

StephaneDelcroix Aug 6, 2019

StephaneDelcroix Aug 6, 2019

StephaneDelcroix Aug 6, 2019

jsuarezruiz commented Aug 12, 2019

SagarPanwala commented Apr 22, 2020


		public event EventHandler<CheckedChangedEventArgs> CheckedChanged;

		public bool IsChecked


		public const string IsCheckedVisualState = "IsChecked";

		public static readonly BindableProperty IsCheckedProperty = BindableProperty.Create(

RadioButton #5349

RadioButton #5349

Conversation

andreinitescu commented Feb 22, 2019 • edited Loading

Description of Change

Issues Resolved

API Changes

Platforms Affected

Behavioral/Visual Changes

GroupName property

Before/After Screenshots

Testing Procedure

PR Checklist

andreinitescu commented Feb 22, 2019 • edited Loading

rookiejava commented Mar 4, 2019

samhouts commented Apr 10, 2019

StephaneDelcroix left a comment

Choose a reason for hiding this comment

andreinitescu commented May 21, 2019 • edited Loading

PureWeen commented May 23, 2019 • edited Loading

andreinitescu commented May 25, 2019 • edited Loading

PureWeen commented Jun 2, 2019

PureWeen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephaneDelcroix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsuarezruiz commented Aug 12, 2019

SagarPanwala commented Apr 22, 2020

andreinitescu commented Feb 22, 2019 •

edited

Loading

andreinitescu commented Feb 22, 2019 •

edited

Loading

andreinitescu commented May 21, 2019 •

edited

Loading

PureWeen commented May 23, 2019 •

edited

Loading

andreinitescu commented May 25, 2019 •

edited

Loading