corrected SliverGridDelegateWithMaxCrossAxisExtent and SliverGridDelegateWithFixedCrossAxisCount mainAxisExtent value #138879

hello-coder-xu · 2023-11-22T08:40:56Z

fix #138871

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.

Piinks

Hi @hello-coder-xu welcome! Thank you for contributing.

Piinks · 2023-11-29T23:27:53Z

packages/flutter/test/widgets/grid_view_test.dart

    expect(controller.position.pixels, 472.0);
  });
+
+  testWidgets('SliverGridDelegateWithFixedCrossAxisCount mainAxisExtent corrected', (WidgetTester tester) async {


Can you add to each of these tests an actual grid of children? I think in the case where childMainAxisExtent is 0 there could be a crash through a divide by zero error in SliverGridRegularTileLayout. This would only become apparent when building the grid. Right now these tests only verify the value of childMainAxisExtent. :)

Thanks for your guidance, I have added it, please check

Piinks · 2023-12-04T21:27:07Z

packages/flutter/test/widgets/grid_view_test.dart

+    ));
+    expect(tester.takeException(), isNull);
+
+    stateSetter(() => mainAxisExtent = -100);


I don't think this updates the widget. It just changes the variable value. I would expect a main axis extent of this value would throw an exception since it is not valid.

I updated, please check

Piinks · 2023-12-04T21:27:14Z

packages/flutter/test/widgets/grid_view_test.dart

+    ));
+    expect(tester.takeException(), isNull);
+
+    stateSetter(() => mainAxisExtent = -100);


Piinks · 2023-12-04T21:27:46Z

packages/flutter/test/widgets/grid_view_test.dart

+      crossAxisCount: 3,
+      mainAxisSpacing: 8,
+      crossAxisSpacing: 8,
+      mainAxisExtent: -100,


I think this should probably assert, this does not seem like a valid main axis extent.

I added assertions and adjusted the test cases

…r-xu/flutter into fix/gridview_delegate * 'fix/gridview_delegate' of https://github.com/hello-coder-xu/flutter: (162 commits) add gridview children view fix gridview delegate mainAxisExtent Enable TapRegion to detect all mouse button click (flutter#136799) [flutter_tools] build ios-frameworks: option to exclude plugin frameworks from the build (flutter#129739) Roll Flutter Engine from b87c1b32d55f to eee8aeb953df (1 revision) (flutter#139495) Add Impeller complex layout impeller benchmarks. (flutter#139490) Roll Flutter Engine from cf122461e94a to b87c1b32d55f (3 revisions) (flutter#139491) Remove InkSparkle.constantTurbulenceSeedSplashFactory usage (flutter#139481) Roll Packages from bc72d15 to 5d9874f (9 revisions) (flutter#139480) Roll Flutter Engine from cc9a72b692fc to cf122461e94a (1 revision) (flutter#139468) Roll Flutter Engine from 69cbf87f33c9 to cc9a72b692fc (1 revision) (flutter#139466) Roll Flutter Engine from 05e7f462048b to 69cbf87f33c9 (1 revision) (flutter#139453) Roll Flutter Engine from e782ab139ef2 to 05e7f462048b (1 revision) (flutter#139439) Roll Flutter Engine from 80a2fd76fd64 to e782ab139ef2 (1 revision) (flutter#139437) Roll Flutter Engine from 7ff24be43a36 to 80a2fd76fd64 (1 revision) (flutter#139436) Roll Flutter Engine from cfabe42bc0c6 to 7ff24be43a36 (1 revision) (flutter#139433) Roll Flutter Engine from f0122c32c5cc to cfabe42bc0c6 (1 revision) (flutter#139423) Roll Flutter Engine from f23c33f3831c to f0122c32c5cc (1 revision) (flutter#139422) Roll Flutter Engine from d441f087052c to f23c33f3831c (2 revisions) (flutter#139421) Roll Flutter Engine from 27d37db84b8e to d441f087052c (1 revision) (flutter#139419) ...

