Skip to content

Add flag to use adjusted value for newIndex in onReorder callback for ReorderableList widgets#93146

Closed
markusaksli-nc wants to merge 2 commits intoflutter:masterfrom
NevercodeHQ:reorderable-index-fix
Closed

Add flag to use adjusted value for newIndex in onReorder callback for ReorderableList widgets#93146
markusaksli-nc wants to merge 2 commits intoflutter:masterfrom
NevercodeHQ:reorderable-index-fix

Conversation

@markusaksli-nc
Copy link
Contributor

@markusaksli-nc markusaksli-nc commented Nov 5, 2021 •
edited
Loading

Adds an opt-in flag to use the adjusted value for the newIndex in onReorder for ReorderableListView and the underlying SliverReorderableList. Starting off with opt-in since this will be a breaking change (see #59103)

From gallery reorderable_list_demo.dart:

  void _onReorder(int oldIndex, int newIndex) {
    setState(() {
      // This part becomes unnecessary
      // if (newIndex > oldIndex) {
      //   newIndex -= 1;
      // }
      final _ListItem item = _items.removeAt(oldIndex);
      _items.insert(newIndex, item);
    });
  }

Starts off the breaking change to fix #24786

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.

@flutter-dashboard flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Nov 5, 2021
@google-cla google-cla bot added the cla: yes label Nov 5, 2021
@markusaksli-nc
Copy link
Contributor Author

markusaksli-nc commented Nov 5, 2021

I haven't done the guide yet, first just let me know if I'm starting this off right since it's my first breaking change.

@HansMuller HansMuller requested review from darrenaustin and removed request for HansMuller November 16, 2021 17:26
@HansMuller
Copy link
Contributor

HansMuller commented Feb 8, 2022 •
edited
Loading

@markusaksli-nc - the 3 1/2 year old issue that this PR would fix is embarrassing for both its age and the triviality of the obvious remedy. The original widget (#18374) had a long gestation period and the onReorder callback problem (#24786) which was reported about 3 months later, was not dealt with promptly. The approach you've suggested here, allowing developers to opt-in to the fix, does make sense, but we don't have a way to automatically update code (via FlutterFix) that works around the problem per the original documentation. That means that we'd have to continue to support the opt-in flag forever, which - IMHO - complicates everything just a little more than the fix is worth.

There are other ways to handle this, e.g. we could add an additional callback, but having two almost identical callbacks doesn't seem like an overall improvement either.

Note also: I apologize for taking so long to review this.

@HansMuller HansMuller requested review from HansMuller and removed request for darrenaustin February 8, 2022 00:28
@markusaksli-nc
Copy link
Contributor Author

markusaksli-nc commented Feb 8, 2022

I had this concern back when I looked at this issue (although I didn't think it was that hopeless). At this point is there even a viable way to get this fix in? Nothing comes to mind right away other than the flag (which seems pointless without migration) or another callback.

@HansMuller
Copy link
Contributor

HansMuller commented Feb 8, 2022

It's a shame, but I think - at this point in time - it's best to leave this issue as it is.

@HansMuller HansMuller mentioned this pull request Feb 8, 2022
Closed
@toysonlocation
Copy link

toysonlocation commented Feb 8, 2022

Personally I think this is like saying that because we accidentally reversed 1 and 0, from henceforth the number line shall be 1, 0, 2, 3, 4, 5, 6, 7, 8, 9. It's a VERY common use item, so it should be corrected for the future of Flutter, which will be much longer than its past, right? I hope? Do you not expect BETTER things to come?

How about advancing the language version just for this, since it's so freaking common a function? Bump it to 2.5, BREAKING, like was done to 2.0. Why is that a problem? Fix this for the future, don't hobble it for the past.

@TahaTesser TahaTesser deleted the reorderable-index-fix branch April 21, 2022 07:54
@1AlexFix1
Copy link

1AlexFix1 commented Jul 18, 2022

official crutches....

@HansMuller HansMuller mentioned this pull request May 31, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@HansMuller HansMuller Awaiting requested review from HansMuller

Assignees

No one assigned

Labels

f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ReorderableListView#onReorder passes an unexpected new index

4 participants

@markusaksli-nc @HansMuller @toysonlocation @1AlexFix1