Skip to content
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

Add findChildIndexCallback examples #133469

Merged
merged 21 commits into from Oct 18, 2023
Merged

Conversation

gildaswise
Copy link
Contributor

@gildaswise gildaswise commented Aug 28, 2023

The documentation for using findChildIndexCallback recommends using indexOf, but that causes this line to throw in debug mode, and when using SliverList, it breaks the render.

This PR changes the usage to check if the index is not negative before using it, and changes to return null instead if the child wasn't able to be found.

There's the related issue #107123, but this doesn't actually fix it.


This PR has been updated to add the snippets that were used in the findChildIndexCallback comment as examples with proper tests, as well as updating the comment to reference the new examples.

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.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: scrolling Viewports, list views, slivers, etc. labels Aug 28, 2023
@Piinks Piinks added the d: api docs Issues with https://api.flutter.dev/ label Aug 30, 2023
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.

Hi @gildaswise thanks for contributing! It looks like we should fix the issue with indexOf returning invalid indices, rather than changing our samples to reflect a workaround. What do you think?

@gildaswise
Copy link
Contributor Author

Hey Kate, thanks for reviewing!

It looks like we should fix the issue with indexOf returning invalid indices

I didn't understand if there's something to be fixed. The findChildIndexCallback expects a nullable index as a return, and unless we change the processing to ignore negative indexes, I believe the correct usage should still be checking for the return of indexOf before returning in the callback, since it should support not finding the index.

@gildaswise
Copy link
Contributor Author

gildaswise commented Sep 1, 2023

I just noticed that the findChildIndexCallback says this:

This callback should take an input [Key], and it should return the index of the child element with that associated key, or null if not found.

So indeed the recommended callback in the documentation was wrong, it's not supposed to return -1 if not found, but null as I changed.

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.

Ah thank you for explaining. I see what you mean.

I think we should likely change these snippets to be dartpad examples. That way we can write a test for each to ensure this works as expected and does not regress in the future. Those examples live in examples/api, and are embedded using {@tool dartpad} instead of {@tool snippet}.

@gildaswise
Copy link
Contributor Author

I think we should likely change these snippets to be dartpad examples.

I can try to do this in this PR then, will look into it once I have time this week. Thank you!

@goderbauer
Copy link
Member

(triage): @gildaswise Do you still have plans to continue working on this PR in the near future?

@gildaswise
Copy link
Contributor Author

@goderbauer Sorry about the delay, been a rough few weeks for me. I'll try to add the dartpad examples this week for sure, and if I can't, I'll ping you here

@github-actions github-actions bot added the d: examples Sample code and demos label Oct 2, 2023
@gildaswise gildaswise changed the title Update findChildIndexCallback comment Add findChildIndexCallback examples Oct 2, 2023
@gildaswise gildaswise requested a review from Piinks October 2, 2023 22:35
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.

Flutter_LGTM

Thanks so much for contributing this! Converting these to dartpads make them even more helpful as interactive examples, plus the tests you added ensure they aren't accidentally broken in the future. :)

@gildaswise
Copy link
Contributor Author

Thanks so much for contributing this! Converting these to dartpads make them even more helpful as interactive examples, plus the tests you added ensure they aren't accidentally broken in the future. :)

Thank you for the review, Kate! I can't merge it myself but the CI passed now, please let me know if I need to do something else.

@Piinks
Copy link
Contributor

Piinks commented Oct 11, 2023

Just waiting on a second review for this to land, I will reach out again to see if we can get one. :)

import 'package:flutter/material.dart';

/// Flutter code sample for a [ListView.custom] using the `findChildIndexCallback` argument

Copy link
Member

Choose a reason for hiding this comment

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

nit: add a period at the end of the comment and remove the blank line so it's snug against main.

Style guide ref: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#use-correct-grammar


import 'package:flutter/material.dart';

/// Flutter code sample for a [PageView] using the `findChildIndexCallback` argument
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Add a period at the end of the comment.

Style guide ref: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#use-correct-grammar

@Piinks Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 17, 2023
@auto-submit auto-submit bot merged commit 492b6f7 into flutter:master Oct 18, 2023
62 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 18, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 18, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 18, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 18, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 18, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 18, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 18, 2023
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Oct 19, 2023
…5178)

