Skip to content

Commit

Permalink
Fix assertion error when page transition in declarative Navigation Sh…
Browse files Browse the repository at this point in the history
…eet (#115)

The core logic of the navigation sheet has been reworked to fix #94.
Here's the summary:
- Added `SheetExtentScopeKey`, which is a reference to a `SheetExtent`
and also provides a hook to instantiation of the extent.
- Added `SheetExtentFactory` and changed `SheetExtentScope` to be able
to inject the factory of the extent object.
- Changed the navigation sheet to use a specialized extent (called
`_NavigationSheetExtent`) instead of the one created by the controller.
- The `_NavigationSheetExtent` stores and manages the
`SheetExtentScopeKey`s of the local extent scopes for each
`NavigationSheetRoute.
- Changed `NavigationSheetRoute` to create/discard a scope key when it
is installed/disposed of.
- Changed `NavigationSheetRouteContent` to attach the scope key
associated with the parent `NavigationSheetRoute` to the child
`SheetExtentScope`, which hosts the local sheet extent for the route.
- Changed `_ProxySheetActivity` and `_TransitionSheetActivity` to the
use scope key(s) to reference the local extent(s).

This PR also involves some breaking changes in the **internal** APIs.
- Renamed `initWith()` to `init()` in `SheetActivity`.
- Removed `SheetExtentInitializer`.
- Removed `initializer` and added `factory` in `SheetExtentScope`.
- Removed `initializer` and added `factory` in `SheetContainer` as well.
- Made `SheetExtentScopeState` private.
- Removed `NavigationSheetEntry`.
  • Loading branch information
fujidaiti authored May 6, 2024
1 parent 2106495 commit def84c8
Show file tree
Hide file tree
Showing 10 changed files with 464 additions and 298 deletions.
4 changes: 4 additions & 0 deletions package/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
# Changelog

## 0.5.3 May 6, 2024

- Fix an assertion error when specific page transition scenarios in declarative 'NavigationSheet' (#94)

## 0.5.2 May 5, 2024

- Fix a crash during the first build of `NavigationSheet` with a path that contains multiple routes such as `/a/b/c` (#109)
Expand Down
1 change: 0 additions & 1 deletion package/lib/smooth_sheets.dart
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ export 'src/foundation/sheet_extent.dart';
export 'src/foundation/theme.dart';
export 'src/modal/cupertino.dart';
export 'src/modal/modal_sheet.dart';
export 'src/navigation/navigation_route.dart';
export 'src/navigation/navigation_routes.dart';
export 'src/navigation/navigation_sheet.dart';
export 'src/scrollable/scrollable_sheet.dart'
Expand Down
12 changes: 5 additions & 7 deletions package/lib/src/foundation/activities.dart
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ abstract class SheetActivity {
SheetStatus get status;

@mustCallSuper
void initWith(SheetExtent owner) {
void init(SheetExtent owner) {
assert(
_owner == null,
'initWith() must be called only once.',
Expand Down Expand Up @@ -142,8 +142,6 @@ abstract class SheetActivity {
Size? oldViewportSize,
EdgeInsets? oldViewportInsets,
) {
assert(owner.metrics.hasDimensions);

if (oldContentSize == null && oldViewportSize == null) {
// The sheet was laid out, but not changed in size.
return;
Expand Down Expand Up @@ -242,8 +240,8 @@ class UserDragSheetActivity extends SheetActivity
final DragGestureRecognizer gestureRecognizer;

@override
void initWith(SheetExtent owner) {
super.initWith(owner);
void init(SheetExtent owner) {
super.init(owner);
gestureRecognizer
..onUpdate = onDragUpdate
..onEnd = onDragEnd
Expand Down Expand Up @@ -304,8 +302,8 @@ mixin ControlledSheetActivityMixin on SheetActivity {
SheetStatus get status => SheetStatus.controlled;

@override
void initWith(SheetExtent delegate) {
super.initWith(delegate);
void init(SheetExtent delegate) {
super.init(delegate);
controller = createAnimationController()..addListener(onAnimationTick);
// Won't trigger if we dispose 'animation' first.
onAnimationStart().whenComplete(onAnimationEnd);
Expand Down
7 changes: 4 additions & 3 deletions package/lib/src/foundation/framework.dart
Original file line number Diff line number Diff line change
Expand Up @@ -11,18 +11,18 @@ class SheetContainer extends StatelessWidget {
const SheetContainer({
super.key,
this.scopeKey,
this.initializer,
this.delegate = const SheetExtentDelegate(),
required this.controller,
required this.config,
this.factory,
required this.child,
});

final Key? scopeKey;
final SheetExtentInitializer? initializer;
final SheetExtentDelegate delegate;
final SheetController controller;
final SheetExtentConfig config;
final SheetExtentFactory? factory;
final Widget child;

@override
Expand All @@ -31,8 +31,9 @@ class SheetContainer extends StatelessWidget {
key: scopeKey,
config: config,
controller: controller,
initializer: initializer,
delegate: delegate,
factory: factory,
isPrimary: true,
child: Builder(
builder: (context) {
return SheetViewport(
Expand Down
4 changes: 2 additions & 2 deletions package/lib/src/foundation/sheet_controller.dart
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import 'sheet_extent.dart';
import 'sheet_status.dart';

class SheetController extends ChangeNotifier
implements ValueListenable<SheetMetrics> {
implements ValueListenable<SheetMetrics>, SheetExtentFactory {
SheetExtent? _client;

/// A notifier which notifies listeners immediately when the [_client] fires.
Expand Down Expand Up @@ -52,7 +52,7 @@ class SheetController extends ChangeNotifier
}
}

@factory
@override
SheetExtent createSheetExtent({
required SheetContext context,
required SheetExtentConfig config,
Expand Down
127 changes: 98 additions & 29 deletions package/lib/src/foundation/sheet_extent.dart
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,13 @@ class SheetExtent extends ChangeNotifier
'Do not call this method until all dimensions changes are finalized.',
),
);
assert(
metrics.hasDimensions,
_debugMessage(
'All the dimension values must be finalized '
'at the time onDimensionsFinalized() is called.',
),
);

_activity!.didFinalizeDimensions(
_oldContentSize,
Expand All @@ -278,7 +285,7 @@ class SheetExtent extends ChangeNotifier
final oldActivity = _activity;
// Update the current activity before initialization.
_activity = activity;
activity.initWith(this);
activity.init(this);

if (oldActivity != null) {
activity.takeOver(oldActivity);
Expand Down Expand Up @@ -623,7 +630,49 @@ abstract class SheetContext {
BuildContext? get notificationContext;
}

typedef SheetExtentInitializer = SheetExtent Function(SheetExtent);
@optionalTypeArgs
class SheetExtentScopeKey<T extends SheetExtent>
extends LabeledGlobalKey<_SheetExtentScopeState> {
SheetExtentScopeKey({String? debugLabel}) : super(debugLabel);

final List<VoidCallback> _onCreatedListeners = [];

T? get maybeCurrentExtent =>
switch (currentState?._extent) { final T extent => extent, _ => null };

T get currentExtent => maybeCurrentExtent!;

void addOnCreatedListener(VoidCallback listener) {
_onCreatedListeners.add(listener);
// Immediately notify the listener if the extent is already created.
if (maybeCurrentExtent != null) {
listener();
}
}

void removeOnCreatedListener(VoidCallback listener) {
_onCreatedListeners.remove(listener);
}

void removeAllOnCreatedListeners() {
_onCreatedListeners.clear();
}

void _notifySheetExtentCreation() {
for (final listener in _onCreatedListeners) {
listener();
}
}
}

abstract class SheetExtentFactory {
@factory
SheetExtent createSheetExtent({
required SheetContext context,
required SheetExtentConfig config,
required SheetExtentDelegate delegate,
});
}

/// A widget that creates a [SheetExtent], manages its lifecycle,
/// and exposes it to the descendant widgets.
Expand All @@ -632,10 +681,10 @@ class SheetExtentScope extends StatefulWidget {
const SheetExtentScope({
super.key,
required this.controller,
this.initializer,
this.isPrimary = true,
required this.config,
this.factory,
this.delegate = const SheetExtentDelegate(),
this.isPrimary = true,
required this.child,
});

Expand All @@ -646,15 +695,18 @@ class SheetExtentScope extends StatefulWidget {

final SheetExtentDelegate delegate;

final SheetExtentInitializer? initializer;
/// The factory that creates the [SheetExtent].
///
/// If this is null, the [controller] will be used to create the extent.
final SheetExtentFactory? factory;

final bool isPrimary;

/// The widget below this widget in the tree.
final Widget child;

@override
State<SheetExtentScope> createState() => SheetExtentScopeState();
State<SheetExtentScope> createState() => _SheetExtentScopeState();

/// Retrieves the [SheetExtent] from the closest [SheetExtentScope]
/// that encloses the given context, if any.
Expand All @@ -678,10 +730,9 @@ class SheetExtentScope extends StatefulWidget {
}
}

class SheetExtentScopeState extends State<SheetExtentScope>
class _SheetExtentScopeState extends State<SheetExtentScope>
with TickerProviderStateMixin
implements SheetContext {
SheetExtent get extent => _extent;
late SheetExtent _extent;

@override
Expand All @@ -690,10 +741,16 @@ class SheetExtentScopeState extends State<SheetExtentScope>
@override
BuildContext? get notificationContext => mounted ? context : null;

SheetExtentScopeKey? get _scopeKey => switch (widget.key) {
final SheetExtentScopeKey key => key,
_ => null,
};

@override
void initState() {
super.initState();
_extent = _createExtent();
_extent = _createExtent(_resolveFactory(widget));
_scopeKey?._notifySheetExtentCreation();
}

@override
Expand All @@ -712,27 +769,28 @@ class SheetExtentScopeState extends State<SheetExtentScope>
void didUpdateWidget(SheetExtentScope oldWidget) {
super.didUpdateWidget(oldWidget);
final oldExtent = _extent;
if (widget.controller != oldWidget.controller ||
widget.delegate != oldWidget.delegate) {
_extent = _createExtent()..takeOver(oldExtent);
final oldFactory = _resolveFactory(oldWidget);
final newFactory = _resolveFactory(widget);
if (newFactory != oldFactory || widget.delegate != oldWidget.delegate) {
_extent = _createExtent(newFactory)..takeOver(oldExtent);
_scopeKey?._notifySheetExtentCreation();
_discard(oldExtent);
} else if (widget.config != oldWidget.config) {
_extent.applyNewConfig(widget.config);
}
_invalidateExtentOwnership();
}

SheetExtent _createExtent() {
final extent = widget.controller.createSheetExtent(
SheetExtentFactory _resolveFactory(SheetExtentScope widget) {
return widget.factory ?? widget.controller;
}

SheetExtent _createExtent(SheetExtentFactory factory) {
return factory.createSheetExtent(
context: this,
config: widget.config,
delegate: widget.delegate,
);

return switch (widget.initializer) {
null => extent,
final initializer => initializer(extent),
};
}

void _discard(SheetExtent extent) {
Expand All @@ -741,24 +799,35 @@ class SheetExtentScopeState extends State<SheetExtentScope>
}

void _invalidateExtentOwnership() {
assert(_debugAssertExtentOwnership());

if (widget.isPrimary) {
widget.controller.attach(_extent);
} else {
widget.controller.detach(_extent);
}
}

bool _debugAssertExtentOwnership() {
assert(
() {
final parentScope = context
.dependOnInheritedWidgetOfExactType<_InheritedSheetExtentScope>();
return !widget.isPrimary ||
if (!widget.isPrimary ||
parentScope == null ||
!parentScope.isPrimary;
!parentScope.isPrimary) {
return true;
}

throw FlutterError(
'Nesting SheetExtentScope widgets that are marked as primary '
'is not allowed. Typically, this error occurs when you try to nest '
'sheet widgets such as DraggableSheet or ScrollableSheet.',
);
}(),
'Nesting SheetExtentScope widgets that are marked as primary '
'is not allowed. Typically, this error occurs when you try to nest '
'sheet widgets such as DraggableSheet or ScrollableSheet.',
);

if (widget.isPrimary) {
widget.controller.attach(_extent);
} else {
widget.controller.detach(_extent);
}
return true;
}

@override
Expand Down
Loading

0 comments on commit def84c8

Please sign in to comment.