-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Fix assertion failure when reordering two dimensional children #141504
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix assertion failure when reordering two dimensional children #141504
Conversation
Piinks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for contributing @Amir-P!
packages/flutter/test/widgets/two_dimensional_viewport_test.dart
Outdated
Show resolved
Hide resolved
c9f3eff to
1bea9f0
Compare
|
I don't think this will fix #130754 This fixes a reordering issue, but I don't think it preserves the state of children when they are reordered. A good way to check this is to place a Checkbox as a child of the viewport, check it, and then reorder it. The checkbox will move, but it won't be checked anymore. Since the test here is a SizedBox, there is no state for it that we're validating in the test. :) |
|
This might fix #138977 though :) |
Piinks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM, the flutter/packages change will need to land first. 💯
I haven't checked how the reordering is handled in 1D scrolling, but using a key on built widgets for 2D viewport it's working as expected. Am I missing something? @Piinks Sample: // Copyright (c) 2019, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
import 'dart:math' as math;
import 'package:flutter/foundation.dart';
import 'package:flutter/gestures.dart';
import 'package:flutter/material.dart';
import 'package:flutter/rendering.dart';
void main() => runApp(const MyApp());
class MyApp extends StatelessWidget {
const MyApp({super.key});
@override
Widget build(BuildContext context) {
return MaterialApp(
title: 'Flutter Demo',
theme: ThemeData(
colorScheme: ColorScheme.fromSeed(seedColor: Colors.deepPurple),
useMaterial3: true,
),
scrollBehavior: const MaterialScrollBehavior().copyWith(
// Mouse dragging enabled for this demo
dragDevices: PointerDeviceKind.values.toSet(),
),
debugShowCheckedModeBanner: false,
home: const MyHomePage(title: 'Flutter Demo Home Page'),
);
}
}
class MyHomePage extends StatefulWidget {
final String title;
const MyHomePage({
Key? key,
required this.title,
}) : super(key: key);
@override
State<MyHomePage> createState() => _MyHomePageState();
}
final vicinityToKey1 = <ChildVicinity, String>{
ChildVicinity(xIndex: 1, yIndex: 1): '1',
ChildVicinity(xIndex: 1, yIndex: 2): '2',
};
final vicinityToKey2 = <ChildVicinity, String>{
ChildVicinity(xIndex: 0, yIndex: 0): '1',
ChildVicinity(xIndex: 1, yIndex: 1): '2',
};
class _MyHomePageState extends State<MyHomePage> {
late Map<ChildVicinity, String> vicinityToKey = vicinityToKey1;
@override
Widget build(BuildContext context) {
return Scaffold(
appBar: AppBar(title: Text(widget.title)),
floatingActionButton: FloatingActionButton(
onPressed: () => setState(() {
if (vicinityToKey == vicinityToKey1) {
vicinityToKey = vicinityToKey2;
} else {
vicinityToKey = vicinityToKey1;
}
}),
),
body: TwoDimensionalGridView(
diagonalDragBehavior: DiagonalDragBehavior.free,
delegate: TwoDimensionalChildBuilderDelegate(
maxXIndex: 30,
maxYIndex: 30,
addAutomaticKeepAlives: false,
addRepaintBoundaries: false,
builder: (BuildContext context, ChildVicinity vicinity) {
final key = vicinityToKey[vicinity];
if (key != null) {
return _Item(key: ValueKey(key), title: key);
}
return null;
}),
),
);
}
}
class _Item extends StatefulWidget {
final String title;
const _Item({super.key, required this.title});
@override
State<_Item> createState() => __ItemState();
}
class __ItemState extends State<_Item> {
Color color = Colors.amber[50]!;
@override
Widget build(BuildContext context) {
return InkWell(
onTap: () => setState(() {
if (color == Colors.amber[50]!) {
color = Colors.purple[50]!;
} else {
color = Colors.amber[50]!;
}
}),
child: Container(
color: color,
height: 200,
width: 200,
child: Center(child: Text(widget.title)),
),
);
}
}
class TwoDimensionalGridView extends TwoDimensionalScrollView {
const TwoDimensionalGridView({
super.key,
super.primary,
super.mainAxis = Axis.vertical,
super.verticalDetails = const ScrollableDetails.vertical(),
super.horizontalDetails = const ScrollableDetails.horizontal(),
required TwoDimensionalChildBuilderDelegate delegate,
super.cacheExtent,
super.diagonalDragBehavior = DiagonalDragBehavior.none,
super.dragStartBehavior = DragStartBehavior.start,
super.keyboardDismissBehavior = ScrollViewKeyboardDismissBehavior.manual,
super.clipBehavior = Clip.hardEdge,
}) : super(delegate: delegate);
@override
Widget buildViewport(
BuildContext context,
ViewportOffset verticalOffset,
ViewportOffset horizontalOffset,
) {
return TwoDimensionalGridViewport(
horizontalOffset: horizontalOffset,
horizontalAxisDirection: horizontalDetails.direction,
verticalOffset: verticalOffset,
verticalAxisDirection: verticalDetails.direction,
mainAxis: mainAxis,
delegate: delegate as TwoDimensionalChildBuilderDelegate,
cacheExtent: cacheExtent,
clipBehavior: clipBehavior,
);
}
}
class TwoDimensionalGridViewport extends TwoDimensionalViewport {
const TwoDimensionalGridViewport({
super.key,
required super.verticalOffset,
required super.verticalAxisDirection,
required super.horizontalOffset,
required super.horizontalAxisDirection,
required TwoDimensionalChildBuilderDelegate super.delegate,
required super.mainAxis,
super.cacheExtent,
super.clipBehavior = Clip.hardEdge,
});
@override
RenderTwoDimensionalViewport createRenderObject(BuildContext context) {
return RenderTwoDimensionalGridViewport(
horizontalOffset: horizontalOffset,
horizontalAxisDirection: horizontalAxisDirection,
verticalOffset: verticalOffset,
verticalAxisDirection: verticalAxisDirection,
mainAxis: mainAxis,
delegate: delegate as TwoDimensionalChildBuilderDelegate,
childManager: context as TwoDimensionalChildManager,
cacheExtent: cacheExtent,
clipBehavior: clipBehavior,
);
}
@override
void updateRenderObject(
BuildContext context,
RenderTwoDimensionalGridViewport renderObject,
) {
renderObject
..horizontalOffset = horizontalOffset
..horizontalAxisDirection = horizontalAxisDirection
..verticalOffset = verticalOffset
..verticalAxisDirection = verticalAxisDirection
..mainAxis = mainAxis
..delegate = delegate
..cacheExtent = cacheExtent
..clipBehavior = clipBehavior;
}
}
class RenderTwoDimensionalGridViewport extends RenderTwoDimensionalViewport {
RenderTwoDimensionalGridViewport({
required super.horizontalOffset,
required super.horizontalAxisDirection,
required super.verticalOffset,
required super.verticalAxisDirection,
required TwoDimensionalChildBuilderDelegate delegate,
required super.mainAxis,
required super.childManager,
super.cacheExtent = 10,
super.clipBehavior = Clip.hardEdge,
}) : super(delegate: delegate);
@override
TwoDimensionalViewportParentData parentDataOf(RenderBox child) {
return child.parentData! as TwoDimensionalViewportParentData;
}
@override
void layoutChildSequence() {
final double horizontalPixels = horizontalOffset.pixels;
final double verticalPixels = verticalOffset.pixels;
final double viewportWidth = viewportDimension.width + cacheExtent;
final double viewportHeight = viewportDimension.height + cacheExtent;
final TwoDimensionalChildBuilderDelegate builderDelegate =
delegate as TwoDimensionalChildBuilderDelegate;
final int maxRowIndex = builderDelegate.maxYIndex!;
final int maxColumnIndex = builderDelegate.maxXIndex!;
final int leadingColumn = math.max((horizontalPixels / 200).floor(), 0);
final int leadingRow = math.max((verticalPixels / 200).floor(), 0);
final int trailingColumn = math.min(
((horizontalPixels + viewportWidth) / 200).ceil(),
maxColumnIndex,
);
final int trailingRow = math.min(
((verticalPixels + viewportHeight) / 200).ceil(),
maxRowIndex,
);
double xLayoutOffset = (leadingColumn * 200) - horizontalOffset.pixels;
for (int column = leadingColumn; column <= trailingColumn; column++) {
double yLayoutOffset = (leadingRow * 200) - verticalOffset.pixels;
for (int row = leadingRow; row <= trailingRow; row++) {
final ChildVicinity vicinity =
ChildVicinity(xIndex: column, yIndex: row);
final RenderBox? child = buildOrObtainChildFor(vicinity);
if (child != null) {
child.layout(constraints.loosen());
// Subclasses only need to set the normalized layout offset. The super
// class adjusts for reversed axes.
parentDataOf(child).layoutOffset =
Offset(xLayoutOffset, yLayoutOffset);
}
yLayoutOffset += 200;
}
xLayoutOffset += 200;
}
// Set the min and max scroll extents for each axis.
final double verticalExtent = 200 * (maxRowIndex + 1);
verticalOffset.applyContentDimensions(
0.0,
clampDouble(
verticalExtent - viewportDimension.height, 0.0, double.infinity),
);
final double horizontalExtent = 200 * (maxColumnIndex + 1);
horizontalOffset.applyContentDimensions(
0.0,
clampDouble(
horizontalExtent - viewportDimension.width, 0.0, double.infinity),
);
// Super class handles garbage collection too!
}
} |
This would fix the analyzer error for `two_dimensional_scrollables` after merging the flutter/flutter#141504.
|
Oh interesting! Let me look at the code you provided. I really appreciate all of the work you have been putting into this! Are you interested in seeing if this fixes #138977 as well? If so, would you add a test? In 1D, the findIndexByKey in SliverChildDelegate, SliverChildBuilderDelegate, and SliverChildListDelegate handles stateful reordering. If we do not need to add more API surface though, it would be great! thanks again, and I will report back once I check out your sample code above! |
1bea9f0 to
06b0a84
Compare
Piinks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did check it out! Thank you! I agree, I think we can close #130754 with this change. However, we will need to add a test that validates that scenario. Some sample code would probably be a good idea to include in the docs as well.
Would you like to add a test/sample code? Or we can leave #130754 open for now and add those in another PR. Let me know how you would like to move forward.
Also - a rebase with head should clear that flutter_plugins check. :)
06b0a84 to
1c05226
Compare
In that case, let's leave it for another PR. But what kind of test do you want for #130754? Isn't the test added in this PR enough?
Done. All green now. @Piinks |
The test here does not validate a stateful widget being reordered in the viewport and having its state maintained before and after reordering. The children in test are SizedBoxes. |
Piinks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change LGTM for resolving #141101! Thank you!
cbracken
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Manual roll Flutter from 2f6fdf2 to ace9181 (57 revisions) Manual roll requested by [email protected] flutter/flutter@2f6fdf2...ace9181 2024-01-29 [email protected] Roll Flutter Engine from 9c3ebf67b5da to bedafa8794b6 (4 revisions) (flutter/flutter#142478) 2024-01-29 [email protected] onNavigationNotification for *App.router (flutter/flutter#142190) 2024-01-29 [email protected] Marks Mac_x64 framework_tests_misc to be unflaky (flutter/flutter#142118) 2024-01-29 [email protected] Roll Flutter Engine from 3e2b8975bd5b to 9c3ebf67b5da (1 revision) (flutter/flutter#142472) 2024-01-29 [email protected] Feat: TextField can scroll when disabled (flutter/flutter#140922) 2024-01-29 [email protected] Roll Flutter Engine from 436f91f3b06b to 3e2b8975bd5b (2 revisions) (flutter/flutter#142466) 2024-01-29 [email protected] Marks Mac_arm64 framework_tests_misc to be unflaky (flutter/flutter#142119) 2024-01-29 [email protected] Fix InputDecorationTheme copyWith fallback for iconColor (flutter/flutter#142462) 2024-01-29 [email protected] Add `SingleChildScrollView` for `NavigationRail` (flutter/flutter#137415) 2024-01-29 [email protected] [Windows Arm64] Run plugin test post-submit (flutter/flutter#141987) 2024-01-29 [email protected] Catch file system exceptions when trying to parse user-provided asset file paths (flutter/flutter#142214) 2024-01-29 [email protected] Implementing `switch` expressions in `foundation/` and `material/` (flutter/flutter#142279) 2024-01-29 [email protected] Roll Flutter Engine from bff1e46c0d65 to 436f91f3b06b (1 revision) (flutter/flutter#142455) 2024-01-29 [email protected] Opt out test from leak tracking. (flutter/flutter#142417) 2024-01-29 [email protected] Update Android minSdkVersion to 21 (flutter/flutter#142267) 2024-01-29 [email protected] Roll Flutter Engine from 3d87470655b1 to bff1e46c0d65 (1 revision) (flutter/flutter#142446) 2024-01-29 [email protected] Roll Packages from cbe8100 to 516648a (3 revisions) (flutter/flutter#142445) 2024-01-29 [email protected] Roll Flutter Engine from 1405cb7b6e74 to 3d87470655b1 (1 revision) (flutter/flutter#142444) 2024-01-29 [email protected] Roll Flutter Engine from 3e65f1720a6f to 1405cb7b6e74 (1 revision) (flutter/flutter#142432) 2024-01-29 [email protected] Roll Flutter Engine from 210ed1dfb8cf to 3e65f1720a6f (1 revision) (flutter/flutter#142429) 2024-01-29 [email protected] Roll Flutter Engine from f15cb86d31c3 to 210ed1dfb8cf (1 revision) (flutter/flutter#142426) 2024-01-29 [email protected] Roll Flutter Engine from f3d48be76630 to f15cb86d31c3 (11 revisions) (flutter/flutter#142425) 2024-01-27 [email protected] Roll Flutter Engine from 95e9a15fd909 to f3d48be76630 (1 revision) (flutter/flutter#142370) 2024-01-27 [email protected] Add no-shuffle to language_version_test.dart (flutter/flutter#142378) 2024-01-27 [email protected] Roll Flutter Engine from 2687ddb2655c to 95e9a15fd909 (8 revisions) (flutter/flutter#142369) 2024-01-27 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Roll Flutter Engine from 2687ddb2655c to 2adad88a39f4 (4 revisions)" (flutter/flutter#142366) 2024-01-27 [email protected] Roll Flutter Engine from 2687ddb2655c to 2adad88a39f4 (4 revisions) (flutter/flutter#142362) 2024-01-27 [email protected] Roll Flutter Engine from 45c06c22d5c7 to 2687ddb2655c (1 revision) (flutter/flutter#142359) 2024-01-27 [email protected] Remove suspicious constant from input decorator layout (flutter/flutter#142342) 2024-01-27 [email protected] Roll Flutter Engine from 2e32acf4c31a to 45c06c22d5c7 (2 revisions) (flutter/flutter#142353) 2024-01-27 [email protected] Roll Flutter Engine from a65a1b55e06a to 2e32acf4c31a (1 revision) (flutter/flutter#142351) 2024-01-26 [email protected] Roll Flutter Engine from 525bd7dcf7f3 to a65a1b55e06a (11 revisions) (flutter/flutter#142347) 2024-01-26 [email protected] Relands "Add runWidget to bootstrap a widget tree without a default View" (flutter/flutter#142344) 2024-01-26 [email protected] Fix assertion failure when reordering two dimensional children (flutter/flutter#141504) 2024-01-26 [email protected] Limit `fuchsia_precache` in presubmit to engine rolls (flutter/flutter#142333) 2024-01-26 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.23.1 to 3.23.2 (flutter/flutter#142345) 2024-01-26 [email protected] refactor asset bundle code to not depend on the global Cache.flutterRoot (flutter/flutter#142277) 2024-01-26 [email protected] [flutter_tools] remove await runZonedGuarded() in tests (flutter/flutter#142336) 2024-01-26 [email protected] Only use iOS 17 physical devices in staging tests (flutter/flutter#142323) 2024-01-26 [email protected] Roll deps from dart-lang/native in templates (flutter/flutter#142322) 2024-01-26 [email protected] Remove duplicate global declaration of `UserMessages` (flutter/flutter#142281) 2024-01-26 [email protected] Redistribute iOS TESTOWNERS (flutter/flutter#142315) 2024-01-26 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Add runWidget to bootstrap a widget tree without a default View" (flutter/flutter#142339) 2024-01-26 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Roll Flutter Engine from 525bd7dcf7f3 to 65d1291c3add (1 revision)" (flutter/flutter#142332) ...
Adds a test to validate state is preserved after reordering in `TwoDimensionalViewport` (reference: #141504 (review)). - Fixes #130754

It fixes assertion failure due to unstable state of children list during reordering in
RenderTwoDimensionalViewport.parentDataOf. This changes the assertion to check debug orphan list andkeepAlivebucket in addition to children list to determine whether child belongs to this render object or not.Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.