Manual roll Flutter from 6eea6e277e56 to 189196d104e2 (44 revisions)

Manual roll requested by tarrinneal@google.com

flutter/flutter@6eea6e2...189196d

2023-10-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from ab86c53c19cd to 6caee3236d37 (2 revisions) (flutter/flutter#136834)
2023-10-18 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Reland] Skip injecting Bonjour settings when port publication is disabled" (flutter/flutter#136839)
2023-10-18 gspencergoog@users.noreply.github.com Convert menus to use OverlayPortal (flutter/flutter#130534)
2023-10-18 15619084+vashworth@users.noreply.github.com [Reland] Skip injecting Bonjour settings when port publication is disabled (flutter/flutter#136751)
2023-10-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from 0eff5d191856 to ab86c53c19cd (1 revision) (flutter/flutter#136832)
2023-10-18 47866232+chunhtai@users.noreply.github.com Fixes ability to call nextFocus() on a node to focus its descendant (flutter/flutter#136773)
2023-10-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from b778a07f8ae9 to 0eff5d191856 (6 revisions) (flutter/flutter#136829)
2023-10-18 robert.ancell@canonical.com Implement GApplication:shutdown so a Flutter developer knows where to put code that should occur on application shutdown. (flutter/flutter#136780)
2023-10-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from 7f37c9b181af to b778a07f8ae9 (1 revision) (flutter/flutter#136818)
2023-10-18 tessertaha@gmail.com Fix `Slider` `onChanged` callback order & never calls `onChangeStart` on  `SliderInteraction.slideOnly` allowed interaction (flutter/flutter#136720)
2023-10-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from 78026b4003fe to 7f37c9b181af (1 revision) (flutter/flutter#136817)
2023-10-18 838@mail.com [Feat] Stroke color for Slider value indicator (flutter/flutter#135986)
2023-10-18 76067278+aakash-pamnani@users.noreply.github.com Fixed : Empty Rows shown at last page in Paginated data table (flutter/flutter#132646)
2023-10-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from 46923fd39032 to 78026b4003fe (1 revision) (flutter/flutter#136814)
2023-10-18 engine-flutter-autoroll@skia.org Roll Packages from d439062 to 14aa69e (1 revision) (flutter/flutter#136813)
2023-10-18 dacoharkes@google.com Don't build native assets in `flutter build bundle` (flutter/flutter#136641)
2023-10-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from c9c9684e03a3 to 46923fd39032 (1 revision) (flutter/flutter#136795)
2023-10-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from 0c1c29271e8b to c9c9684e03a3 (1 revision) (flutter/flutter#136792)
2023-10-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from 1de09d13e708 to 0c1c29271e8b (3 revisions) (flutter/flutter#136789)
2023-10-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from 6fc36e61a99a to 1de09d13e708 (1 revision) (flutter/flutter#136785)
2023-10-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from 3f818efff3c5 to 6fc36e61a99a (1 revision) (flutter/flutter#136781)
2023-10-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from 2eef9b43cfb4 to 3f818efff3c5 (3 revisions) (flutter/flutter#136777)
2023-10-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from 5df7af34a718 to 2eef9b43cfb4 (4 revisions) (flutter/flutter#136774)
2023-10-18 christopherfujino@gmail.com Flutter preview device (flutter/flutter#135639)
2023-10-18 gildasiogx@gmail.com Add findChildIndexCallback examples (flutter/flutter#133469)
2023-10-18 109253501+pdblasi-google@users.noreply.github.com Adds API for performing semantics actions in tests (flutter/flutter#132598)
2023-10-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from e57b5bac4244 to 5df7af34a718 (2 revisions) (flutter/flutter#136772)
2023-10-17 engine-flutter-autoroll@skia.org Roll Flutter Engine from f9f937e51080 to e57b5bac4244 (3 revisions) (flutter/flutter#136768)
2023-10-17 andrewrkolos@gmail.com clean up `--dart-define-from-file` option tests (flutter/flutter#135980)
2023-10-17 sokolovskyi.konstantin@gmail.com GoldenFileComparators should dispose created Image objects.  (flutter/flutter#136716)
2023-10-17 lc1990linux@gmail.com make integration_test_driver_extended.dart support writeResponseData--(done) (flutter/flutter#128382)
2023-10-17 49699333+dependabot[bot]@users.noreply.github.com Bump actions/checkout from 3.6.0 to 4.1.1 (flutter/flutter#136762)
2023-10-17 engine-flutter-autoroll@skia.org Roll Flutter Engine from 289f29b1ad00 to f9f937e51080 (1 revision) (flutter/flutter#136755)
2023-10-17 engine-flutter-autoroll@skia.org Roll Flutter Engine from 659e68a097b5 to 289f29b1ad00 (1 revision) (flutter/flutter#136752)
2023-10-17 aran@arandonohue.com Support --web-header option for flutter run (flutter/flutter#136297)
2023-10-17 34871572+gmackall@users.noreply.github.com Add Android 14 physical devices to firebase tests (flutter/flutter#136736)
2023-10-17 engine-flutter-autoroll@skia.org Roll Flutter Engine from 62a90a91cee3 to 659e68a097b5 (3 revisions) (flutter/flutter#136746)
2023-10-17 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Skip injecting Bonjour settings when port publication is disabled" (flutter/flutter#136750)
2023-10-17 katelovett@google.com Revert "[SingleChildScrollView] Correct the offset pixels if it is out of range during layout" (flutter/flutter#136744)
2023-10-17 15619084+vashworth@users.noreply.github.com Skip injecting Bonjour settings when port publication is disabled (flutter/flutter#136562)
2023-10-17 engine-flutter-autoroll@skia.org Roll Flutter Engine from 3ecbe924a598 to 62a90a91cee3 (2 revisions) (flutter/flutter#136739)
2023-10-17 engine-flutter-autoroll@skia.org Roll Flutter Engine from 0a4d8b99a95b to 3ecbe924a598 (3 revisions) (flutter/flutter#136732)
2023-10-17 32242716+ricardoamador@users.noreply.github.com Reenable NexusLowRes API 29 (flutter/flutter#136686)
2023-10-17 32242716+ricardoamador@users.noreply.github.com Reenable the nexus 6p tests (flutter/flutter#136689)
...
@gildaswise gildaswise deleted the patch-1 branch October 23, 2023 14:01
HugoOlthof pushed a commit to moneybird/packages that referenced this pull request Dec 13, 2023
…lutter#5178)

Manual roll Flutter from 6eea6e277e56 to 189196d104e2 (44 revisions)

Manual roll requested by tarrinneal@google.com

flutter/flutter@6eea6e2...189196d

2023-10-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from ab86c53c19cd to 6caee3236d37 (2 revisions) (flutter/flutter#136834)
2023-10-18 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Reland] Skip injecting Bonjour settings when port publication is disabled" (flutter/flutter#136839)
2023-10-18 gspencergoog@users.noreply.github.com Convert menus to use OverlayPortal (flutter/flutter#130534)
2023-10-18 15619084+vashworth@users.noreply.github.com [Reland] Skip injecting Bonjour settings when port publication is disabled (flutter/flutter#136751)
2023-10-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from 0eff5d191856 to ab86c53c19cd (1 revision) (flutter/flutter#136832)
2023-10-18 47866232+chunhtai@users.noreply.github.com Fixes ability to call nextFocus() on a node to focus its descendant (flutter/flutter#136773)
2023-10-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from b778a07f8ae9 to 0eff5d191856 (6 revisions) (flutter/flutter#136829)
2023-10-18 robert.ancell@canonical.com Implement GApplication:shutdown so a Flutter developer knows where to put code that should occur on application shutdown. (flutter/flutter#136780)
2023-10-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from 7f37c9b181af to b778a07f8ae9 (1 revision) (flutter/flutter#136818)
2023-10-18 tessertaha@gmail.com Fix `Slider` `onChanged` callback order & never calls `onChangeStart` on  `SliderInteraction.slideOnly` allowed interaction (flutter/flutter#136720)
2023-10-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from 78026b4003fe to 7f37c9b181af (1 revision) (flutter/flutter#136817)
2023-10-18 838@mail.com [Feat] Stroke color for Slider value indicator (flutter/flutter#135986)
2023-10-18 76067278+aakash-pamnani@users.noreply.github.com Fixed : Empty Rows shown at last page in Paginated data table (flutter/flutter#132646)
2023-10-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from 46923fd39032 to 78026b4003fe (1 revision) (flutter/flutter#136814)
2023-10-18 engine-flutter-autoroll@skia.org Roll Packages from d439062 to 14aa69e (1 revision) (flutter/flutter#136813)
2023-10-18 dacoharkes@google.com Don't build native assets in `flutter build bundle` (flutter/flutter#136641)
2023-10-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from c9c9684e03a3 to 46923fd39032 (1 revision) (flutter/flutter#136795)
2023-10-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from 0c1c29271e8b to c9c9684e03a3 (1 revision) (flutter/flutter#136792)
2023-10-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from 1de09d13e708 to 0c1c29271e8b (3 revisions) (flutter/flutter#136789)
2023-10-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from 6fc36e61a99a to 1de09d13e708 (1 revision) (flutter/flutter#136785)
2023-10-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from 3f818efff3c5 to 6fc36e61a99a (1 revision) (flutter/flutter#136781)
2023-10-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from 2eef9b43cfb4 to 3f818efff3c5 (3 revisions) (flutter/flutter#136777)
2023-10-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from 5df7af34a718 to 2eef9b43cfb4 (4 revisions) (flutter/flutter#136774)
2023-10-18 christopherfujino@gmail.com Flutter preview device (flutter/flutter#135639)
2023-10-18 gildasiogx@gmail.com Add findChildIndexCallback examples (flutter/flutter#133469)
2023-10-18 109253501+pdblasi-google@users.noreply.github.com Adds API for performing semantics actions in tests (flutter/flutter#132598)
2023-10-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from e57b5bac4244 to 5df7af34a718 (2 revisions) (flutter/flutter#136772)
2023-10-17 engine-flutter-autoroll@skia.org Roll Flutter Engine from f9f937e51080 to e57b5bac4244 (3 revisions) (flutter/flutter#136768)
2023-10-17 andrewrkolos@gmail.com clean up `--dart-define-from-file` option tests (flutter/flutter#135980)
2023-10-17 sokolovskyi.konstantin@gmail.com GoldenFileComparators should dispose created Image objects.  (flutter/flutter#136716)
2023-10-17 lc1990linux@gmail.com make integration_test_driver_extended.dart support writeResponseData--(done) (flutter/flutter#128382)
2023-10-17 49699333+dependabot[bot]@users.noreply.github.com Bump actions/checkout from 3.6.0 to 4.1.1 (flutter/flutter#136762)
2023-10-17 engine-flutter-autoroll@skia.org Roll Flutter Engine from 289f29b1ad00 to f9f937e51080 (1 revision) (flutter/flutter#136755)
2023-10-17 engine-flutter-autoroll@skia.org Roll Flutter Engine from 659e68a097b5 to 289f29b1ad00 (1 revision) (flutter/flutter#136752)
2023-10-17 aran@arandonohue.com Support --web-header option for flutter run (flutter/flutter#136297)
2023-10-17 34871572+gmackall@users.noreply.github.com Add Android 14 physical devices to firebase tests (flutter/flutter#136736)
2023-10-17 engine-flutter-autoroll@skia.org Roll Flutter Engine from 62a90a91cee3 to 659e68a097b5 (3 revisions) (flutter/flutter#136746)
2023-10-17 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Skip injecting Bonjour settings when port publication is disabled" (flutter/flutter#136750)
2023-10-17 katelovett@google.com Revert "[SingleChildScrollView] Correct the offset pixels if it is out of range during layout" (flutter/flutter#136744)
2023-10-17 15619084+vashworth@users.noreply.github.com Skip injecting Bonjour settings when port publication is disabled (flutter/flutter#136562)
2023-10-17 engine-flutter-autoroll@skia.org Roll Flutter Engine from 3ecbe924a598 to 62a90a91cee3 (2 revisions) (flutter/flutter#136739)
2023-10-17 engine-flutter-autoroll@skia.org Roll Flutter Engine from 0a4d8b99a95b to 3ecbe924a598 (3 revisions) (flutter/flutter#136732)
2023-10-17 32242716+ricardoamador@users.noreply.github.com Reenable NexusLowRes API 29 (flutter/flutter#136686)
2023-10-17 32242716+ricardoamador@users.noreply.github.com Reenable the nexus 6p tests (flutter/flutter#136689)
...
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 d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos 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.

None yet

4 participants