Implement Android UI using Material Design components with MAUI and MvvmCross#100
Conversation
…droid : Buform.Example.Maui; Buform.Maui; Set CommunityToolkit.Maui to v.9.0.1; Set MvvmCross to v.9.3.1; Fix Android.App/CreateMauiApp; Add Platforms\Android\FormViewHandler.cs;
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughProjects were updated to multi-target 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
Buform.Example.Maui/Platforms/Android/MainApplication.cs (1)
6-7: 🛠️ Refactor suggestion | 🟠 MajorMark the class as
sealed.
MainApplicationis not designed for inheritance, contains no virtual/abstract members, and is not inherited elsewhere. As per coding guidelines, "Require marking C# classes as sealed if they are not designed for inheritance."Proposed fix
[Application] -public class MainApplication : MauiApplication +public sealed class MainApplication : MauiApplicationBuform.Example/MenuViewModel.cs (3)
7-7: 🛠️ Refactor suggestion | 🟠 MajorMark the class as sealed.
The class
MenuViewModelshould be marked assealedas it contains no virtual or abstract members and is not designed for inheritance. As per coding guidelines, C# classes must be marked as sealed if they are not designed for inheritance.🔒 Proposed fix to mark the class as sealed
-public partial class MenuViewModel : ObservableObject +public sealed partial class MenuViewModel : ObservableObject
62-62:⚠️ Potential issue | 🔴 CriticalAdd a single newline at the end of the file.
The file does not end with a single newline character, causing a pipeline formatting failure. Ensure the file ends with exactly one newline after the closing brace.
52-54:⚠️ Potential issue | 🟠 MajorRemove unused
cancellationTokenparameter.The
CreateEventAsyncmethod declares acancellationTokenparameter but does not use it. TheINavigationService.OpenCreateEventAsync()method does not support cancellation tokens, so the parameter cannot be passed through. Remove the unused parameter from the method signature.
🧹 Nitpick comments (3)
Buform.Example/MenuViewModel.cs (1)
18-42: Consider extracting UI string literals to constants.The constructor contains several hardcoded string literals for form labels ("Gallery", "Show All Components", "Examples", etc.). Whilst these might be considered one-off UI text, extracting them to named constants would improve maintainability and support future localisation.
♻️ Example refactor to extract string constants
Add constants at the class level:
private const string GalleryGroupTitle = "Gallery"; private const string ShowAllComponentsLabel = "Show All Components"; private const string ExamplesGroupTitle = "Examples"; private const string ExamplesGroupFooter = "Contains some real-life examples."; private const string SetupConnectionLabel = "Setup New Connection"; private const string CreateEventLabel = "Create New Event";Then update the constructor:
- new TextFormGroup("Gallery") + new TextFormGroup(GalleryGroupTitle) { new ButtonFormItem(ShowControlsCommand) { - Label = "Show All Components", + Label = ShowAllComponentsLabel, InputType = ButtonInputType.Done } }, - new TextFormGroup("Examples", "Contains some real-life examples.") + new TextFormGroup(ExamplesGroupTitle, ExamplesGroupFooter) { new ButtonFormItem(CreateConnectionCommand) { - Label = "Setup New Connection", + Label = SetupConnectionLabel, InputType = ButtonInputType.Done }, new ButtonFormItem(CreateEventCommand) { - Label = "Create New Event", + Label = CreateEventLabel, InputType = ButtonInputType.Done } }Buform.Maui/Platforms/Android/MauiFormRecyclerAdapter.cs (1)
117-127: Centralise the header/footer reflection keys.
"HeaderLabel"and"FooterLabel"are domain keys used here and again in the header/footer view holder. Keeping them inline on both sides makes this path easy to break during a rename. Please extract shared constants or a small helper.As per coding guidelines, "Avoid hardcoded string literals in code logic. Any repeated or domain-relevant string (e.g. keys, identifiers, status values, error codes, comparison values) must be extracted into clearly named constants or static readonly fields."
Buform.Maui/Platforms/Android/FormAdapterItem.cs (1)
12-29: Make the adapter payload non-nullable.All factory methods take non-null inputs, so exposing
Itemas nullable just weakens the contract and pushes an unnecessary null path intoOnBindViewHolder. Tighten this toobjectand guard once in the constructor.Suggested fix
- public object? Item { get; } + public object Item { get; } - private FormAdapterItem(FormAdapterItemKind kind, object? item) + private FormAdapterItem(FormAdapterItemKind kind, object item) { + ArgumentNullException.ThrowIfNull(item); Kind = kind; Item = item; }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a83bd230-495b-4309-88e4-1198dad44595
📒 Files selected for processing (10)
Buform.Example.Maui/Buform.Example.Maui.csprojBuform.Example.Maui/Platforms/Android/MainApplication.csBuform.Example/MenuViewModel.csBuform.Maui/Buform.Maui.csprojBuform.Maui/Platforms/Android/FormAdapterItem.csBuform.Maui/Platforms/Android/FormViewHandler.csBuform.Maui/Platforms/Android/MauiFormHeaderFooterViewHolder.csBuform.Maui/Platforms/Android/MauiFormItemViewHolder.csBuform.Maui/Platforms/Android/MauiFormRecyclerAdapter.csBuform.MvvmCross/Buform.MvvmCross.csproj
Update Xamarin.AndroidX.Collection from v1.5.0.4 to v1.5.0.5; Update Xamarin.AndroidX.Collection.Ktx from v1.5.0.4 to v1.5.0.5; Update Xamarin.AndroidX.Fragment.Ktx from v1.8.6.1 to v 1.8.9.2; Update Humanizer.Core from v3.0.8 to v3.0.10 Add Xamarin.AndroidX.Collection v1.5.0.5; Add Xamarin.Google.Android.Material v1.12.0.4
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| BestPractice | 1 medium |
| ErrorProne | 1 medium |
🟢 Metrics 654 complexity · 19 duplication
Metric Results Complexity 654 Duplication 19
TIP This summary will be updated as you push new changes. Give us feedback
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Buform/Platforms/Android/Items/Button/ButtonFormViewHolder.cs (1)
136-176: 🛠️ Refactor suggestion | 🟠 MajorKeep
Disposeas the last method in the class.Line 136 introduces
Dispose, but helper methods continue after it. Please move cleanup methods to the end and place helpers above.As per coding guidelines, "Enforce consistent member ordering in C# types: order members as follows – static, const, properties, constructors, methods. Within each group, order by access level: private, protected, then public. Always place Dispose() and related cleanup methods at the end of the class."
♻️ Duplicate comments (7)
.github/workflows/ci.yml (1)
22-23:⚠️ Potential issue | 🟠 MajorHarden Xcode selection to prevent runner-image breakage.
Using a fixed absolute path on
macos-latestcan fail when the hosted image changes, causing avoidable CI outages.Proposed hardening
- - name: Setup Xcode 26.2 (required for .NET iOS build) - run: sudo xcode-select -s /Applications/Xcode_26.2.app + - name: Setup Xcode 26.2 (required for .NET iOS build) + run: | + if [ -d "/Applications/Xcode_26.2.app" ]; then + sudo xcode-select -s /Applications/Xcode_26.2.app + else + echo "Xcode 26.2 not found on this runner image" >&2 + ls -1 /Applications | grep Xcode || true + exit 1 + fiBuform/Platforms/Android/Items/DateTime/ContextExtensions.cs (1)
12-26:⚠️ Potential issue | 🟡 MinorAdd a loop guard when unwrapping
ContextWrapper.The
while (true)pattern can loop indefinitely if a wrapper chain is cyclic or ifBaseContextreturns the same instance. Add a break guard whenBaseContextis null or references the same instance.Suggested fix
while (true) { if (current is FragmentActivity fragmentActivity) { return fragmentActivity; } if (current is ContextWrapper wrapper) { - current = wrapper.BaseContext!; + var baseContext = wrapper.BaseContext; + if (baseContext is null || ReferenceEquals(baseContext, current)) + { + break; + } + + current = baseContext; continue; } break; }Buform/Resources/layout/PickerDialogLayout.xml (1)
89-89:⚠️ Potential issue | 🟡 MinorUse a string resource for the search hint.
Line 89 still hardcodes
"Search"; please move it to@string/...for localisation consistency.Buform/Platforms/Android/Items/Slider/SliderFormViewHolder.cs (1)
20-25:⚠️ Potential issue | 🟠 MajorReplace the touch-event subscription with the slider's proper value-change listener.
The Material Slider API provides
AddOnChangeListenerwith anonValueChangecallback that includes afromUserboolean. Subscribe to this instead ofView.Touch(line 24). This captures all user input—dragging, keyboard, and accessibility—and only fires when the slider value is committed. CheckfromUserto persist only user-initiated updates, avoiding programmatic feedback loops without needing manual flags.Note: Due to Xamarin binding limitations with
BaseSlider, you may need to use reflection to invokeaddOnChangeListenerand implementIBaseOnChangeListener.Buform/Platforms/Android/Items/Picker/PickerDialogFragment.cs (1)
14-15:⚠️ Potential issue | 🟠 MajorPersist the picker item state to survive fragment recreation.
_itemis stored only as an instance field without being persisted toArgumentsorSaveInstanceState. WhenDialogFragmentis recreated by Android (after rotation or process death), the new instance initialises with_item == null, causing all setup methods to silently exit early and the dialog to render without content or callbacks.Consider storing a stable identifier in
Argumentsbundle duringCreate()and restoring the item reference inOnViewCreated()by re-resolving it from a presenter, parent fragment, or activity.Also applies to: 25-28
Buform/Platforms/Android/Items/DateTime/DateTimeFormViewHolder.cs (2)
125-141:⚠️ Potential issue | 🟠 MajorImplement separate picker flows for
DateTimeInputType.TimeandDateTimeInputType.DateTime.The rendering correctly switches on
InputType(lines 125–129) to display formatted values, but the interaction always opens a date picker regardless of type. This prevents editing:
- Time-only items: clicking displays a date picker when only time should be editable
- Date-time items: time components are preserved from the existing value; only the date can be changed
Implement a time picker for
DateTimeInputType.Timeand a date-time picker (or sequential date and time pickers) forDateTimeInputType.DateTimein theOnClickmethod to match the rendering logic.
151-159:⚠️ Potential issue | 🔴 CriticalMaterialDatePicker converts date values through UTC epoch, not local time.
MaterialDatePickeroperates on UTC epoch milliseconds representing 00:00:00 UTC on a given calendar date. The current implementation incorrectly converts fullDateTimevalues (including time components) through local timezone boundaries, causing off-by-one date errors.The bug manifests in both directions:
ToUtcMillis(currentValue)converts the full local timestamp to UTC, then to epoch. For users west of UTC, this shifts the date forward by one day in the picker.FromUtcMillis(selectedMillis.LongValue()).Dateconverts the UTC epoch back through the local timezone, shifting the date backward.🐛 Suggested fix
- private static long ToUtcMillis(DateTime dateTime) + private static long ToPickerUtcDateMillis(DateTime dateTime) { - var utc = dateTime.Kind switch - { - DateTimeKind.Utc => dateTime, - DateTimeKind.Local => dateTime.ToUniversalTime(), - _ => DateTime.SpecifyKind(dateTime, DateTimeKind.Local).ToUniversalTime() - }; - - return new DateTimeOffset(utc).ToUnixTimeMilliseconds(); + var utcDate = DateTime.SpecifyKind(dateTime.Date, DateTimeKind.Utc); + return new DateTimeOffset(utcDate).ToUnixTimeMilliseconds(); } - private static DateTime FromUtcMillis(long millis) + private static DateTime FromPickerUtcDateMillis(long millis) { - return DateTimeOffset.FromUnixTimeMilliseconds(millis).LocalDateTime; + return DateTimeOffset.FromUnixTimeMilliseconds(millis).UtcDateTime.Date; }Also applies to: 176-189, 243-257
🧹 Nitpick comments (23)
Buform/Resources/drawable/bg_stepper_capsule.xml (1)
4-4: Prefer a colour resource over an inline hex value.Line 4 hardcodes the background colour, which makes theme/day-night customisation harder. Please move this to
@color/...and provide day/night overrides if needed.Suggested change
- <solid android:color="#E5E5EA" /> + <solid android:color="@color/stepper_capsule_background" />Buform/Resources/drawable/ic_stepper_minus.xml (1)
8-8: Use a theme-aware colour resource for the vector path.Line 8 hardcodes black; consider
@color/...(or view tinting) so the icon adapts better across themes.Suggested change
- android:fillColor="#000000" + android:fillColor="@color/stepper_icon_color"Buform/Resources/layout/FormItemDateTimeLayout.xml (1)
22-22: Consider extracting hardcoded dimension to a resource.The
6dpbottom margin is hardcoded whilst other spacing values use dimension resources (@dimen/form_horizontal_padding, etc.). For consistency and easier maintenance, consider extracting this to a dimension resource.- android:layout_marginBottom="6dp" + android:layout_marginBottom="@dimen/label_bottom_margin"Buform/Resources/layout/PickerOptionItemLayout.xml (1)
21-21: Consider using a Material Design icon instead of platform drawable.
@android:drawable/checkbox_on_backgroundis a platform drawable that may appear inconsistent across different Android versions and manufacturer skins. Consider using a Material icon (e.g., from@drawable/ic_checkor a vector drawable) for consistent appearance.Buform/Resources/layout/FormItemTextLayout.xml (1)
23-23: Use a dimension resource for vertical padding.Line 23 hardcodes
20dpwhile adjacent spacing is resource-based. Please extract this to@dimen/...to keep spacing rules consistent.Buform/Resources/layout/FormItemSwitchLayout.xml (1)
7-7: Reference shared height dimen instead of inline56dp.Line 7 can use
@dimen/form_item_heightto align with the new shared sizing tokens and avoid drift.Buform/Resources/layout/PickerDialogLayout.xml (1)
79-80: Remove redundant namespace declarations inTextInputLayout.
xmlns:androidandxmlns:appare already declared on the root node, so repeating them here is unnecessary.Buform/Platforms/Android/Items/Button/ButtonFormViewHolder.cs (1)
101-175: Use descriptive names in helper methods.Please rename abbreviated identifiers like
lp,attr, andato explicit names (layoutParameters,attribute,alphaChannel) for readability.As per coding guidelines, "Prefer full, descriptive names for variables and class names. Avoid abbreviations like ct, cts, cfg, or svc. Use complete names such as cancellationToken, taskCompletionSource, configuration, service."
Buform/Platforms/Android/Items/Picker/MultiValuePickerFormViewHolder.cs (1)
20-35: Consider consolidating duplicate switch arms.The
Default,Dialog, andPopUpcases all return identicalDialogFragmentPickerPresenterinstances. This could be simplified using combined case patterns.♻️ Proposed refactor
return inputType switch { - PickerInputType.Default - => new DialogFragmentPickerPresenter<IMultiValuePickerFormItem>( - CreatePickerDialogFragment - ), - PickerInputType.Dialog - => new DialogFragmentPickerPresenter<IMultiValuePickerFormItem>( - CreatePickerDialogFragment - ), - PickerInputType.PopUp + PickerInputType.Default or PickerInputType.Dialog or PickerInputType.PopUp => new DialogFragmentPickerPresenter<IMultiValuePickerFormItem>( CreatePickerDialogFragment ), _ => null };Buform/Resources/layout/AsyncPickerDialogLayout.xml (2)
79-93: Remove redundant namespace declarations.Lines 81-82 redeclare
xmlns:androidandxmlns:appwhich are already declared on the rootLinearLayoutelement (lines 3-4). These redundant declarations should be removed.♻️ Proposed fix
<com.google.android.material.textfield.TextInputLayout android:id="@+id/SearchLayout" - xmlns:android="http://schemas.android.com/apk/res/android" - xmlns:app="http://schemas.android.com/apk/res-auto" android:layout_width="match_parent" android:layout_height="wrap_content" app:boxBackgroundMode="filled" app:hintEnabled="false">
43-43: Hardcoded ripple colour should use a theme attribute or colour resource.The ripple colour
#4A4458is hardcoded inline. For consistency and theme support, consider extracting this to a colour resource.Buform.Maui/Platforms/Android/MauiFormHeaderFooterViewHolder.cs (1)
67-83: Consider extracting dimension constants.The hardcoded padding and text size values (16, 24, 8, 4 for padding; 14, 12 for text size) are scattered through the switch cases. Per coding guidelines, consider extracting these to named constants for maintainability.
♻️ Example constants
private const int PaddingHorizontal = 16; private const int HeaderPaddingTop = 24; private const int HeaderPaddingBottom = 8; private const int FooterPaddingTop = 4; private const int FooterPaddingBottom = 16; private const float HeaderTextSizeSp = 14f; private const float FooterTextSizeSp = 12f;As per coding guidelines: "Avoid hardcoding format strings directly in code... Move formatting patterns to application-level constants, helpers, or localisation resources."
Buform.Maui/FormReflectionHelper.cs (1)
17-18: Consider caching property lookups for frequently accessed types.The reflection-based property lookup (
GetType().GetProperty(...)) is performed on every call. If this helper is invoked frequently for the same types, consider cachingPropertyInfoobjects per type to improve performance.♻️ Example with caching
private static readonly ConcurrentDictionary<(Type, string), PropertyInfo?> PropertyCache = new(); private static string? GetStringProperty(object item, string propertyName) { var key = (item.GetType(), propertyName); var property = PropertyCache.GetOrAdd(key, k => k.Item1.GetProperty(k.Item2)); return property?.GetValue(item) as string; }Buform/Resources/layout/FormItemStepperLayout.xml (2)
45-49: Extract hardcoded divider colour to a resource.The divider background colour
#D1D1D6is hardcoded inline. For consistency and theme support, extract this to a colour resource.♻️ Proposed fix
In
values/colors.xml:<color name="stepper_divider">#D1D1D6</color>In the layout:
<View android:layout_width="1dp" android:layout_height="24dp" android:layout_gravity="center_vertical" - android:background="#D1D1D6" /> + android:background="@color/stepper_divider" />As per coding guidelines: "Avoid hardcoded string literals in code logic. Any repeated or domain-relevant string... must be extracted into clearly named constants."
34-44: Useapp:tintinstead ofandroid:tintfor AppCompat consistency.Since the project uses AppCompat (as evidenced by
androidx.appcompat.widget.SwitchCompat), preferapp:tintonImageButtonfor consistent behaviour across all API levels. Whileandroid:tintis supported natively from API 21+, AppCompat provides better compatibility throughapp:tint.♻️ Proposed fix
<ImageButton android:id="@+id/MinusButton" android:layout_width="0dp" android:layout_height="match_parent" android:layout_weight="1" android:background="?attr/selectableItemBackgroundBorderless" android:contentDescription="@string/decrease" android:padding="0dp" android:scaleType="center" android:src="@drawable/ic_stepper_minus" - android:tint="@android:color/black" /> + app:tint="@android:color/black" />Buform/Platforms/Android/Items/Picker/PickerDialogFragment.cs (1)
230-232: Consider a more readable approach for checking option count.Using
Skip(1).Any()to check if there's more than one option works but is less immediately clear than alternatives.♻️ Optional refactor
- _searchLayout.Visibility = _item.Options.Skip(1).Any() + _searchLayout.Visibility = _item.Options.Count() > 1 ? ViewStates.Visible : ViewStates.Gone;Buform/Platforms/Android/Items/Text/TextInputFormViewHolder.cs (1)
338-363: Consider moving the extension class to a separate file.
TextInputTypeExtensionsis a separate public static class that could be placed in its own file for better organisation and discoverability. However, given it's closely related to this view holder's functionality, keeping it here is acceptable.Buform/Platforms/Android/Items/DateTime/DateTimeFormViewHolder.cs (2)
56-64: MoveDisposemethod to the end of the class.Per coding guidelines,
Dispose()and related cleanup methods should be placed at the end of the class, before any nested types.As per coding guidelines: "Enforce consistent member ordering in C# types... Always place Dispose() and related cleanup methods at the end of the class."
193-193: Extract hardcoded string literal to a constant.The
"DATE_PICKER"tag should be extracted to a named constant for maintainability.♻️ Proposed fix
+ private const string DatePickerTag = "DATE_PICKER"; + // ... in ShowDatePicker method: - picker.Show(activity.SupportFragmentManager, "DATE_PICKER"); + picker.Show(activity.SupportFragmentManager, DatePickerTag);As per coding guidelines: "Avoid hardcoded string literals in code logic. Any repeated or domain-relevant string... must be extracted into clearly named constants."
Buform/Platforms/Android/Items/Text/TextFormViewHolder.cs (1)
83-110: MoveDispose()to the end of the class.There is still a private helper below it, so this type no longer follows the repository’s C# member-ordering rule.
As per coding guidelines, "Enforce consistent member ordering in C# types: order members as follows – static, const, properties, constructors, methods. Within each group, order by access level: private, protected, then public. Always place Dispose() and related cleanup methods at the end of the class."
Buform/Platforms/Android/Items/Segments/SegmentsFormViewHolder.cs (1)
235-268: KeepDispose()as the final member.
ButtonCheckedListenerstill sits belowDispose(), which breaks the member ordering used for C# types in this repository.As per coding guidelines, "Enforce consistent member ordering in C# types: order members as follows – static, const, properties, constructors, methods. Within each group, order by access level: private, protected, then public. Always place Dispose() and related cleanup methods at the end of the class."
Buform.Maui/Platforms/Android/MauiFormRecyclerAdapter.cs (1)
128-336: PlaceDispose()after the helper methods.This class still has several private methods below
Dispose(), so its layout breaks the repository’s C# member-ordering rule.As per coding guidelines, "Enforce consistent member ordering in C# types: order members as follows – static, const, properties, constructors, methods. Within each group, order by access level: private, protected, then public. Always place Dispose() and related cleanup methods at the end of the class."
Buform/Platforms/Android/Items/Picker/PresentedPickerFormViewHolderBase.cs (1)
73-83: Consider combining identical switch cases.
PickerInputType.DefaultandPickerInputType.Dialogproduce the same presenter. Combining them reduces duplication and makes the intent clearer.♻️ Proposed simplification
protected virtual PickerPresenterBase<TFormItem>? GetPickerPresenter(PickerInputType inputType) { return inputType switch { - PickerInputType.Default - => new DialogFragmentPickerPresenter<TFormItem>(CreatePickerDialogFragment), - PickerInputType.Dialog + PickerInputType.Default or PickerInputType.Dialog => new DialogFragmentPickerPresenter<TFormItem>(CreatePickerDialogFragment), _ => null }; }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 09e97856-48ba-45b1-aed4-ea54074cb111
📒 Files selected for processing (67)
.github/workflows/ci.ymlBuform.Example.Droid/Buform.Example.Droid.csprojBuform.Example.Maui/Buform.Example.Maui.csprojBuform.Example.Maui/Platforms/Android/MainApplication.csBuform.Example.Maui/RandomNumberGeneratorView.xamlBuform.Example/Buform.Example.csprojBuform.Example/MenuViewModel.csBuform.Maui/Buform.Maui.csprojBuform.Maui/FormReflectionHelper.csBuform.Maui/FormReflectionKeys.csBuform.Maui/Platforms/Android/FormAdapterItem.csBuform.Maui/Platforms/Android/FormViewHandler.csBuform.Maui/Platforms/Android/MauiFormHeaderFooterViewHolder.csBuform.Maui/Platforms/Android/MauiFormItemViewHolder.csBuform.Maui/Platforms/Android/MauiFormRecyclerAdapter.csBuform.Maui/Platforms/Android/MauiFormViewHolderBase.csBuform.MvvmCross/Buform.MvvmCross.csprojBuform/Buform.csprojBuform/Platforms/Android/Items/Button/ButtonFormViewHolder.csBuform/Platforms/Android/Items/DateTime/ContextExtensions.csBuform/Platforms/Android/Items/DateTime/DateTimeFormItemComponent.csBuform/Platforms/Android/Items/DateTime/DateTimeFormViewHolder.csBuform/Platforms/Android/Items/Picker/AsyncPickerDialogFragment.csBuform/Platforms/Android/Items/Picker/AsyncPickerFormViewHolder.csBuform/Platforms/Android/Items/Picker/CallbackPickerFormViewHolder.csBuform/Platforms/Android/Items/Picker/DialogFragmentPickerPresenter.csBuform/Platforms/Android/Items/Picker/MultiValuePickerFormViewHolder.csBuform/Platforms/Android/Items/Picker/PickerDialogFragment.csBuform/Platforms/Android/Items/Picker/PickerFormItemComponent.csBuform/Platforms/Android/Items/Picker/PickerFormViewHolderBase.csBuform/Platforms/Android/Items/Picker/PickerOptionViewHolder.csBuform/Platforms/Android/Items/Picker/PickerOptionsAdapter.csBuform/Platforms/Android/Items/Picker/PickerPresenterBase.csBuform/Platforms/Android/Items/Picker/PopUpPickerFormViewHolder.csBuform/Platforms/Android/Items/Picker/PresentedPickerFormViewHolderBase.csBuform/Platforms/Android/Items/Segments/SegmentsFormItemComponent.csBuform/Platforms/Android/Items/Segments/SegmentsFormViewHolder.csBuform/Platforms/Android/Items/Slider/SliderFormItemComponent.csBuform/Platforms/Android/Items/Slider/SliderFormViewHolder.csBuform/Platforms/Android/Items/Stepper/StepperFormItemComponent.csBuform/Platforms/Android/Items/Stepper/StepperFormViewHolder.csBuform/Platforms/Android/Items/Switch/SwitchFormItemComponent.csBuform/Platforms/Android/Items/Switch/SwitchFormViewHolder.csBuform/Platforms/Android/Items/Text/TextFormItemComponent.csBuform/Platforms/Android/Items/Text/TextFormViewHolder.csBuform/Platforms/Android/Items/Text/TextInputFormViewHolder.csBuform/Platforms/Android/Items/Text/TextMultilineFormViewHolder.csBuform/Resources/drawable/bg_stepper_capsule.xmlBuform/Resources/drawable/ic_stepper_minus.xmlBuform/Resources/drawable/ic_stepper_plus.xmlBuform/Resources/layout/AsyncPickerDialogLayout.xmlBuform/Resources/layout/FormGroupTextFooterLayout.xmlBuform/Resources/layout/FormGroupTextHeaderLayout.xmlBuform/Resources/layout/FormItemButtonLayout.xmlBuform/Resources/layout/FormItemDateTimeLayout.xmlBuform/Resources/layout/FormItemPickerLayout.xmlBuform/Resources/layout/FormItemSegmentsLayout.xmlBuform/Resources/layout/FormItemSliderLayout.xmlBuform/Resources/layout/FormItemStepperLayout.xmlBuform/Resources/layout/FormItemSwitchLayout.xmlBuform/Resources/layout/FormItemTextInputLayout.xmlBuform/Resources/layout/FormItemTextLayout.xmlBuform/Resources/layout/FormItemTextMultilineLayout.xmlBuform/Resources/layout/PickerDialogLayout.xmlBuform/Resources/layout/PickerOptionItemLayout.xmlBuform/Resources/values/dimens.xmlBuform/Resources/values/strings.xml
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (5)
Buform/Resources/layout/FormItemButtonLayout.xml (1)
16-17:⚠️ Potential issue | 🟠 MajorVerify the compact button height still meets the 48dp touch target minimum.
Using
@dimen/form_item_height_compactfor both height fields can reintroduce an accessibility blocker if that dimen is under 48dp.#!/bin/bash set -euo pipefail # Find where form_item_height_compact is defined and inspect its value. fd -i 'dimens*.xml' | xargs -r rg -n --no-heading '<dimen name="form_item_height_compact">[^<]+</dimen>' # Optional: show related touch-target dimensions for comparison. fd -i 'dimens*.xml' | xargs -r rg -n --no-heading 'touch|min_height|minHeight|48dp'Expected result:
form_item_height_compactresolves to at least48dp, orandroid:minHeightis set to48dphere.Buform/Platforms/Android/Items/Picker/PickerDialogFragment.cs (1)
14-28:⚠️ Potential issue | 🟠 MajorPersist the picker state instead of storing it only in
_item.
Create()only initialises_itemon the fragment instance. After configuration change or process recreation, Android can rebuildPickerDialogFragmentwith_item == null, and thenBindItem(),SetupRecyclerView(), and the button setup all degrade to no-ops. Persist a stable identifier inArgumentsand rehydrate the picker during fragment creation instead of relying on the field.Buform/Platforms/Android/Items/Slider/SliderFormViewHolder.cs (1)
24-25:⚠️ Potential issue | 🟠 MajorPrefer the slider change listener over
View.Touch.
Touchfires for every motion event and misses keyboard/accessibility adjustments, so this write-back path is both noisy and incomplete. If1.12.0.4exposesAddOnChangeListener, bind that instead and use its user-origin flag when updatingData.Value.In Xamarin.Google.Android.Material 1.12.0.4, what is the recommended C# API for observing Google.Android.Material.Slider.Slider value changes? Does the binding expose AddOnChangeListener / IOnChangeListener, and does it report whether the change came from the user?Also applies to: 140-152
Buform/Platforms/Android/Items/DateTime/DateTimeFormViewHolder.cs (2)
133-141:⚠️ Potential issue | 🟠 MajorBranch the click flow by
InputType.Time-only items cannot be edited correctly here, and
DateTimeitems never let the user change the time component. The interaction should match the rendering switch inGetDisplayValue().
152-159:⚠️ Potential issue | 🔴 CriticalMaterialDatePicker dates should not round-trip through local-time conversion.
These helpers convert full
DateTimevalues to and from local timestamps, which shifts the calendar date around timezone boundaries.MaterialDatePickerexpects a date-only UTC selection value, so bothSetSelection(...)and the positive-button readback path need date-only conversion instead.How does Google MaterialDatePicker represent selected dates for Builder.DatePicker().SetSelection(...) and the positive-button callback? Are the values UTC-midnight milliseconds for the chosen calendar date rather than local-time timestamps?Also applies to: 176-189, 243-257
🧹 Nitpick comments (12)
Buform/Resources/layout/PickerDialogLayout.xml (1)
43-43: Use theme/resource-driven ripple colours instead of hex literals.Inline hex ripple colours make theme adaptation brittle. Prefer an attribute or colour resource.
Proposed fix
- app:rippleColor="#22000000" + app:rippleColor="?attr/colorControlHighlight" @@ - app:rippleColor="#22000000" + app:rippleColor="?attr/colorControlHighlight"Also applies to: 66-66
Buform.Maui/FormReflectionHelper.cs (2)
5-20: Reorder methods to match the repository’s C# member-order convention.The private helper should be placed before public methods in this class.
As per coding guidelines, "Enforce consistent member ordering in C# types: order members as follows – static, const, properties, constructors, methods. Within each group, order by access level: private, protected, then public. Always place Dispose() and related cleanup methods at the end of the class."
17-20: Cache reflection lookups to avoid repeated property discovery overhead.
GetType().GetProperty(...)on every call can become expensive in list binding flows.Proposed fix
+using System.Collections.Concurrent; +using System.Reflection; + internal static class FormReflectionHelper { + private static readonly ConcurrentDictionary<(Type Type, string PropertyName), PropertyInfo?> PropertyCache = new(); + @@ private static string? GetStringProperty(object item, string propertyName) { ArgumentNullException.ThrowIfNull(item); - return item.GetType().GetProperty(propertyName)?.GetValue(item) as string; + var type = item.GetType(); + var propertyInfo = PropertyCache.GetOrAdd((type, propertyName), static key => key.Type.GetProperty(key.PropertyName)); + return propertyInfo?.GetValue(item) as string; } }Buform/Resources/drawable/bg_stepper_capsule.xml (1)
4-4: Prefer a colour resource token over a hardcoded hex value.Using a resource here improves theme consistency and future palette changes.
Proposed fix
- <solid android:color="#E5E5EA" /> + <solid android:color="@color/stepper_capsule_background" />Buform/Resources/layout/FormItemTextLayout.xml (1)
22-23: Consider moving hardcoded text padding to a dimen resource.On Line 23,
android:paddingVertical="20dp"is a literal that may drift from global spacing rules; extracting it to@dimen/...would improve consistency and easier theming.Proposed refactor
- android:paddingVertical="20dp" + android:paddingVertical="@dimen/form_item_text_vertical_padding"Buform/Resources/layout/FormItemSwitchLayout.xml (1)
7-7: Inconsistent hardcodedminHeightvalue.This layout uses a hardcoded
56dpforminHeight, whileFormItemPickerLayout.xmluses@dimen/form_item_heightfor the same purpose. Consider using the dimension resource for consistency and easier maintenance.Suggested fix
- android:minHeight="56dp" + android:minHeight="@dimen/form_item_height"Buform/Platforms/Android/Items/Picker/MultiValuePickerFormViewHolder.cs (1)
20-35: Consolidate duplicate switch arms.The
Default,Dialog, andPopUpcases all create identicalDialogFragmentPickerPresenterinstances. Consider combining them to reduce duplication.Suggested refactor
return inputType switch { - PickerInputType.Default - => new DialogFragmentPickerPresenter<IMultiValuePickerFormItem>( - CreatePickerDialogFragment - ), - PickerInputType.Dialog - => new DialogFragmentPickerPresenter<IMultiValuePickerFormItem>( - CreatePickerDialogFragment - ), - PickerInputType.PopUp + PickerInputType.Default or PickerInputType.Dialog or PickerInputType.PopUp => new DialogFragmentPickerPresenter<IMultiValuePickerFormItem>( CreatePickerDialogFragment ), _ => null };Buform/Platforms/Android/Items/Picker/PickerOptionsAdapter.cs (1)
12-27: MoveItemCountabove the constructor.
ItemCountis this type’s only property, so it should appear before the constructor to stay aligned with the repo’s C# member ordering rule.As per coding guidelines, "Enforce consistent member ordering in C# types: order members as follows – static, const, properties, constructors, methods. Within each group, order by access level: private, protected, then public. Always place Dispose() and related cleanup methods at the end of the class."
Buform/Platforms/Android/Items/DateTime/DateTimeFormViewHolder.cs (1)
125-129: Extract these date/time format specifiers.
"d","t"and"g"are user-facing formatting rules; per repo guidance they should live in shared constants/helpers/resources rather than inline in the view holder.As per coding guidelines: "Avoid hardcoding format strings directly in code, especially for dates, text templates, or numeric formats. Move formatting patterns to application-level constants, helpers, or localisation resources."
Buform/Platforms/Android/Items/Text/TextInputFormViewHolder.cs (1)
195-208: Redundant type conversion used only for validation gating.The
TryConvertTextmethod parses the input and produces a typed value, but only the originaltextstring is passed toSetValue(). The conversion result is used solely to gate whetherSetValueis called. IfITextInputFormItem.SetValuealready performs its own validation/conversion internally (which the past review comment indicates), this pre-validation may be redundant and could prevent valid inputs from being accepted if parsing differs between the two implementations.Consider whether
TryConvertTextvalidation is truly necessary, or ifSetValueshould handle all validation.Buform/Platforms/Android/Items/Picker/PickerFormViewHolderBase.cs (2)
59-73: Consider using theme error colour instead of hardcodedColor.Red.For Material Design consistency, consider resolving the error colour from the theme (e.g.,
colorError) rather than using a hardcodedColor.Red. This would respect the app's theme and dark mode configurations.Suggested improvement
protected virtual void UpdateValidationErrorMessage(string? validationErrorMessage) { if (LabelView == null) { return; } var hasValidationError = !string.IsNullOrWhiteSpace(validationErrorMessage); LabelView.SetTextColor( !hasValidationError ? ResolveThemeColor(Android.Resource.Attribute.TextColorPrimary) - : Color.Red + : ResolveThemeColor(Resource.Attribute.colorError) ); }
75-90: Fallback colourColor.Blackmay not suit dark themes.If theme attribute resolution fails,
Color.Blackis returned as a fallback. This would be invisible on dark backgrounds. Consider using a neutral fallback or logging when resolution fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cf231f14-eea3-4999-adb7-7654abc2e02e
📒 Files selected for processing (67)
.github/workflows/ci.ymlBuform.Example.Droid/Buform.Example.Droid.csprojBuform.Example.Maui/Buform.Example.Maui.csprojBuform.Example.Maui/Platforms/Android/MainApplication.csBuform.Example.Maui/RandomNumberGeneratorView.xamlBuform.Example/Buform.Example.csprojBuform.Example/MenuViewModel.csBuform.Maui/Buform.Maui.csprojBuform.Maui/FormReflectionHelper.csBuform.Maui/FormReflectionKeys.csBuform.Maui/Platforms/Android/FormAdapterItem.csBuform.Maui/Platforms/Android/FormViewHandler.csBuform.Maui/Platforms/Android/MauiFormHeaderFooterViewHolder.csBuform.Maui/Platforms/Android/MauiFormItemViewHolder.csBuform.Maui/Platforms/Android/MauiFormRecyclerAdapter.csBuform.Maui/Platforms/Android/MauiFormViewHolderBase.csBuform.MvvmCross/Buform.MvvmCross.csprojBuform/Buform.csprojBuform/Platforms/Android/Items/Button/ButtonFormViewHolder.csBuform/Platforms/Android/Items/DateTime/ContextExtensions.csBuform/Platforms/Android/Items/DateTime/DateTimeFormItemComponent.csBuform/Platforms/Android/Items/DateTime/DateTimeFormViewHolder.csBuform/Platforms/Android/Items/Picker/AsyncPickerDialogFragment.csBuform/Platforms/Android/Items/Picker/AsyncPickerFormViewHolder.csBuform/Platforms/Android/Items/Picker/CallbackPickerFormViewHolder.csBuform/Platforms/Android/Items/Picker/DialogFragmentPickerPresenter.csBuform/Platforms/Android/Items/Picker/MultiValuePickerFormViewHolder.csBuform/Platforms/Android/Items/Picker/PickerDialogFragment.csBuform/Platforms/Android/Items/Picker/PickerFormItemComponent.csBuform/Platforms/Android/Items/Picker/PickerFormViewHolderBase.csBuform/Platforms/Android/Items/Picker/PickerOptionViewHolder.csBuform/Platforms/Android/Items/Picker/PickerOptionsAdapter.csBuform/Platforms/Android/Items/Picker/PickerPresenterBase.csBuform/Platforms/Android/Items/Picker/PopUpPickerFormViewHolder.csBuform/Platforms/Android/Items/Picker/PresentedPickerFormViewHolderBase.csBuform/Platforms/Android/Items/Segments/SegmentsFormItemComponent.csBuform/Platforms/Android/Items/Segments/SegmentsFormViewHolder.csBuform/Platforms/Android/Items/Slider/SliderFormItemComponent.csBuform/Platforms/Android/Items/Slider/SliderFormViewHolder.csBuform/Platforms/Android/Items/Stepper/StepperFormItemComponent.csBuform/Platforms/Android/Items/Stepper/StepperFormViewHolder.csBuform/Platforms/Android/Items/Switch/SwitchFormItemComponent.csBuform/Platforms/Android/Items/Switch/SwitchFormViewHolder.csBuform/Platforms/Android/Items/Text/TextFormItemComponent.csBuform/Platforms/Android/Items/Text/TextFormViewHolder.csBuform/Platforms/Android/Items/Text/TextInputFormViewHolder.csBuform/Platforms/Android/Items/Text/TextMultilineFormViewHolder.csBuform/Resources/drawable/bg_stepper_capsule.xmlBuform/Resources/drawable/ic_stepper_minus.xmlBuform/Resources/drawable/ic_stepper_plus.xmlBuform/Resources/layout/AsyncPickerDialogLayout.xmlBuform/Resources/layout/FormGroupTextFooterLayout.xmlBuform/Resources/layout/FormGroupTextHeaderLayout.xmlBuform/Resources/layout/FormItemButtonLayout.xmlBuform/Resources/layout/FormItemDateTimeLayout.xmlBuform/Resources/layout/FormItemPickerLayout.xmlBuform/Resources/layout/FormItemSegmentsLayout.xmlBuform/Resources/layout/FormItemSliderLayout.xmlBuform/Resources/layout/FormItemStepperLayout.xmlBuform/Resources/layout/FormItemSwitchLayout.xmlBuform/Resources/layout/FormItemTextInputLayout.xmlBuform/Resources/layout/FormItemTextLayout.xmlBuform/Resources/layout/FormItemTextMultilineLayout.xmlBuform/Resources/layout/PickerDialogLayout.xmlBuform/Resources/layout/PickerOptionItemLayout.xmlBuform/Resources/values/dimens.xmlBuform/Resources/values/strings.xml
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Buform/Platforms/Android/Items/Button/ButtonFormViewHolder.cs (1)
57-58:⚠️ Potential issue | 🟠 MajorDisabled buttons lack visual indication of their disabled state.
UpdateReadOnlyState()correctly setsEnabled = false, butApplyTextStyle()overrides Material's built-in disabled state styling. It creates single-stateColorStateListinstances and setsStateListAnimator = null, removing the default greyed/dimmed appearance for disabled buttons. The button becomes non-interactive but visually unchanged, potentially confusing users about whether they can interact with it.To fix this, use
ColorStateListdefinitions that include disabled state colours, or re-apply appropriate visual styling (e.g. reduced opacity, greyed text) when the button becomes disabled.
♻️ Duplicate comments (12)
Buform/Platforms/Android/Items/Button/ButtonFormViewHolder.cs (1)
12-12:⚠️ Potential issue | 🟡 MinorStill unresolved: replace obsolete
[Preserve].CI is already warning on this attribute. Please verify the trimming strategy against current .NET for Android guidance and move this to
DynamicDependencyor a linker descriptor for the actual reflected members.Buform/Resources/layout/AsyncPickerDialogLayout.xml (1)
29-30:⚠️ Potential issue | 🟠 MajorStill unresolved: restore the clear action's minimum tap target.
minHeight="0dp"lets this button drop below the 48dp accessibility floor once the small vertical padding is applied.Suggested fix
- android:minHeight="0dp" + android:minHeight="48dp"Buform/Resources/layout/PickerDialogLayout.xml (1)
29-30:⚠️ Potential issue | 🟠 MajorStill unresolved: restore a 48dp minimum height on the dialog actions.
Both text buttons opt out of the minimum tap target here, so they can drop below accessible size on short strings or tighter font metrics.
Suggested fix
- android:minHeight="0dp" + android:minHeight="48dp" @@ - android:minHeight="0dp" + android:minHeight="48dp"Also applies to: 51-52
Buform/Platforms/Android/Items/Picker/PickerDialogFragment.cs (2)
10-10: 🛠️ Refactor suggestion | 🟠 MajorMark class as
sealed.
PickerDialogFragmentdoes not declare anyvirtualorabstractmembers and is not designed for inheritance. Per coding guidelines, it should be markedsealed.♻️ Proposed fix
-public class PickerDialogFragment : AndroidX.Fragment.App.DialogFragment +public sealed class PickerDialogFragment : AndroidX.Fragment.App.DialogFragmentAs per coding guidelines: "Require marking C# classes as sealed if they are not designed for inheritance."
14-15:⚠️ Potential issue | 🟠 MajorPersist the picker item state to survive fragment recreation.
_itemis stored only as an instance field without being persisted toArgumentsorSaveInstanceState. WhenDialogFragmentis recreated by Android (after rotation or process death), the new instance initialises with_item == null, causing all setup methods to silently exit early and the dialog to render without content or callbacks.Consider storing a stable identifier in the
Argumentsbundle duringCreate()and resolving the item from a presenter or parent owner inOnViewCreated().Also applies to: 25-28
Buform/Platforms/Android/Items/DateTime/DateTimeFormViewHolder.cs (3)
133-141:⚠️ Potential issue | 🟠 MajorImplement separate picker flows for
DateTimeInputType.TimeandDateTimeInputType.DateTime.The display formatting correctly handles different
InputTypevalues (lines 125–129), butOnClickalways callsShowDatePicker()regardless of type. This prevents proper editing:
- Time-only items: clicking displays a date picker when only time should be editable.
- Date-time items: only the date can be changed; a sequential date + time picker flow is needed.
Consider implementing
ShowTimePicker()and a combined flow forDateTimeInputType.DateTime.
243-258:⚠️ Potential issue | 🔴 CriticalMaterialDatePicker converts date values through UTC epoch, not local time.
MaterialDatePickeroperates on UTC epoch milliseconds representing 00:00:00 UTC on a given calendar date. The current implementation incorrectly converts fullDateTimevalues (including time components) through local timezone boundaries, causing off-by-one date errors for users in timezones offset from UTC.🐛 Suggested fix
- private static long ToUtcMillis(DateTime dateTime) + private static long ToPickerUtcDateMillis(DateTime dateTime) { - var utc = dateTime.Kind switch - { - DateTimeKind.Utc => dateTime, - DateTimeKind.Local => dateTime.ToUniversalTime(), - _ => DateTime.SpecifyKind(dateTime, DateTimeKind.Local).ToUniversalTime() - }; - - return new DateTimeOffset(utc).ToUnixTimeMilliseconds(); + var utcDate = DateTime.SpecifyKind(dateTime.Date, DateTimeKind.Utc); + return new DateTimeOffset(utcDate).ToUnixTimeMilliseconds(); } - private static DateTime FromUtcMillis(long millis) + private static DateTime FromPickerUtcDateMillis(long millis) { - return DateTimeOffset.FromUnixTimeMilliseconds(millis).LocalDateTime; + return DateTimeOffset.FromUnixTimeMilliseconds(millis).UtcDateTime.Date; }Update call sites at lines 152, 158, 159, and 176 to use the renamed methods.
38-54:⚠️ Potential issue | 🟡 MinorHandle
InputTypechanges and full-refresh notifications.
GetDisplayValue()depends onData.InputType, butOnDataPropertyChangeddoes not handlenameof(DateTimeFormItem.InputType). Additionally, anullor emptypropertyName(indicating a full refresh) falls through to the empty default case without updating the UI.♻️ Proposed fix
protected override void OnDataPropertyChanged(string? propertyName) { switch (propertyName) { case nameof(DateTimeFormItem.Label): UpdateLabel(); break; case nameof(DateTimeFormItem.Value): + case nameof(DateTimeFormItem.InputType): UpdateValue(); break; case nameof(DateTimeFormItem.IsReadOnly): UpdateReadOnlyState(); break; default: + UpdateLabel(); + UpdateValue(); + UpdateReadOnlyState(); break; } }Buform/Platforms/Android/Items/Segments/SegmentsFormViewHolder.cs (1)
1-4:⚠️ Potential issue | 🔴 CriticalMissing
using Android.Widget;import —TextViewandLinearLayoutare unresolved.Line 10 uses
TextView?and line 168 usesLinearLayout.LayoutParams, but the requiredAndroid.Widgetnamespace is not imported. This will cause compilation errors.Minimal fix
using Android.Runtime; using Android.Views; +using Android.Widget; using Google.Android.Material.Button;Buform/Platforms/Android/Items/Picker/CallbackPickerFormViewHolder.cs (1)
57-76:⚠️ Potential issue | 🟠 MajorAdd a fallback refresh for unhandled property changes.
The default branch is a no-op, so any update that arrives under another property name can leave the label, value, or validation state stale until the row is rebound. This was previously flagged.
Proposed fix
case nameof(ICallbackPickerFormItem.ValidationErrorMessage): UpdateValidationErrorMessage(Data?.ValidationErrorMessage); break; default: - break; + UpdateReadOnlyState(); + UpdateLabel(Data?.Label); + UpdateValue(Data?.FormattedValue); + UpdateValidationErrorMessage(Data?.ValidationErrorMessage); + break; }Buform/Platforms/Android/Items/Picker/AsyncPickerDialogFragment.cs (2)
288-328: 🛠️ Refactor suggestion | 🟠 MajorExtract hardcoded UI strings to constants or localisation resources.
The state text values
"Loading...","No items", and"Failed to load items"are hardcoded inline. As per coding guidelines, user-facing strings should be extracted to constants or localisation resources.♻️ Proposed fix
public sealed class AsyncPickerDialogFragment : PickerDialogFragment { + private const string LoadingStateText = "Loading..."; + private const string EmptyStateText = "No items"; + private const string FailedStateText = "Failed to load items"; + private readonly List<IPickerOptionFormItem> _filteredOptions = [];Then use them in
UpdateState():- _stateTextView.Text = "Loading..."; + _stateTextView.Text = LoadingStateText;- _stateTextView.Text = "No items"; + _stateTextView.Text = EmptyStateText;- _stateTextView.Text = "Failed to load items"; + _stateTextView.Text = FailedStateText;
298-312:⚠️ Potential issue | 🟡 MinorCheck
_filteredOptionsinstead of_item.Optionsfor empty state.When the user filters with no matches,
_item.Optionsmay still have items while_filteredOptionsis empty. The empty state check should use the filtered list to correctly show "No items" for zero search results.♻️ Proposed fix
case AsyncPickerLoadingState.Loaded: _progressIndicator.Visibility = ViewStates.Gone; - if (!_item.Options.Any()) + if (!_filteredOptions.Any()) { _stateTextView.Visibility = ViewStates.Visible; _stateTextView.Text = "No items"; _contentContainer.Visibility = ViewStates.Gone; }
🧹 Nitpick comments (12)
Buform/Resources/drawable/bg_stepper_capsule.xml (1)
4-4: Avoid hardcoded background colour in a Material-themed control.Using a fixed hex colour here can look off in dark mode and across themes. Prefer a theme attribute or colour resource.
Proposed tweak
- <solid android:color="#E5E5EA" /> + <solid android:color="?attr/colorSurfaceVariant" />Buform/Resources/layout/FormItemSegmentsLayout.xml (1)
12-25: Link the label to the toggle group for TalkBack.If
Labelis shown, it will be announced separately rather than as the control's name. Addingandroid:labelFormakes that relationship explicit.♿ Suggested change
<com.google.android.material.textview.MaterialTextView android:id="@+id/Label" + android:labelFor="@id/ToggleGroup" android:layout_width="wrap_content" android:layout_height="wrap_content" android:layout_marginBottom="@dimen/item_spacing_small"Buform/Resources/layout/FormItemStepperLayout.xml (1)
7-13: Move the remaining sizing values into@dimenresources.This layout still hardcodes the row height, paddings, control width, divider height, and text size even though the same PR introduces
Resources/values/dimens.xml. Pulling those values through resources will make the Android Material sizing much easier to tune consistently across item layouts.Also applies to: 22-22, 29-30, 46-47
Buform/Platforms/Android/Items/Switch/SwitchFormViewHolder.cs (1)
21-152: Apply the repository member order in this holder.The private listener/update helpers currently sit below the protected overrides. Please move the private methods above
Initialize/OnDataSet/OnDataPropertyChanged, while keepingDisposeat the end.As per coding guidelines, "Enforce consistent member ordering in C# types: order members as follows – static, const, properties, constructors, methods. Within each group, order by access level: private, protected, then public. Always place Dispose() and related cleanup methods at the end of the class."
Buform.Maui/FormReflectionHelper.cs (1)
5-20: Cache the reflected lookup in the private helper.Even though this is fallback code,
GetType().GetProperty(...)will still run on every bind that lands here. A small cache inGetStringPropertywould keep reflection out of that path, and moving the private helper above the public accessors would also bring the file back in line with the repository member-order rule.As per coding guidelines, "Enforce consistent member ordering in C# types: order members as follows – static, const, properties, constructors, methods. Within each group, order by access level: private, protected, then public. Always place Dispose() and related cleanup methods at the end of the class."
Buform/Platforms/Android/Items/Slider/SliderFormViewHolder.cs (1)
20-155: Apply the repository member order in this holder.The private event handler currently sits below the protected update members. Please move private helpers above the protected methods, while keeping
Disposeat the end.As per coding guidelines, "Enforce consistent member ordering in C# types: order members as follows – static, const, properties, constructors, methods. Within each group, order by access level: private, protected, then public. Always place Dispose() and related cleanup methods at the end of the class."
Buform.Maui/Platforms/Android/MauiFormHeaderFooterViewHolder.cs (1)
67-83: Consider extracting hardcoded dimensions to resource constants.The padding values (16, 24, 8, 4 dp) and text sizes (14, 12 sp) are hardcoded inline. For consistency with Material Design resource management and easier maintenance, consider moving these to
dimens.xmlor at least to named constants within the class.Buform/Platforms/Android/Items/Text/TextFormViewHolder.cs (1)
22-22: Extract hardcoded text size to a constant.The text size
18is hardcoded inline. Per coding guidelines, hardcoded values should be extracted to named constants for maintainability and consistency.♻️ Proposed fix
+private const int DefaultTextSizeSp = 18; + public TextFormViewHolder(IntPtr javaReference, JniHandleOwnership transfer) : base(javaReference, transfer) { }-_textView.SetTextSize(ComplexUnitType.Sp, 18); +_textView.SetTextSize(ComplexUnitType.Sp, DefaultTextSizeSp);Buform/Platforms/Android/Items/Segments/SegmentsFormViewHolder.cs (1)
162-163: Store button index usingJava.Lang.Integerexplicitly.The implicit boxing of
indextoTagworks, but the retrieval at line 96 expectsJava.Lang.Integer. For clarity and type safety, consider explicitly wrapping the value.♻️ Suggested improvement
- Tag = index + Tag = new Java.Lang.Integer(index)Buform.Maui/Platforms/Android/MauiFormRecyclerAdapter.cs (1)
220-250: Consider caching the chrome count calculation.
GetItemsCount()iterates all sections on everyItemCountaccess. For large forms, this could be called frequently (e.g., during scrolling). Consider caching the computed count and invalidating it when the form changes.Buform/Platforms/Android/Items/Picker/PickerFormViewHolderBase.cs (1)
68-72: Use theme-aware colour for validation errors.
Color.Redis hardcoded, which bypasses Material theme attributes and may not render correctly in dark mode. Other view holders in this PR (e.g.,SegmentsFormViewHolder) useResource.Attribute.colorErrorfor consistency.♻️ Proposed fix
protected virtual void UpdateValidationErrorMessage(string? validationErrorMessage) { if (LabelView == null) { return; } var hasValidationError = !string.IsNullOrWhiteSpace(validationErrorMessage); LabelView.SetTextColor( !hasValidationError ? ResolveThemeColor(Android.Resource.Attribute.TextColorPrimary) - : Color.Red + : ResolveThemeColor(Resource.Attribute.colorError) ); }Buform/Platforms/Android/Items/Picker/PresentedPickerFormViewHolderBase.cs (1)
27-42: Regroup the private click handler with the private methods.
OnItemViewClicksits in the middle of the protected method block, so the methods section no longer follows the repo’s private → protected → public ordering.As per coding guidelines, "Enforce consistent member ordering in C# types: order members as follows – static, const, properties, constructors, methods. Within each group, order by access level: private, protected, then public. Always place Dispose() and related cleanup methods at the end of the class."
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aca40aee-bfae-425f-ae37-49a7b3be1b89
📒 Files selected for processing (67)
.github/workflows/ci.ymlBuform.Example.Droid/Buform.Example.Droid.csprojBuform.Example.Maui/Buform.Example.Maui.csprojBuform.Example.Maui/Platforms/Android/MainApplication.csBuform.Example.Maui/RandomNumberGeneratorView.xamlBuform.Example/Buform.Example.csprojBuform.Example/MenuViewModel.csBuform.Maui/Buform.Maui.csprojBuform.Maui/FormReflectionHelper.csBuform.Maui/FormReflectionKeys.csBuform.Maui/Platforms/Android/FormAdapterItem.csBuform.Maui/Platforms/Android/FormViewHandler.csBuform.Maui/Platforms/Android/MauiFormHeaderFooterViewHolder.csBuform.Maui/Platforms/Android/MauiFormItemViewHolder.csBuform.Maui/Platforms/Android/MauiFormRecyclerAdapter.csBuform.Maui/Platforms/Android/MauiFormViewHolderBase.csBuform.MvvmCross/Buform.MvvmCross.csprojBuform/Buform.csprojBuform/Platforms/Android/Items/Button/ButtonFormViewHolder.csBuform/Platforms/Android/Items/DateTime/ContextExtensions.csBuform/Platforms/Android/Items/DateTime/DateTimeFormItemComponent.csBuform/Platforms/Android/Items/DateTime/DateTimeFormViewHolder.csBuform/Platforms/Android/Items/Picker/AsyncPickerDialogFragment.csBuform/Platforms/Android/Items/Picker/AsyncPickerFormViewHolder.csBuform/Platforms/Android/Items/Picker/CallbackPickerFormViewHolder.csBuform/Platforms/Android/Items/Picker/DialogFragmentPickerPresenter.csBuform/Platforms/Android/Items/Picker/MultiValuePickerFormViewHolder.csBuform/Platforms/Android/Items/Picker/PickerDialogFragment.csBuform/Platforms/Android/Items/Picker/PickerFormItemComponent.csBuform/Platforms/Android/Items/Picker/PickerFormViewHolderBase.csBuform/Platforms/Android/Items/Picker/PickerOptionViewHolder.csBuform/Platforms/Android/Items/Picker/PickerOptionsAdapter.csBuform/Platforms/Android/Items/Picker/PickerPresenterBase.csBuform/Platforms/Android/Items/Picker/PopUpPickerFormViewHolder.csBuform/Platforms/Android/Items/Picker/PresentedPickerFormViewHolderBase.csBuform/Platforms/Android/Items/Segments/SegmentsFormItemComponent.csBuform/Platforms/Android/Items/Segments/SegmentsFormViewHolder.csBuform/Platforms/Android/Items/Slider/SliderFormItemComponent.csBuform/Platforms/Android/Items/Slider/SliderFormViewHolder.csBuform/Platforms/Android/Items/Stepper/StepperFormItemComponent.csBuform/Platforms/Android/Items/Stepper/StepperFormViewHolder.csBuform/Platforms/Android/Items/Switch/SwitchFormItemComponent.csBuform/Platforms/Android/Items/Switch/SwitchFormViewHolder.csBuform/Platforms/Android/Items/Text/TextFormItemComponent.csBuform/Platforms/Android/Items/Text/TextFormViewHolder.csBuform/Platforms/Android/Items/Text/TextInputFormViewHolder.csBuform/Platforms/Android/Items/Text/TextMultilineFormViewHolder.csBuform/Resources/drawable/bg_stepper_capsule.xmlBuform/Resources/drawable/ic_stepper_minus.xmlBuform/Resources/drawable/ic_stepper_plus.xmlBuform/Resources/layout/AsyncPickerDialogLayout.xmlBuform/Resources/layout/FormGroupTextFooterLayout.xmlBuform/Resources/layout/FormGroupTextHeaderLayout.xmlBuform/Resources/layout/FormItemButtonLayout.xmlBuform/Resources/layout/FormItemDateTimeLayout.xmlBuform/Resources/layout/FormItemPickerLayout.xmlBuform/Resources/layout/FormItemSegmentsLayout.xmlBuform/Resources/layout/FormItemSliderLayout.xmlBuform/Resources/layout/FormItemStepperLayout.xmlBuform/Resources/layout/FormItemSwitchLayout.xmlBuform/Resources/layout/FormItemTextInputLayout.xmlBuform/Resources/layout/FormItemTextLayout.xmlBuform/Resources/layout/FormItemTextMultilineLayout.xmlBuform/Resources/layout/PickerDialogLayout.xmlBuform/Resources/layout/PickerOptionItemLayout.xmlBuform/Resources/values/dimens.xmlBuform/Resources/values/strings.xml
| _container.RemoveAllViews(); | ||
|
|
||
| _legacyViewHolder = viewHolder; | ||
|
|
||
| _container.AddView(view); | ||
|
|
||
| if (item is not IDisposable disposable) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| viewHolder.Initialize(disposable); | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
Reorder the IDisposable check to avoid partial binding state.
The current logic adds the view to the container (lines 81, 85) before checking if item is IDisposable (line 87). If the check fails, the method returns false but the view remains attached, leaving the holder in an inconsistent state where subsequent binding paths may be attempted whilst the legacy view is already displayed.
🐛 Proposed fix
if (Activator.CreateInstance(viewHolderType, view) is not FormViewHolder viewHolder)
{
return false;
}
+ if (item is not IDisposable disposable)
+ {
+ return false;
+ }
+
_container.RemoveAllViews();
_legacyViewHolder = viewHolder;
_container.AddView(view);
- if (item is not IDisposable disposable)
- {
- return false;
- }
-
viewHolder.Initialize(disposable);
return true;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _container.RemoveAllViews(); | |
| _legacyViewHolder = viewHolder; | |
| _container.AddView(view); | |
| if (item is not IDisposable disposable) | |
| { | |
| return false; | |
| } | |
| viewHolder.Initialize(disposable); | |
| return true; | |
| } | |
| if (item is not IDisposable disposable) | |
| { | |
| return false; | |
| } | |
| _container.RemoveAllViews(); | |
| _legacyViewHolder = viewHolder; | |
| _container.AddView(view); | |
| viewHolder.Initialize(disposable); | |
| return true; |
| protected void BindMauiView(object bindingContext, Type viewType) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(bindingContext); | ||
| ArgumentNullException.ThrowIfNull(viewType); | ||
|
|
||
| EnsureMauiView(viewType); | ||
| EnsureMauiContentAttached(); | ||
|
|
||
| if (_mauiView == null) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| OnBeforeBind(_mauiView, bindingContext); | ||
|
|
||
| _mauiView.BindingContext = bindingContext; | ||
|
|
||
| OnAfterBind(_mauiView, bindingContext); | ||
|
|
||
| _contentMode = ContentMode.Maui; | ||
| MeasureAndArrange(); | ||
| } |
There was a problem hiding this comment.
Reset the layout retry budget for each new bind.
_layoutRetryCount is only cleared after a successful measure. Once a holder has exhausted MaxLayoutRetries, later binds on the same recycled instance inherit that exhausted state and stop posting retries, even if the next item would lay out correctly.
♻️ Possible fix
protected void BindMauiView(object bindingContext, Type viewType)
{
ArgumentNullException.ThrowIfNull(bindingContext);
ArgumentNullException.ThrowIfNull(viewType);
+ _layoutRetryCount = 0;
EnsureMauiView(viewType);
EnsureMauiContentAttached();
@@
protected void ClearAllCachedContent()
{
if (_platformView?.Parent == _container)
{
_container.RemoveView(_platformView);
}
@@
_fallbackView = null;
_contentMode = ContentMode.None;
+ _layoutRetryCount = 0;
}Also applies to: 154-176, 245-263
| protected void ShowFallback(AView fallbackView) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(fallbackView); | ||
|
|
||
| if (ReferenceEquals(_fallbackView, fallbackView) && _contentMode == ContentMode.Fallback) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| DetachCurrentContent(); | ||
|
|
||
| _fallbackView = fallbackView; | ||
| _container.AddView( | ||
| _fallbackView, | ||
| new FrameLayout.LayoutParams( | ||
| ViewGroup.LayoutParams.MatchParent, | ||
| ViewGroup.LayoutParams.WrapContent | ||
| ) | ||
| ); | ||
|
|
||
| _contentMode = ContentMode.Fallback; | ||
| } |
There was a problem hiding this comment.
Unbind cached views before you replace them.
ShowFallback(), EnsureMauiView() and ClearAllCachedContent() can all drop the current view without going through the unbind path. On a recycled holder that switches MAUI view type, the abandoned _mauiView keeps its previous BindingContext; fallback swaps also skip OnFallbackUnbind().
♻️ Possible fix
protected void ShowFallback(AView fallbackView)
{
ArgumentNullException.ThrowIfNull(fallbackView);
@@
+ Unbind();
DetachCurrentContent();
@@
protected void ClearAllCachedContent()
{
+ Unbind();
if (_platformView?.Parent == _container)
{
_container.RemoveView(_platformView);
}
@@
private void EnsureMauiView(Type viewType)
{
if (_mauiView != null && _platformView != null && _boundViewType == viewType)
{
return;
}
+ Unbind();
if (_platformView?.Parent == _container)
{
_container.RemoveView(_platformView);
}Also applies to: 154-176, 178-205
| var min = Data.MinValue; | ||
| var max = Data.MaxValue; | ||
|
|
||
| if (max < min) | ||
| { | ||
| max = min; | ||
| } | ||
|
|
||
| if (Math.Abs(_slider.ValueFrom - min) > Tolerance) | ||
| { | ||
| _slider.ValueFrom = min; | ||
| } | ||
|
|
||
| if (Math.Abs(_slider.ValueTo - max) > Tolerance) | ||
| { | ||
| _slider.ValueTo = max; | ||
| } | ||
| } | ||
|
|
||
| protected virtual void UpdateStepSize() | ||
| { | ||
| if (_slider == null || Data == null) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| const int stepSize = 1; | ||
| if (Math.Abs(_slider.StepSize - stepSize) > Tolerance) | ||
| { | ||
| _slider.StepSize = stepSize; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the slider model and related references without modifying the repo.
fd -a "SliderFormItem.cs" -x sed -n '1,220p' {}
printf '\n--- slider model references ---\n'
rg -n --type=cs -C2 '\b(SliderFormItem|MinValue|MaxValue|Value|Step(Size)?|Increment)\b'Repository: fedandburk/buform
Length of output: 50373
Do not hardcode step size for float-based sliders, and validate range configuration.
SliderFormItem is a FormItem<float> supporting fractional values across a floating-point range. Hardcoding StepSize = 1 (line 107) breaks this capability—you cannot represent fractional increments or sliders with ranges smaller than 1. Additionally, when MaxValue < MinValue, the code collapses both to the same value (lines 84–87), leaving zero selectable interval.
Derive the step size from the form item or calculate it from the range. Validate that MinValue ≤ MaxValue before assignment, or reject invalid configurations early.
| private void UpdateValue() | ||
| { | ||
| if (_switch == null || Data == null) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| if (_switch.Checked == Data.Value) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| _isUpdatingFromModel = true; | ||
|
|
||
| try | ||
| { | ||
| _switch.Checked = Data.Value; | ||
| } | ||
| finally | ||
| { | ||
| _isUpdatingFromModel = false; | ||
| } |
There was a problem hiding this comment.
Reset the switch when no item is bound.
UpdateTitle() and UpdateReadOnlyState() already cope with Data == null, but UpdateValue() returns before touching _switch.Checked. On a recycled holder that can leave the previous row's value visible until another item is bound.
💡 Suggested fix
private void UpdateValue()
{
- if (_switch == null || Data == null)
+ if (_switch == null)
{
return;
}
+
+ var isChecked = Data?.Value ?? false;
- if (_switch.Checked == Data.Value)
+ if (_switch.Checked == isChecked)
{
return;
}
_isUpdatingFromModel = true;
try
{
- _switch.Checked = Data.Value;
+ _switch.Checked = isChecked;
}
finally
{
_isUpdatingFromModel = false;
}| android:layout_width="0dp" | ||
| android:layout_height="wrap_content" | ||
| android:layout_weight="1" |
There was a problem hiding this comment.
Prevent long picker values from hiding the field label.
Value is the only non-weighted child here, so a long formatted value can consume the row and leave Label at 0 width. Give the value view an explicit width cap or constraint so both texts remain visible.
Also applies to: 28-35
| <com.google.android.material.textview.MaterialTextView | ||
| android:id="@+id/Title" |
There was a problem hiding this comment.
Give the switch an explicit accessibility label.
The visible title is a separate TextView, so accessibility services can land on an unnamed switch. Link the label to @id/Switch (or move the text onto the control itself).
Suggested fix
<com.google.android.material.textview.MaterialTextView
android:id="@+id/Title"
+ android:labelFor="@id/Switch"
android:layout_width="0dp"Also applies to: 26-30
1 participant
No description provided.