refactor: Use WidgetState instead of MixWidgetState #582
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
WalkthroughThe changes refactor how the "long pressed" state is handled in the widget state management system. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GestureWidget
participant _GestureMixStateWidgetState
participant LongPressInheritedState
participant DescendantWidget
User->>GestureWidget: Performs long press gesture
GestureWidget->>_GestureMixStateWidgetState: onLongPressStart callback
_GestureMixStateWidgetState->>_GestureMixStateWidgetState: _setLongPressed(true)
_GestureMixStateWidgetState->>LongPressInheritedState: Rebuild with longPressed=true
DescendantWidget->>LongPressInheritedState: Access longPressed via of(context)
LongPressInheritedState-->>DescendantWidget: Returns current longPressed state
User->>GestureWidget: Releases long press
GestureWidget->>_GestureMixStateWidgetState: onLongPressEnd callback
_GestureMixStateWidgetState->>_GestureMixStateWidgetState: _setLongPressed(false)
_GestureMixStateWidgetState->>LongPressInheritedState: Rebuild with longPressed=false
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (4)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (1)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/mix/lib/src/core/widget_state/internal/mix_widget_state_builder.dart (3)
49-55: Consider exposing amaybeOfhelper for null-safe look-ups
of(context)returnsnullwhen the model is not in the tree, but callers have to remember to perform the null-check every time. A smallmaybeOf(ortryOf) wrapper that simply delegates toinheritFromwithout the non-nullable cast would make call-sites cleaner and keep the public API symmetrical with Flutter’s ownTheme.of / Theme.maybeOfpattern.
56-71: Replace the longswitchwith a map to improve maintainabilityAs new
WidgetStatevalues are introduced, thisswitchcan easily fall out of sync and silently returnfalsefor unhandled states (as the default branch does today). Building a map once and indexing into it keeps the code DRY and makes missing keys immediately obvious.- return switch (state) { - WidgetState.disabled => model.disabled, - WidgetState.hovered => model.hovered, - WidgetState.focused => model.focused, - WidgetState.pressed => model.pressed, - WidgetState.dragged => model.dragged, - WidgetState.selected => model.selected, - WidgetState.error => model.error, - _ => false, - }; + const stateSelectors = <WidgetState, bool Function(MixWidgetStateModel)>{ + WidgetState.disabled: (m) => m.disabled, + WidgetState.hovered: (m) => m.hovered, + WidgetState.focused: (m) => m.focused, + WidgetState.pressed: (m) => m.pressed, + WidgetState.dragged: (m) => m.dragged, + WidgetState.selected: (m) => m.selected, + WidgetState.error: (m) => m.error, + }; + + final selector = stateSelectors[state]; + return selector != null ? selector(model) : false;
96-111: Readability: parenthesise OR-chains or extract to helperThe current boolean expression relies on
&&having higher precedence than||. While correct, it is easy to mis-read. Two small tweaks can avoid cognitive overhead:
- Add explicit parentheses around each
<diff && dependencies.contains>pair, or- Extract the comparison into a private helper returning
bool hasChanged(WidgetState)and loop overdependencies.Either way future maintainers will thank you. 🙂
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/mix/example/.gitignore(1 hunks)packages/mix/lib/src/core/widget_state/internal/gesture_mix_state.dart(3 hunks)packages/mix/lib/src/core/widget_state/internal/mix_widget_state_builder.dart(3 hunks)packages/mix/lib/src/core/widget_state/widget_state_controller.dart(4 hunks)packages/mix/lib/src/variants/widget_state_variant.dart(3 hunks)packages/mix/test/helpers/testing_utils.dart(0 hunks)packages/mix/test/src/core/mix_state/internal/gesture_mix_state_test.dart(6 hunks)packages/mix/test/src/core/mix_state/internal/interactive_mix_state_test.dart(5 hunks)packages/mix/test/src/variants/widget_state_variant_test.dart(2 hunks)packages/mix/test/src/widgets/widget_state/widget_state_controller_test.dart(7 hunks)
💤 Files with no reviewable changes (1)
- packages/mix/test/helpers/testing_utils.dart
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test Min SDK
- GitHub Check: Test
🔇 Additional comments (30)
packages/mix/example/.gitignore (1)
8-8: Appropriate addition to .gitignore.Adding
.build/and.swiftpm/directories to the ignore list ensures Swift Package Manager build artifacts aren't accidentally committed, maintaining a clean repository.Also applies to: 12-12
packages/mix/test/src/variants/widget_state_variant_test.dart (2)
5-5: Import added for LongPressInheritedState.Correctly added the import for the new
gesture_mix_state.dartto support the refactored long press state implementation.
37-43: Refactored long press state test to use LongPressInheritedState.The test has been appropriately updated to use the new
LongPressInheritedStatewidget directly rather than relying on the helper method with alongPressedparameter. This aligns with the PR's objective of removingMixWidgetStateand managing long press state through a dedicated inherited widget.packages/mix/test/src/core/mix_state/internal/interactive_mix_state_test.dart (1)
20-20: Updated widget state references.All references to
MixWidgetState.disabled,MixWidgetState.focused, andMixWidgetState.hoveredhave been consistently replaced withWidgetState.disabled,WidgetState.focused, andWidgetState.hovered. This aligns with the PR objective to useWidgetStateinstead ofMixWidgetState.Also applies to: 30-30, 43-44, 53-53, 67-67, 75-75
packages/mix/test/src/core/mix_state/internal/gesture_mix_state_test.dart (2)
59-60: Updated pressed state references.All references to
MixWidgetState.pressedhave been consistently replaced withWidgetState.pressed, maintaining the same functionality while aligning with the PR objective.Also applies to: 100-103, 109-112, 118-122, 136-136
81-84: Refactored long press state checking.The test has been appropriately updated to check the long press state using
LongPressInheritedState.of(context).longPressedinstead ofMixWidgetState.hasStateOf(context, MixWidgetState.longPressed). This aligns with the new approach of managing long press state through a dedicated inherited widget.packages/mix/test/src/widgets/widget_state/widget_state_controller_test.dart (7)
80-83: LGTM! Clean enum type replacement.The code correctly updates the batch operation to use
WidgetStateenum instead of the oldMixWidgetState. This is consistent with the PR objective of removingMixWidgetState.
103-105: LGTM! Enum type consistently updated.The batch operations parameter updates maintain the same functionality while using the new enum type.
167-168: LGTM! Updated widget state testing.The state verification is correctly updated to use
WidgetState.disabledinstead of the previous enum type.
174-175: LGTM! Consistent enum usage maintained.The negative test case is also correctly updated to use
WidgetState.hoveredenum value.
235-239: LGTM! Dependency notification checks updated.Both the positive and negative dependency notification tests are correctly updated to use
WidgetStateenum values.
444-444: LGTM! Method signature updated correctly.The
_widgetStateOfmethod now acceptsWidgetStateinstead ofMixWidgetState, consistent with the overall refactoring.
459-468: LGTM! Widget state builder invocations updated.All test widget state builders have been updated to use the new
WidgetStateenum values.packages/mix/lib/src/core/widget_state/internal/gesture_mix_state.dart (5)
90-90: LGTM! Added local state for long press tracking.This new field correctly implements the PR objective to move long press state management away from
MixWidgetStateControllerinto a local variable.
98-102: LGTM! Well-encapsulated state update method.This helper method ensures the long press state is consistently updated through setState, triggering proper widget rebuilds.
127-127: LGTM! Consistent local state updates.All gesture callbacks have been updated to use the new
_setLongPressedmethod instead of modifying the controller directly, ensuring proper state management and widget rebuilds.Also applies to: 132-132, 137-137, 142-142, 147-147
198-218: LGTM! Appropriate use of InheritedWidget pattern.The build method now correctly wraps the gesture detector with the new
LongPressInheritedStatewidget, making the long press state available to descendants through the widget tree rather than through the controller.
222-246: LGTM! Well-implemented InheritedWidget.The
LongPressInheritedStateimplementation follows best practices:
- Provides both
maybeOfandofstatic methods for accessing the state- Proper assertion message when not found in context
- Correct implementation of
updateShouldNotifyto only trigger rebuilds when the state changesThis is a clean way to expose the long press state to descendant widgets.
packages/mix/lib/src/variants/widget_state_variant.dart (5)
5-5: LGTM! Import added for new dependency.The import for the gesture_mix_state.dart file is necessary to access the new
LongPressInheritedStateclass.
29-29: LGTM! Updated enum type in base class.The
_ToggleMixStateVariantclass now correctly usesWidgetStateinstead ofMixWidgetState.
59-60: LGTM! Updated enum reference in hover variant.The
OnHoverVariantclass now correctly referencesWidgetState.hovered.
65-66: LGTM! Consistent enum updates in all variant classes.All variant class constructors have been updated to use
WidgetStateenum values instead ofMixWidgetState.Also applies to: 83-84, 88-89, 93-94, 98-99, 103-104
69-79: LGTM! Well-refactored long press variant.The
OnLongPressVariantclass has been completely refactored to:
- Extend
ContextVariantdirectly instead of_ToggleMixStateVariant- Access the long press state through
LongPressInheritedState.of(context).longPressedThis implementation properly aligns with the new architecture where long press state is managed by an inherited widget rather than the controller.
packages/mix/lib/src/core/widget_state/widget_state_controller.dart (6)
5-8: LGTM! Clean extension implementation.Creating an extension on
WidgetStateis an elegant way to maintain the static helper methods while eliminating the separate enum. This preserves the familiar API interface while simplifying the underlying implementation.
24-24: LGTM! Updated internal state storage type.The controller's internal state set has been updated to use
WidgetStateinstead ofMixWidgetState.
27-67: LGTM! Consistent getter and setter updates.All the getters and setters for individual widget states have been updated to use
WidgetStateenum values. Note that thelongPressedproperty has been removed as expected, since it's now managed separately.
73-79: LGTM! Updated method signature.The
updatemethod's parameter type has been changed fromMixWidgetStatetoWidgetState.
86-101: LGTM! Updated batch method signature.The
batchmethod now accepts a list of tuples withWidgetStateinstead ofMixWidgetState.
104-104: LGTM! Updated has method signature.The
hasmethod now acceptsWidgetStateinstead ofMixWidgetState.packages/mix/lib/src/core/widget_state/internal/mix_widget_state_builder.dart (1)
36-47:InheritedModelgeneric update looks correct – ✅Switching the generic from
MixWidgetStatetoWidgetStateis exactly what enables dependants to subscribe to the new, unified enum. No functional or type–safety issues spotted here.
| @@ -87,13 +87,20 @@ class _GestureMixStateWidgetState extends State<GestureMixStateWidget> { | |||
| int _pressCount = 0; | |||
| Timer? _timer; | |||
| late final MixWidgetStateController _controller; | |||
| bool _longPressed = false; | |||
There was a problem hiding this comment.
So this is done just for backwards compatibility? We also want to deprecate this, so can we also make sure the utilities that use long press have this marked as deprecated?
There was a problem hiding this comment.
Yes, this is solely for backward compatibility. I will include the deprecated annotation for them.
| @@ -85,5 +85,5 @@ final class BoolUtility<T extends Attribute> extends ScalarUtility<T, bool> { | |||
| /// This class extends [DoubleUtility] and serves as a base for more specific sizing utilities. | |||
| abstract base class SizingUtility<T extends Attribute> | |||
| extends ScalarUtility<T, double> { | |||
| SizingUtility(super.builder); | |||
| const SizingUtility(super.builder); | |||
There was a problem hiding this comment.
Why is the const being added now? Is this the Dart fix? Because removing this later would be a breaking change, just FYI. But I think we should keep it; it is not a mistake.
Description
MixWidgetStateand useMixWidgetStateinsteadChanges
LongPressInheritedStatethat expose the longPress state instead of depend onMixWidgetState;MixWidgetStateforWidgetStateReview Checklist
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests