Add draggable parameter to ReorderableDragStartListener #81396

ValentinVignal · 2021-04-28T16:29:22Z

This PR is adding a parameter draggable to the widget ReorderableDragStartListener. This allows the user to dynamically activate/deactivate the drag/drop behaviour for some items in the ReorderableListView without changing the tree structure ; and therefore losing the state of the the stateful widgets inside the items of the list.

List which issues are fixed by this PR. You must list at least one issue.
#81395

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

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 feature I am adding, or Hixie said the 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.

ValentinVignal · 2021-04-28T16:31:40Z

Please let me know if I missed something

darrenaustin

Thanks so much for the contribution! I totally missed this use case when I was developing the drag listeners.

I have a few comments and suggestions below that will need to be addressed before we can merge this, but otherwise this looks good. Thanks for including tests.

Let me know if there is anything that isn't clear, or if you aren't interested in taking this further. Thanks again.

packages/flutter/lib/src/widgets/reorderable_list.dart

packages/flutter/test/widgets/reorderable_list_test.dart

…arameter # Conflicts: # packages/flutter/test/widgets/reorderable_list_test.dart

…gStartListener

ValentinVignal · 2021-05-01T15:46:12Z

@darrenaustin I applied the changes you requested, let me know if I should do something more.

Also, I am a bit confused:

For the ReorderableDragStartListener tests, I have to drag the item 0 to Offset(0, 150) to reorder it at the second position.
For the ReorderableDelayedDragStartListener tests, I have to drag the item 0 to Offset(0, 50) to reorder it at the second position.

I tried to investigate why I was having these different behaviours but I didn't found anything.
I wanted to use golden images to have more information, but for some reasons, adding

await expectLater(find.byType(ReorderableListView), matchesGoldenFile('golden.png'));

doesn't generate any image.

Do you have an explanation or could you highlight me on how to debug/investigate the test runs ?

darrenaustin

@darrenaustin I applied the changes you requested, let me know if I should do something more.

Looks good thanks. I have a small issue I missed the first time (see new comment). Also it looks like there is a merge conflict with what is on the master branch, so that will need to be resolved before we can land this.

Also, I am a bit confused:

For the ReorderableDragStartListener tests, I have to drag the item 0 to Offset(0, 150) to reorder it at the second position.

For the ReorderableDelayedDragStartListener tests, I have to drag the item 0 to Offset(0, 50) to reorder it at the second position.

That is odd. I spent a bunch time looking into this and it appears to be a bug with the drag calculations on the very first drag update event. It will be off by the size of the gap, but only for the first update. The current tests that are based off of moving the item by 150 are incorrect, and should only need to move by 50. The reason it works fine for the delayed listener is that under the covers it generates two update events on the start of the drag, and the second one uses the correct calculation. I am looking into fixing this, but go ahead and use the values you have in your tests now and if this lands before I can fix it will update them with my fix. Thanks for pointing this out!

I tried to investigate why I was having these different behaviours but I didn't found anything.
I wanted to use golden images to have more information, but for some reasons, adding
await expectLater(find.byType(ReorderableListView), matchesGoldenFile('golden.png'));

This may not give you what you want here, text in golden images are just blocks of color to avoid font changes effecting golden tests. In addition you wouldn't be able to see the dragged item as it is in an overlay, so you would have to include the top level overlay to get the full image. All that said, when you run this with a new golden image it should have given you a directory where the generated images were.

packages/flutter/test/widgets/reorderable_list_test.dart

…tart-listener/draggable-parameter # Conflicts: # packages/flutter/test/widgets/reorderable_list_test.dart

…in tests

ValentinVignal · 2021-05-05T14:46:43Z

Thank you for all those precisions and feedbacks @darrenaustin!

I applied your requests changes. I hope everything is fine now :)

Also it looks like the windows tests are failing ("download dependencies"). Is it because of my changes?

darrenaustin

Thanks for working through all the issues here.

darrenaustin · 2021-05-05T19:12:53Z

Also it looks like the windows tests are failing ("download dependencies"). Is it because of my changes?

Looks like it was an infra problem on the test machines. I have restarted the tests and hopefully they will go through this time.

ValentinVignal · 2021-05-06T01:22:36Z

Looks like it was an infra problem on the test machines. I have restarted the tests and hopefully they will go through this time.

Ok, good to know, let me know if I can do something else.

darrenaustin · 2021-05-10T20:07:06Z

Looks like it was an infra problem on the test machines. I have restarted the tests and hopefully they will go through this time.

Ok, good to know, let me know if I can do something else.

Hi @ValentinVignal, there is still a windows test that is not passing, but I think it is a test that has been updated or turned off on the master branch. Can you pull in the latest from master and rebase your stuff on top of it? I think that will finally allow us to land this. Thx.

