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

Rewrite onDrop handler to handle multi files #49

Closed
wants to merge 1 commit into from

Conversation

lasys
Copy link

@lasys lasys commented Jan 5, 2022

Currently when onDrop is called each file will be handled separately.
My suggestion is that onDrop returns an array which contains all dropped files.

@deakjahn
Copy link
Owner

deakjahn commented Jan 5, 2022

The idea is sane but it won't work this way. Being a federated plugin with a platform interface, breaking changes are very much discouraged. So, first we would need to change the interface, then adapt the rest. And even that only in a way that doesn't break current functionality.

So, I would suggest to leave onDrop as it is, to add a new onDropMultiple beside it (with all the underlying necessities like DropzoneDropMultipleEvent and more). Then everybody can decide which event to subscribe to and nobody has to change existing code.

@@ -168,8 +169,8 @@ class DropzoneHoverEvent extends DropzoneEvent {
}

/// Event called when the user drops a file onto the dropzone.
class DropzoneDropEvent extends DropzoneEvent<dynamic> {
DropzoneDropEvent(int viewId, dynamic file) : super(viewId, file);
class DropzoneDropEvent extends DropzoneEvent<List<dynamic>> {
Copy link
Owner

@deakjahn deakjahn Jan 5, 2022

Choose a reason for hiding this comment

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

/// Event called when the user drops multiple files onto the dropzone.
class DropzoneDropMultipleEvent extends DropzoneEvent<List<dynamic>> {
  DropzoneDropMultipleEvent(int viewId, List<dynamic> files) : super(viewId, files);
}
/// Event called when the user drops multiple files onto the dropzone.
Stream<DropzoneDropMultipleEvent> onDropMultiple({required int viewId}) {
  return events.stream //
     .where((event) => event.viewId == viewId && event is DropzoneDropMultipleEvent)
      .cast<DropzoneDropMultipleEvent>();
}

@@ -129,7 +129,7 @@ class FlutterDropzoneView {

void _onHover(MouseEvent event) => FlutterDropzonePlatform.instance.events.add(DropzoneHoverEvent(viewId));

void _onDrop(MouseEvent event, File data) => FlutterDropzonePlatform.instance.events.add(DropzoneDropEvent(viewId, data));
void _onDrop(MouseEvent event, List<dynamic> data) => FlutterDropzonePlatform.instance.events.add(DropzoneDropEvent(viewId, data));
Copy link
Owner

Choose a reason for hiding this comment

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

void _onDropMultiple(MouseEvent event, List<File> data) => FlutterDropzonePlatform.instance.events.add(DropzoneDropMultipleEvent(viewId, data));

But then again, we can't add this before the interface is modified and uploaded with a new version. This takes more than one step.

deakjahn added a commit that referenced this pull request Jan 7, 2022
deakjahn added a commit that referenced this pull request Jan 7, 2022
@deakjahn
Copy link
Owner

deakjahn commented Jan 7, 2022

OK, I changed the interface a little bit (version 2.0.5), let's follow from here.

deakjahn added a commit that referenced this pull request Jan 7, 2022
@deakjahn
Copy link
Owner

deakjahn commented Jan 7, 2022

@lasys because this change goes against yours, you probably want to pull the new stuff first and experiment with that. It seems to work for me all right but let's test if before publishing. :-)

@lasys
Copy link
Author

lasys commented Jan 7, 2022

Thank you for the quick response, support and implementation! Great work!
You are right. It is much better to let the developer decide whether to process one file or multiple files.

I skimmed the code and it looks good to me, but to be on the safe side, I will integrate and test your new code on Monday and give you feedback then.

@lasys
Copy link
Author

lasys commented Jan 10, 2022

@deakjahn I just checked your new code and it works fine!
One small suggestion: When someone adds onDrop and onDropMultiple, both handlers are executed.
Maybe it is more intuitive that if more than one file is dropped and onDropMultiple is set, then only this method is called and the other way around.

@deakjahn
Copy link
Owner

deakjahn commented Jan 10, 2022

Well, I don't know. And if they want to have both? Who knows? :-)

@lasys
Copy link
Author

lasys commented Jan 10, 2022

Nobody knows :D

So from my side, it fits and it would be great, if you release a new version.

Thanks again!

deakjahn added a commit that referenced this pull request Jan 10, 2022
deakjahn added a commit that referenced this pull request Jan 10, 2022
@deakjahn
Copy link
Owner

I think I have it.

@deakjahn deakjahn closed this Jan 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants