Performance fixing#96
Conversation
Multiple UI and ViewModel refactors: - SearchablePickerPopup: replaced ScrollView/StackLayout batching with a CollectionView bound to an ObservableCollection (FilteredItems); implemented debounced search, Select/Add commands, cancellation-safe background work, normalized _allItems, and post-add synchronization that refreshes ingredient caches and notifies AddRecipeViewModel and IngredientsPage. Removed complex background batching/versioning logic in favor of incremental collection synchronization. - SimpleListPopup: replaced ScrollView/StackLayout with a CollectionView, added VisibleItems/PrimaryTextColor bindings, improved theme handling, added OnPopupContentChanged to rebuild on property updates, support for add/edit flows, and a PopupItemHostView to render items with the existing CreateViewForItem logic. Simplified and hardened meal preview and recipe refresh flows. - AddRecipeViewModel: introduced public SyncAvailableIngredientNames to efficiently synchronize AvailableIngredientNames on the main thread using minimal diffs (avoids full UI rebuilds); updated ingredient loading to prepare normalized name list and call the new sync method. - Localization: added new resource accessors for ForceSync* and SyncOverlay* strings in ProfilePageResources.Designer.cs. Overall: improves responsiveness, reduces UI blocking, centralizes synchronization of ingredient lists, and adds missed localization keys.
Replace mutable ObservableCollection for available ingredient names with an IReadOnlyList snapshot and centralized normalization to avoid redundant UI syncs. Introduced NormalizeIngredientNames and made MainThread-safe updates (with change detection and debug logs) from LoadAvailableIngredientsAsync and SyncAvailableIngredientNames. Refactored AddRecipePage modal popup overlay logic into ApplyModalPopupOverlay/RestoreModalPopupOverlay/ResolveOpaquePopupOverlayColor, removed wallpaper-dependent transparency, and ensure overlays are applied/restored (including CRUDComponentPopup) and underlying content hidden. Also broadened SearchablePickerComponent.ItemsSource from IList<string> to IEnumerable<string> to accept more collection types.
There was a problem hiding this comment.
Pull request overview
This PR aims to improve UI performance and responsiveness in popup-based pickers/lists by replacing ScrollView + StackLayout rendering with CollectionView virtualization and simplifying ingredient-name synchronization after adding a new ingredient.
Changes:
- Migrated
SimpleListPopupandSearchablePickerPopupitem rendering toCollectionViewwith bound item sources. - Added debounced search and incremental
ObservableCollectionsyncing for filtered results inSearchablePickerPopup. - Refactored modal popup overlay handling in
AddRecipePageand changed ingredient-name exposure to an immutable snapshot (IReadOnlyList) inAddRecipeViewModel.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| FoodbookApp.App/Views/Components/SimpleListPopup.xaml.cs | Reworks popup to bind a flattened VisibleItems list into a CollectionView and sets templates in code-behind. |
| FoodbookApp.App/Views/Components/SimpleListPopup.xaml | Replaces ScrollView host with CollectionView and adds a header row for “Dodaj składnik”. |
| FoodbookApp.App/Views/Components/SearchablePickerPopup.xaml.cs | Switches to bound FilteredItems, adds debounce search, and refactors post-add ingredient refresh/sync. |
| FoodbookApp.App/Views/Components/SearchablePickerPopup.xaml | Replaces ScrollView host with CollectionView and item template buttons. |
| FoodbookApp.App/Views/Components/SearchablePickerComponent.xaml.cs | Changes ItemsSource bindable property type to IEnumerable<string>. |
| FoodbookApp.App/Views/AddRecipePage.xaml.cs | Refactors popup overlay application/restoration and centralizes overlay color resolution. |
| FoodbookApp.App/ViewModels/AddRecipeViewModel.cs | Changes ingredient names to IReadOnlyList snapshot with a sync helper method and normalization. |
| FoodbookApp.App/Localization/ProfilePageResources.Designer.cs | Adds new localized resource accessors (auto-generated). |
Files not reviewed (1)
- FoodbookApp.App/Localization/ProfilePageResources.Designer.cs: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| private void DebounceApplySearch() | ||
| { | ||
| var addRow = new HorizontalStackLayout | ||
| { | ||
| Spacing = 12, | ||
| Padding = new Thickness(4, 0, 4, 8), | ||
| VerticalOptions = LayoutOptions.Center | ||
| }; | ||
| _searchDebounceCts?.Cancel(); | ||
| _searchDebounceCts?.Dispose(); | ||
| _searchDebounceCts = new CancellationTokenSource(); | ||
| var token = _searchDebounceCts.Token; | ||
|
|
||
| var plusButton = new Button | ||
| { | ||
| Text = "+", | ||
| WidthRequest = 36, | ||
| HeightRequest = 36, | ||
| CornerRadius = 18, | ||
| Padding = 0, | ||
| VerticalOptions = LayoutOptions.Center | ||
| }; | ||
| plusButton.StyleClass = new[] { "Secondary" }; | ||
| plusButton.Clicked += async (_, __) => await OnAddIngredientAsync(); | ||
|
|
||
| var textLabel = new Label | ||
| _ = Task.Run(async () => | ||
| { | ||
| Text = "Dodaj składnik", | ||
| FontSize = 15, | ||
| VerticalOptions = LayoutOptions.Center | ||
| }; | ||
| textLabel.SetAppThemeColor(Label.TextColorProperty, | ||
| (Color)Application.Current!.Resources["PrimaryText"], | ||
| (Color)Application.Current!.Resources["PrimaryText"]); | ||
|
|
||
| var tap = new TapGestureRecognizer(); | ||
| tap.Tapped += async (_, __) => await OnAddIngredientAsync(); | ||
| textLabel.GestureRecognizers.Add(tap); | ||
|
|
||
| addRow.Children.Add(plusButton); | ||
| addRow.Children.Add(textLabel); | ||
|
|
||
| return addRow; | ||
| try | ||
| { | ||
| await Task.Delay(200, token); | ||
| if (token.IsCancellationRequested) | ||
| return; | ||
|
|
||
| await MainThread.InvokeOnMainThreadAsync(() => | ||
| { | ||
| if (!token.IsCancellationRequested) | ||
| ApplySearchCore(); | ||
| }); | ||
| } | ||
| catch (OperationCanceledException) | ||
| { | ||
| } | ||
| }, token); | ||
| } |
There was a problem hiding this comment.
The debounce CancellationTokenSource is never cleaned up when the popup is closed. If the user closes the popup quickly, the background debounce task can still run and attempt to update FilteredItems, and the CTS will remain undisposed. Cancel+dispose _searchDebounceCts in CloseWithResultAsync and/or hook the popup Closed event (or override the appropriate lifecycle method) to ensure cleanup.
|
|
||
| InitializeComponent(); | ||
| BindingContext = this; | ||
| DetectContext(); |
There was a problem hiding this comment.
The selected constructor parameter is no longer used. Either remove it (and update call sites) or use it to initialize state (e.g., scroll to / visually indicate the initially selected item) to avoid confusion and dead parameters.
| DetectContext(); | |
| DetectContext(); | |
| if (!string.IsNullOrWhiteSpace(selected)) | |
| { | |
| _searchText = selected; | |
| OnPropertyChanged(nameof(SearchText)); | |
| } |
| app.Resources["PopupBackgroundBrush"] = _originalPopupBackgroundBrushResource; | ||
| _originalPopupBackgroundBrushResource = null; | ||
| } | ||
|
|
There was a problem hiding this comment.
ApplyModalPopupOverlay() hides the underlying Shell page by setting its Opacity = 0, but RestoreModalPopupOverlay() never calls RestoreUnderlyingContent(). This can leave the underlying page invisible after the popup closes while AddRecipePage remains open modally. Call RestoreUnderlyingContent() when restoring the popup overlay (or otherwise ensure opacity is reset when isOpen == false).
| RestoreUnderlyingContent(); |
| Margin="16,12" | ||
| SelectionMode="None" | ||
| ItemsSource="{Binding VisibleItems, Source={x:Reference ThisPopup}}" | ||
| ItemSizingStrategy="MeasureFirstItem"> |
There was a problem hiding this comment.
ItemSizingStrategy="MeasureFirstItem" assumes all rows have the same height. SimpleListPopup renders multiple item types (headers, macro rows, meal preview blocks with ingredient lists) whose heights can vary, so this strategy can cause clipping/overlap or excessive whitespace. Consider removing ItemSizingStrategy or switching to MeasureAllItems (or a fixed item template height if the content is truly uniform).
| ItemSizingStrategy="MeasureFirstItem"> | |
| ItemSizingStrategy="MeasureAllItems"> |
No description provided.