…tart-listener/draggable-parameter

ValentinVignal · 2021-05-11T07:55:28Z

Hi @ValentinVignal, there is still a windows test that is not passing, but I think it is a test that has been updated or turned off on the master branch. Can you pull in the latest from master and rebase your stuff on top of it? I think that will finally allow us to land this. Thx.

Hello @darrenaustin it looks like the windows test is not failing anymore 👍 . Thank you for monitoring this.

Add draggable parameter to ReorderableDragStartListener

e00011e

flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Apr 28, 2021

google-cla bot added the cla: yes label Apr 28, 2021

goderbauer requested a review from darrenaustin April 28, 2021 21:43

darrenaustin suggested changes Apr 30, 2021

View reviewed changes

Merge branch 'master' into reoderable-drag-start-listener/draggable-p…

dbbe311

…arameter # Conflicts: # packages/flutter/test/widgets/reorderable_list_test.dart

ValentinVignal marked this pull request as draft May 1, 2021 14:43

draggable -> enabled - Add enabled parameter to ReorderableDelayedDra…

2e0fded

…gStartListener

ValentinVignal marked this pull request as ready for review May 1, 2021 15:39

ValentinVignal requested a review from darrenaustin May 1, 2021 15:39

ValentinVignal added 3 commits May 1, 2021 23:47

Add missing tab

2253e46

Remove trailing spaces

0e2a18e

Remove another trailing space

083f28f

darrenaustin suggested changes May 4, 2021

View reviewed changes

packages/flutter/test/widgets/reorderable_list_test.dart Outdated Show resolved Hide resolved

darrenaustin reviewed May 4, 2021

View reviewed changes

packages/flutter/test/widgets/reorderable_list_test.dart Outdated Show resolved Hide resolved

ValentinVignal added 2 commits May 5, 2021 22:28

Merge remote-tracking branch 'upstream/master' into reoderable-drag-s…

8cf9c87

…tart-listener/draggable-parameter # Conflicts: # packages/flutter/test/widgets/reorderable_list_test.dart

Use ReorderableList and specify types and names of unused parameters …

fbda674

…in tests

ValentinVignal requested a review from darrenaustin May 5, 2021 14:44

darrenaustin approved these changes May 5, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/master' into reoderable-drag-s…

e4fff53

…tart-listener/draggable-parameter

darrenaustin added the waiting for tree to go green label May 11, 2021

fluttergithubbot merged commit 43d06c8 into flutter:master May 11, 2021

darrenaustin mentioned this pull request May 11, 2021

Add a draggable parameter to the ReorderableDragStartListener to keep widget tree architecture #81395

Closed

darrenaustin mentioned this pull request May 11, 2021

Fixed a problem with the first drag update on a reorderable list. #82296

Merged

8 tasks

This was referenced Nov 24, 2021

Wrong test descriptions for the non draggable items in reorderable_list_test.dart #94166

Closed

✏️ Correct test description for a non-draggable item #94167

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add draggable parameter to ReorderableDragStartListener #81396

Add draggable parameter to ReorderableDragStartListener #81396

Uh oh!

ValentinVignal commented Apr 28, 2021 •

edited

Loading

Uh oh!

ValentinVignal commented Apr 28, 2021

Uh oh!

darrenaustin left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ValentinVignal commented May 1, 2021 •

edited

Loading

Uh oh!

darrenaustin left a comment

Uh oh!

Uh oh!

Uh oh!

ValentinVignal commented May 5, 2021 •

edited

Loading

Uh oh!

darrenaustin left a comment

Uh oh!

darrenaustin commented May 5, 2021

Uh oh!

ValentinVignal commented May 6, 2021

Uh oh!

darrenaustin commented May 10, 2021

Uh oh!

ValentinVignal commented May 11, 2021

Uh oh!

Uh oh!

Add draggable parameter to ReorderableDragStartListener #81396

Add draggable parameter to ReorderableDragStartListener #81396

Uh oh!

Conversation

ValentinVignal commented Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

ValentinVignal commented Apr 28, 2021

Uh oh!

darrenaustin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ValentinVignal commented May 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

darrenaustin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ValentinVignal commented May 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

darrenaustin left a comment

Choose a reason for hiding this comment

Uh oh!

darrenaustin commented May 5, 2021

Uh oh!

ValentinVignal commented May 6, 2021

Uh oh!

darrenaustin commented May 10, 2021

Uh oh!

ValentinVignal commented May 11, 2021

Uh oh!

Uh oh!

ValentinVignal commented Apr 28, 2021 •

edited

Loading

ValentinVignal commented May 1, 2021 •

edited

Loading

ValentinVignal commented May 5, 2021 •

edited

Loading