Piinks

Oh my, I just came back to this after the holidays to see I had not submitted my last review. Apologies for the delay here.

Piinks · 2023-12-11T21:04:42Z

packages/flutter/lib/src/rendering/sliver_grid.dart

    );
    final double childCrossAxisExtent = usableCrossAxisExtent / crossAxisCount;
-    final double childMainAxisExtent = mainAxisExtent ?? childCrossAxisExtent / childAspectRatio;
+    final double childMainAxisExtent = math.max(


Do the tests include a scenario where childMainAxisExtent is calculated to be 0? Later on, this value is used as a denominator in the layout object. Since we are making 0 an explicit option here, we should check that is does not result in a crash later.

Also, if we are now asserting mainAxisExtent is non-negative in the constructor, and it looks like childCrossAxisExtent / childAspectRatio will not be negative, do we need to add the math.max(...) at all?

It won't actually correct the mainAxisExtent as the change/issue describes now, it will just assert as it did in the issue. Can you update the PR to reflect what the expected result is now if we are not correcting the main axis extent?

Piinks · 2023-12-11T21:05:52Z

packages/flutter/lib/src/rendering/sliver_grid.dart

    this.crossAxisSpacing = 0.0,
    this.childAspectRatio = 1.0,
    this.mainAxisExtent,
-  }) : assert(crossAxisCount > 0),


Can you revert the formatting changes here? This was the proper alignment.

goderbauer · 2024-01-30T23:18:39Z

(triage) @hello-coder-xu Do you still have plans to come back and address the feedback given above soon-ish?

goderbauer · 2024-02-27T23:17:03Z

(triage): I am going to close this since there was no follow-up. Feel free to reopen this if you find the time to work on this again. We also have an issue on file (#138871) to keep track of this.

fix gridview delegate mainAxisExtent

757ceea

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: scrolling Viewports, list views, slivers, etc. labels Nov 22, 2023

LinXunFeng requested a review from chinmoy12c November 28, 2023 07:52

goderbauer requested a review from Piinks November 28, 2023 23:11

Piinks reviewed Nov 29, 2023

View reviewed changes

hello-coder-xu added 3 commits December 4, 2023 10:44

add gridview children view

f6a781f

fix gridview delegate mainAxisExtent

688149a

add gridview children view

f3161c6

Piinks force-pushed the fix/gridview_delegate branch from f6a781f to f3161c6 Compare December 4, 2023 20:54

Piinks reviewed Dec 4, 2023

View reviewed changes

hello-coder-xu and others added 5 commits December 5, 2023 20:13

Adjust test plan

ccbd7f7

change it

4f0cce9

Merge branch 'master' into fix/gridview_delegate

c862bef

verify assert

2b7a8e3

goderbauer requested a review from Piinks December 19, 2023 22:58

Piinks reviewed Jan 2, 2024

View reviewed changes

goderbauer mentioned this pull request Feb 27, 2024

mainAxisExtent of SliverGridDelegateWithFixedCrossAxisCount and SliverGridDelegateWithMaxCrossAxisExtent may trigger assert #138871

Closed

2 tasks

goderbauer closed this Feb 27, 2024

hello-coder-xu mentioned this pull request May 22, 2024

sliverGridDelegate mainAxisExtent add assert #148470

Merged

9 tasks

corrected SliverGridDelegateWithMaxCrossAxisExtent and SliverGridDelegateWithFixedCrossAxisCount mainAxisExtent value #138879

corrected SliverGridDelegateWithMaxCrossAxisExtent and SliverGridDelegateWithFixedCrossAxisCount mainAxisExtent value #138879

Uh oh!

Conversation

hello-coder-xu commented Nov 22, 2023

Pre-launch Checklist

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goderbauer commented Jan 30, 2024

Uh oh!

goderbauer commented Feb 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants