Skip to content
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

Picker ItemsSource fixes #19728

Merged
merged 10 commits into from
Jun 2, 2024
Merged

Conversation

BurningLights
Copy link
Contributor

Description of Change

Fixes the Picker getting stuck in an infinite loop when the ItemsSource is assigned as an ObservableCollection and the collection is modified during a change handler when the SelectedIndex changes.

Issues Fixed

Fixes #19702

@BurningLights BurningLights requested a review from a team as a code owner January 7, 2024 00:41
@ghost ghost added the community ✨ Community Contribution label Jan 7, 2024
@ghost
Copy link

ghost commented Jan 7, 2024

Hey there @BurningLights! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@@ -682,7 +682,7 @@ public async Task AbsoluteRoutingToPage()

Routing.RegisterRoute("catdetails", typeof(ContentPage));

Assert.ThrowsAnyAsync<Exception>(async () => await shell.GoToAsync($"//catdetails"));
await Assert.ThrowsAnyAsync<Exception>(async () => await shell.GoToAsync($"//catdetails"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is interesting. Is there no CS4014 warning when await is missing?

Copy link
Contributor

@MartyIX MartyIX Jan 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW: The regex ^[ \t]+Assert\.ThrowsAsync seems to discover similar cases:

9 results - 6 files

src\Controls\tests\Core.UnitTests\DeviceUnitTests.cs:
  110  			async Task MethodThatThrows() => await task;
  111: 			Assert.ThrowsAsync<ApplicationException>(MethodThatThrows);
  112  		});

  146  			async Task MethodThatThrows() => await task;
  147: 			Assert.ThrowsAsync<ApplicationException>(MethodThatThrows);
  148  		});

src\Controls\tests\Core.UnitTests\NavigationUnitTest.cs:
  838  			var navigationPage = new TestNavigationPage(true, contentPage1);
  839: 			Assert.ThrowsAsync<InvalidOperationException>(() => window.Navigation.PopModalAsync());
  840  		}

src\Controls\tests\Core.UnitTests\ScrollViewUnitTests.cs:
  273  
  274: 			Assert.ThrowsAsync<ArgumentException>(() => scrollView.ScrollToAsync(new VisualElement(), ScrollToPosition.Center, true));
  275: 			Assert.ThrowsAsync<ArgumentException>(() => scrollView.ScrollToAsync(null, (ScrollToPosition)500, true));
  276  		}

src\Essentials\test\UnitTests\AppActions_Tests.cs:
  10  		public void AppActions_SetActions() =>
  11: 			Assert.ThrowsAsync<NotImplementedInReferenceAssemblyException>(() => AppActions.SetAsync(new List<AppAction>()));
  12  

  14  		public void AppActions_GetActions() =>
  15: 			Assert.ThrowsAsync<NotImplementedInReferenceAssemblyException>(() => AppActions.GetAsync());
  16  

src\Essentials\test\UnitTests\Sms_Tests.cs:
  11  		public Task Sms_Fail_On_NetStandard() =>
  12: 			Assert.ThrowsAsync<NotImplementedInReferenceAssemblyException>(() => Sms.ComposeAsync());
  13  	}

src\Essentials\test\UnitTests\TextToSpeech_Tests.cs:
  10  		public void TextToSpeech_Speak_Fail_On_NetStandard() =>
  11: 			 Assert.ThrowsAsync<NotImplementedInReferenceAssemblyException>(() => TextToSpeech.SpeakAsync("Xamarin Essentials!"));
  12  

Edit: regex ^[ \t]+Assert\.Throws[a-zA-Z]*Async seems to be better.

@jfversluis
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@Eilon Eilon added the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label Jan 10, 2024
@rmarinho
Copy link
Member

Can you rebase on main please?

@BurningLights
Copy link
Contributor Author

@dotnet-policy-service agree

@rmarinho
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@SarthakB26
Copy link

I am experiencing this too. Picker behave very weirdly when inside collection view.

@@ -267,12 +267,12 @@ public void TestScrollToElementNotAnimated()
}

[Fact]
public void TestScrollToInvalid()
public async void TestScrollToInvalid()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

async Task

@samhouts samhouts added the stale Indicates a stale issue/pr and will be closed soon label Feb 6, 2024
@BurningLights
Copy link
Contributor Author

/azp run

Copy link

Commenter does not have sufficient privileges for PR 19728 in repo dotnet/maui

@Eilon Eilon added area-controls-picker Picker and removed legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor labels May 13, 2024
@PureWeen
Copy link
Member

PureWeen commented Jun 2, 2024

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen
Copy link
Member

PureWeen commented Jun 2, 2024

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@kubaflo
Copy link
Contributor

kubaflo commented Jun 2, 2024

Looks fine to me! Maybe a UI test would be useful, but I can add it in this PR: #22353

@PureWeen PureWeen merged commit cfbb5ae into dotnet:main Jun 2, 2024
47 of 49 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jul 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-controls-picker Picker community ✨ Community Contribution stale Indicates a stale issue/pr and will be closed soon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Picker goes into infinite loop when updating ItemsSource ObservableCollection during SelectedIndex update
10 participants