Skip to content

Conversation

@Amir-P
Copy link
Contributor

@Amir-P Amir-P commented Jan 13, 2024

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 and keepAlive bucket in addition to children list to determine whether child belongs to this render object or not.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@Amir-P Amir-P changed the title Fixes assertion failure when reordering two dimensional children Fix assertion failure when reordering two dimensional children Jan 13, 2024
@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: scrolling Viewports, list views, slivers, etc. labels Jan 13, 2024
@goderbauer goderbauer requested a review from Piinks January 16, 2024 21:12
Copy link
Contributor

@Piinks Piinks left a 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!

@Amir-P Amir-P force-pushed the fix/two_dimensional_reordering branch from c9f3eff to 1bea9f0 Compare January 18, 2024 07:13
@Amir-P
Copy link
Contributor Author

Amir-P commented Jan 18, 2024

@Piinks Could you please take a look again? Also do you think we should include #130754 as issues this PR fixes?

@Piinks
Copy link
Contributor

Piinks commented Jan 18, 2024

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. :)

@Piinks
Copy link
Contributor

Piinks commented Jan 18, 2024

This might fix #138977 though :)

Copy link
Contributor

@Piinks Piinks left a 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. 💯

@Amir-P
Copy link
Contributor Author

Amir-P commented Jan 18, 2024

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. :)

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!
  }
}

auto-submit bot pushed a commit to flutter/packages that referenced this pull request Jan 18, 2024
This would fix the analyzer error for `two_dimensional_scrollables` after merging the flutter/flutter#141504.
@Piinks
Copy link
Contributor

Piinks commented Jan 18, 2024

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!

@Amir-P Amir-P force-pushed the fix/two_dimensional_reordering branch from 1bea9f0 to 06b0a84 Compare January 19, 2024 08:03
@Amir-P
Copy link
Contributor Author

Amir-P commented Jan 22, 2024

Hi @Piinks, were you able to test the snippet? Can we add the #130754 too and get this merged?

Copy link
Contributor

@Piinks Piinks left a 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. :)

@Amir-P Amir-P force-pushed the fix/two_dimensional_reordering branch from 06b0a84 to 1c05226 Compare January 23, 2024 06:38
@Amir-P
Copy link
Contributor Author

Amir-P commented Jan 23, 2024

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.

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?

Also - a rebase with head should clear that flutter_plugins check. :)

Done. All green now. @Piinks

@Amir-P Amir-P requested a review from Piinks January 23, 2024 06:52
@Piinks
Copy link
Contributor

Piinks commented Jan 26, 2024

Isn't the test added in this PR enough?

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.
But if we are covering that in a separate change, let's do that then! :)

Copy link
Contributor

@Piinks Piinks left a 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!

Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM stamp from a Japanese personal seal

@Piinks Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 26, 2024
@auto-submit auto-submit bot merged commit a5ad088 into flutter:master Jan 26, 2024
@Amir-P Amir-P deleted the fix/two_dimensional_reordering branch January 27, 2024 14:02
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 27, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 27, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 28, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 28, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 29, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 29, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 29, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 29, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 30, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 30, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 30, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Jan 30, 2024
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)
...
auto-submit bot pushed a commit that referenced this pull request Feb 29, 2024
Adds a test to validate state is preserved after reordering in `TwoDimensionalViewport` (reference: #141504 (review)).

- Fixes #130754
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RenderTwoDimensionalViewport.buildOrObtainChildFor throws assertion failure for a valid vicinity

3 participants