Skip to content

Commit

Permalink
Fix memory leaks in navigation rail (#146988)
Browse files Browse the repository at this point in the history
  • Loading branch information
@ValentinVignal
ValentinVignal authored Apr 18, 2024
1 parent fb110b9 commit cbf35b4
Show file tree
Hide file tree
Showing 2 changed files with 104 additions and 65 deletions.
163 changes: 99 additions & 64 deletions packages/flutter/lib/src/material/navigation_rail.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,7 @@ class _NavigationRailState extends State<NavigationRail> with TickerProviderStat
late List<AnimationController> _destinationControllers;
late List<Animation<double>> _destinationAnimations;
late AnimationController _extendedController;
late Animation<double> _extendedAnimation;
late CurvedAnimation _extendedAnimation;

@override
void initState() {
Expand Down Expand Up @@ -488,6 +488,8 @@ class _NavigationRailState extends State<NavigationRail> with TickerProviderStat
controller.dispose();
}
_extendedController.dispose();
_extendedAnimation.dispose();

}

void _initControllers() {
Expand Down Expand Up @@ -528,8 +530,8 @@ class _NavigationRailState extends State<NavigationRail> with TickerProviderStat
}
}

class _RailDestination extends StatelessWidget {
_RailDestination({
class _RailDestination extends StatefulWidget {
const _RailDestination({
required this.minWidth,
required this.minExtendedWidth,
required this.icon,
Expand All @@ -547,11 +549,7 @@ class _RailDestination extends StatelessWidget {
this.indicatorColor,
this.indicatorShape,
this.disabled = false,
}) : _positionAnimation = CurvedAnimation(
parent: ReverseAnimation(destinationAnimation),
curve: Curves.easeInOut,
reverseCurve: Curves.easeInOut.flipped,
);
});

final double minWidth;
final double minExtendedWidth;
Expand All @@ -571,111 +569,148 @@ class _RailDestination extends StatelessWidget {
final ShapeBorder? indicatorShape;
final bool disabled;

final Animation<double> _positionAnimation;

@override
State<_RailDestination> createState() => _RailDestinationState();
}

class _RailDestinationState extends State<_RailDestination> {
late CurvedAnimation _positionAnimation;

@override
void initState() {
super.initState();
_setPositionAnimation();
}

@override
void didUpdateWidget(_RailDestination oldWidget) {
super.didUpdateWidget(oldWidget);
if (widget.destinationAnimation != oldWidget.destinationAnimation) {
_positionAnimation.dispose();
_setPositionAnimation();
}
}

void _setPositionAnimation() {
_positionAnimation = CurvedAnimation(
parent: ReverseAnimation(widget.destinationAnimation),
curve: Curves.easeInOut,
reverseCurve: Curves.easeInOut.flipped,
);
}

@override
void dispose() {
_positionAnimation.dispose();
super.dispose();
}



@override
Widget build(BuildContext context) {
assert(
useIndicator || indicatorColor == null,
widget.useIndicator || widget.indicatorColor == null,
'[NavigationRail.indicatorColor] does not have an effect when [NavigationRail.useIndicator] is false',
);

final ThemeData theme = Theme.of(context);
final TextDirection textDirection = Directionality.of(context);
final bool material3 = theme.useMaterial3;
final EdgeInsets destinationPadding = (padding ?? EdgeInsets.zero).resolve(textDirection);
final EdgeInsets destinationPadding = (widget.padding ?? EdgeInsets.zero).resolve(textDirection);
Offset indicatorOffset;
bool applyXOffset = false;

final Widget themedIcon = IconTheme(
data: disabled
? iconTheme.copyWith(color: theme.colorScheme.onSurface.withOpacity(0.38))
: iconTheme,
child: icon,
data: widget.disabled
? widget.iconTheme.copyWith(color: theme.colorScheme.onSurface.withOpacity(0.38))
: widget.iconTheme,
child: widget.icon,
);
final Widget styledLabel = DefaultTextStyle(
style: disabled
? labelTextStyle.copyWith(color: theme.colorScheme.onSurface.withOpacity(0.38))
: labelTextStyle,
child: label,
style: widget.disabled
? widget.labelTextStyle.copyWith(color: theme.colorScheme.onSurface.withOpacity(0.38))
: widget.labelTextStyle,
child: widget.label,
);

Widget content;

// The indicator height is fixed and equal to _kIndicatorHeight.
// When the icon height is larger than the indicator height the indicator
// vertical offset is used to vertically center the indicator.
final bool isLargeIconSize = iconTheme.size != null && iconTheme.size! > _kIndicatorHeight;
final double indicatorVerticalOffset = isLargeIconSize ? (iconTheme.size! - _kIndicatorHeight) / 2 : 0;
final bool isLargeIconSize = widget.iconTheme.size != null && widget.iconTheme.size! > _kIndicatorHeight;
final double indicatorVerticalOffset = isLargeIconSize ? (widget.iconTheme.size! - _kIndicatorHeight) / 2 : 0;

switch (labelType) {
switch (widget.labelType) {
case NavigationRailLabelType.none:
// Split the destination spacing across the top and bottom to keep the icon centered.
final Widget? spacing = material3 ? const SizedBox(height: _verticalDestinationSpacingM3 / 2) : null;
indicatorOffset = Offset(
minWidth / 2 + destinationPadding.left,
widget.minWidth / 2 + destinationPadding.left,
_verticalDestinationSpacingM3 / 2 + destinationPadding.top + indicatorVerticalOffset,
);
final Widget iconPart = Column(
children: <Widget>[
if (spacing != null) spacing,
SizedBox(
width: minWidth,
height: material3 ? null : minWidth,
width: widget.minWidth,
height: material3 ? null : widget.minWidth,
child: Center(
child: _AddIndicator(
addIndicator: useIndicator,
indicatorColor: indicatorColor,
indicatorShape: indicatorShape,
addIndicator: widget.useIndicator,
indicatorColor: widget.indicatorColor,
indicatorShape: widget.indicatorShape,
isCircular: !material3,
indicatorAnimation: destinationAnimation,
indicatorAnimation: widget.destinationAnimation,
child: themedIcon,
),
),
),
if (spacing != null) spacing,
],
);
if (extendedTransitionAnimation.value == 0) {
if (widget.extendedTransitionAnimation.value == 0) {
content = Padding(
padding: padding ?? EdgeInsets.zero,
padding: widget.padding ?? EdgeInsets.zero,
child: Stack(
children: <Widget>[
iconPart,
// For semantics when label is not showing,
SizedBox.shrink(
child: Visibility.maintain(
visible: false,
child: label,
child: widget.label,
),
),
],
),
);
} else {
final Animation<double> labelFadeAnimation = extendedTransitionAnimation.drive(CurveTween(curve: const Interval(0.0, 0.25)));
final Animation<double> labelFadeAnimation = widget.extendedTransitionAnimation.drive(CurveTween(curve: const Interval(0.0, 0.25)));
applyXOffset = true;
content = Padding(
padding: padding ?? EdgeInsets.zero,
padding: widget.padding ?? EdgeInsets.zero,
child: ConstrainedBox(
constraints: BoxConstraints(
minWidth: lerpDouble(minWidth, minExtendedWidth, extendedTransitionAnimation.value)!,
minWidth: lerpDouble(widget.minWidth, widget.minExtendedWidth, widget.extendedTransitionAnimation.value)!,
),
child: ClipRect(
child: Row(
children: <Widget>[
iconPart,
Align(
heightFactor: 1.0,
widthFactor: extendedTransitionAnimation.value,
widthFactor: widget.extendedTransitionAnimation.value,
alignment: AlignmentDirectional.centerStart,
child: FadeTransition(
alwaysIncludeSemantics: true,
opacity: labelFadeAnimation,
child: styledLabel,
),
),
SizedBox(width: _horizontalDestinationPadding * extendedTransitionAnimation.value),
SizedBox(width: _horizontalDestinationPadding * widget.extendedTransitionAnimation.value),
],
),
),
Expand All @@ -685,42 +720,42 @@ class _RailDestination extends StatelessWidget {
case NavigationRailLabelType.selected:
final double appearingAnimationValue = 1 - _positionAnimation.value;
final double verticalPadding = lerpDouble(_verticalDestinationPaddingNoLabel, _verticalDestinationPaddingWithLabel, appearingAnimationValue)!;
final Interval interval = selected ? const Interval(0.25, 0.75) : const Interval(0.75, 1.0);
final Animation<double> labelFadeAnimation = destinationAnimation.drive(CurveTween(curve: interval));
final double minHeight = material3 ? 0 : minWidth;
final Interval interval = widget.selected ? const Interval(0.25, 0.75) : const Interval(0.75, 1.0);
final Animation<double> labelFadeAnimation = widget.destinationAnimation.drive(CurveTween(curve: interval));
final double minHeight = material3 ? 0 : widget.minWidth;
final Widget topSpacing = SizedBox(height: material3 ? 0 : verticalPadding);
final Widget labelSpacing = SizedBox(height: material3 ? lerpDouble(0, _verticalIconLabelSpacingM3, appearingAnimationValue)! : 0);
final Widget bottomSpacing = SizedBox(height: material3 ? _verticalDestinationSpacingM3 : verticalPadding);
final double indicatorHorizontalPadding = (destinationPadding.left / 2) - (destinationPadding.right / 2);
final double indicatorVerticalPadding = destinationPadding.top;
indicatorOffset = Offset(
minWidth / 2 + indicatorHorizontalPadding,
widget.minWidth / 2 + indicatorHorizontalPadding,
indicatorVerticalPadding + indicatorVerticalOffset,
);
if (minWidth < _NavigationRailDefaultsM2(context).minWidth!) {
if (widget.minWidth < _NavigationRailDefaultsM2(context).minWidth!) {
indicatorOffset = Offset(
minWidth / 2 + _horizontalDestinationSpacingM3,
widget.minWidth / 2 + _horizontalDestinationSpacingM3,
indicatorVerticalPadding + indicatorVerticalOffset,
);
}
content = Container(
constraints: BoxConstraints(
minWidth: minWidth,
minWidth: widget.minWidth,
minHeight: minHeight,
),
padding: padding ?? const EdgeInsets.symmetric(horizontal: _horizontalDestinationPadding),
padding: widget.padding ?? const EdgeInsets.symmetric(horizontal: _horizontalDestinationPadding),
child: ClipRect(
child: Column(
mainAxisSize: MainAxisSize.min,
mainAxisAlignment: MainAxisAlignment.center,
children: <Widget>[
topSpacing,
_AddIndicator(
addIndicator: useIndicator,
indicatorColor: indicatorColor,
indicatorShape: indicatorShape,
addIndicator: widget.useIndicator,
indicatorColor: widget.indicatorColor,
indicatorShape: widget.indicatorShape,
isCircular: false,
indicatorAnimation: destinationAnimation,
indicatorAnimation: widget.destinationAnimation,
child: themedIcon,
),
labelSpacing,
Expand All @@ -740,37 +775,37 @@ class _RailDestination extends StatelessWidget {
),
);
case NavigationRailLabelType.all:
final double minHeight = material3 ? 0 : minWidth;
final double minHeight = material3 ? 0 : widget.minWidth;
final Widget topSpacing = SizedBox(height: material3 ? 0 : _verticalDestinationPaddingWithLabel);
final Widget labelSpacing = SizedBox(height: material3 ? _verticalIconLabelSpacingM3 : 0);
final Widget bottomSpacing = SizedBox(height: material3 ? _verticalDestinationSpacingM3 : _verticalDestinationPaddingWithLabel);
final double indicatorHorizontalPadding = (destinationPadding.left / 2) - (destinationPadding.right / 2);
final double indicatorVerticalPadding = destinationPadding.top;
indicatorOffset = Offset(
minWidth / 2 + indicatorHorizontalPadding,
widget.minWidth / 2 + indicatorHorizontalPadding,
indicatorVerticalPadding + indicatorVerticalOffset,
);
if (minWidth < _NavigationRailDefaultsM2(context).minWidth!) {
if (widget.minWidth < _NavigationRailDefaultsM2(context).minWidth!) {
indicatorOffset = Offset(
minWidth / 2 + _horizontalDestinationSpacingM3,
widget.minWidth / 2 + _horizontalDestinationSpacingM3,
indicatorVerticalPadding + indicatorVerticalOffset,
);
}
content = Container(
constraints: BoxConstraints(
minWidth: minWidth,
minWidth: widget.minWidth,
minHeight: minHeight,
),
padding: padding ?? const EdgeInsets.symmetric(horizontal: _horizontalDestinationPadding),
padding: widget.padding ?? const EdgeInsets.symmetric(horizontal: _horizontalDestinationPadding),
child: Column(
children: <Widget>[
topSpacing,
_AddIndicator(
addIndicator: useIndicator,
indicatorColor: indicatorColor,
indicatorShape: indicatorShape,
addIndicator: widget.useIndicator,
indicatorColor: widget.indicatorColor,
indicatorShape: widget.indicatorShape,
isCircular: false,
indicatorAnimation: destinationAnimation,
indicatorAnimation: widget.destinationAnimation,
child: themedIcon,
),
labelSpacing,
Expand All @@ -791,15 +826,15 @@ class _RailDestination extends StatelessWidget {
: colors.primary.withOpacity(0.04);
return Semantics(
container: true,
selected: selected,
selected: widget.selected,
child: Stack(
children: <Widget>[
Material(
type: MaterialType.transparency,
child: _IndicatorInkWell(
onTap: disabled ? null : onTap,
borderRadius: BorderRadius.all(Radius.circular(minWidth / 2.0)),
customBorder: indicatorShape,
onTap: widget.disabled ? null : widget.onTap,
borderRadius: BorderRadius.all(Radius.circular(widget.minWidth / 2.0)),
customBorder: widget.indicatorShape,
splashColor: effectiveSplashColor,
hoverColor: effectiveHoverColor,
useMaterial3: material3,
Expand All @@ -810,7 +845,7 @@ class _RailDestination extends StatelessWidget {
),
),
Semantics(
label: indexLabel,
label: widget.indexLabel,
),
],
),
Expand Down
6 changes: 5 additions & 1 deletion packages/flutter/test/material/navigation_rail_theme_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import 'package:flutter/material.dart';
import 'package:flutter/rendering.dart';
import 'package:flutter_test/flutter_test.dart';
import 'package:leak_tracker_flutter_testing/leak_tracker_flutter_testing.dart';

void main() {
test('copyWith, ==, hashCode basics', () {
Expand Down Expand Up @@ -145,7 +146,10 @@ void main() {
expect(_indicatorDecoration(tester)?.shape, indicatorShape);
});

testWidgets('NavigationRail values take priority over NavigationRailThemeData values when both properties are specified', (WidgetTester tester) async {
testWidgets('NavigationRail values take priority over NavigationRailThemeData values when both properties are specified',
// TODO(polina-c): remove when fixed https://github.com/flutter/flutter/issues/145600 [leak-tracking-opt-in]
experimentalLeakTesting: LeakTesting.settings.withTracked(classes: const <String>['CurvedAnimation']),
(WidgetTester tester) async {
const Color backgroundColor = Color(0x00000001);
const double elevation = 7.0;
const double selectedIconSize = 25.0;
Expand Down

0 comments on commit cbf35b4

Please sign in to comment.