-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Fix Switch Off Track Color - Fixes #10099 #10758
Conversation
Needs tests 😋 |
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.
Let's guard against NREs here.
I added the NRE checks fine, but the tests that I added started failing again locally for other reasons. I am looking into it some more, but it seems that the UIView that I get into my test is different (different handle at least) and the colors that were set are not set for the argument passed into my test. Visually, things work as expected though.. |
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
Thank you for your pull request. We are auto-formatting your source code to follow our code guidelines. |
@PureWeen @mattleibow @mandel-macaque This one should be good now! |
if (OperatingSystem.IsIOSVersionAtLeast(13) || OperatingSystem.IsTvOSVersionAtLeast(13)) | ||
uIView = uISwitch.Subviews[0].Subviews[0]; | ||
return uISwitch.Subviews?.FirstOrDefault()?.Subviews?.FirstOrDefault(); |
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.
Can we not use linq for this?
Just make it a long if statement or nested?
We typically just stay away from LINQ for performance 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.
I'm going to agree, there is nothing nice we can do. Sometimes code has to be ugly and work.
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.
@mandel-macaque and @PureWeen updated! Thanks for the idea!
} | ||
|
||
internal static UIColor? GetTrackColor(this UISwitch uISwitch) | ||
{ | ||
return uISwitch.GetTrackSubview()?.BackgroundColor; | ||
} | ||
|
||
internal static T? FirstOrDefaultNoLinq<T>(this T[] items) |
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.
can you move this into ArrayExtensions?
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.
@PureWeen When I move this to ArrayExtensions as is, I get this error: "Error CS0518: Predefined type 'System.Index' is not defined or imported(CS0518) (Core)". I'm not sure why as it appears that this is an error I could get prior to having C#8 but we are in the same project.
I changed the implementation to something that I don't believe is worse, but let me know if there is a better solution to this?
@@ -64,5 +64,7 @@ public static T[] RemoveAt<T>(this T[] self, int index) | |||
/// <param name="self">The array to retrieve the last item from.</param> | |||
/// <returns>An object of type <typeparamref name="T"/> that is the last item in the collection.</returns> | |||
public static T Last<T>(this T[] self) => self[self.Length - 1]; | |||
|
|||
internal static T? FirstOrDefaultNoLinq<T>(this T[] items) => items is null || items.Length == 0 ? default : items[0]; |
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.
@mandel-macaque this look alright to you?
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.
Looks good, but I am surprise the compiler is not printing a warning with nullability enabled saying something like 'items is not set as nullable'. I think we probably want to do something like:
internal static T? FirstOrDefaultNoLinq<T>(this T[] items) => items is null || items.Length == 0 ? default : items[0]; | |
internal static T? FirstOrDefaultNoLinq<T>(this T[]? items) => items is null || items.Length == 0 ? default : items[0]; |
Right?
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.
Looking at FirstOrDefaults behavior, this sounds like the correct approach :)
@tj-devel709 would love a backport to .NET 7 |
Some people requested a backport to net7. This is functionality that works in Xamarin.Forms and the change seems safe enough. On the other hand, a workaround is available and we're close to .NET 8 where this is already fixed. |
Fixes #10099
The customer points out that when a switch track color is set, the switch is turned on, then when the switch is turned off, the switch track color stays on which is not the expected behavior. See below:
Original Incorrect Behavior
Adjusted Behavior
After the fix from this PR, the switch works as expected:
Customer reported issues in both android and iOS when using the VisualStateManager, but android worked fine for me without any fixes as shown above