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

Full rewrite #102

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Full rewrite #102

wants to merge 3 commits into from

Conversation

iazel
Copy link

@iazel iazel commented Mar 19, 2023

Hey there, thanks for publishing this plugin, it was of great inspiration for my work.

While playing around with it for my use case, I ended up doing a full rewrite. I believe this to be a better version, because:

  • use Canvas API for the actual crop, thus removing the need for platform specific plugins. Now it runs everywhere.
  • state is externalized in a controller, which makes it quite easy to test
  • good test coverage
  • crops images at a specific size
  • impossible to alter crop area aspect ratio
  • image doesn't need to be repositioned because we prevent image to become smaller than crop area and its edges to move inside the crop area. This behavior feels more natural to me.

Please let me know if you have any questions, hope you'd like it! :)

- Externalize state
- Write tests
- Use Canvas API for actual cropping
@creativecreatorormaybenot

Hi there! I am not the author of the package. However, I suggest that you create your own new package for your rewrite.

This makes much more sense from both a maintenance and also a usage perspective. This will signal to users that this is a new, better package that has to be used differently.

Also, you will not have to wait for the maintainer to follow up.

@iazel
Copy link
Author

iazel commented Apr 14, 2023

Hi there! I am not the author of the package. However, I suggest that you create your own new package for your rewrite.

This makes much more sense from both a maintenance and also a usage perspective. This will signal to users that this is a new, better package that has to be used differently.

Also, you will not have to wait for the maintainer to follow up.

Yes, I thought about that too, but it comes with a bag of issues, like having to let people discover it. If the author likes the new version, then we all win because it will be a much smoother transition than finding a new library, and will better credit the effort they have invested in it so far :)

Also, right now I don't have much time to curate a new library, perhaps in the future I will properly publish it if this doesn't bring us anywhere.

Nonetheless, thanks for your point of view.

@lukehutch
Copy link

Hi @iazel, did you ever publish this anywhere?

Does your rewrite have the problem with Flutter 3.10 / Dart 3.0, mentioned in #106?

@lukehutch
Copy link

@iazel I tried your rewrite on Flutter 3.0.2, and I get this error:

: Error: The class 'Drag' can't be used as a mixin because it isn't a mixin class nor a mixin.
widgets.dart:68
    with TickerProviderStateMixin, Drag {
                                   ^

@iazel
Copy link
Author

iazel commented Jun 22, 2023

Hi @lukehutch , thanks for letting me know, I've updated this branch to work with Flutter 3.10. Let me know how it goes :)

About publishing as another package, I currently don't have much time to dedicate, but I may release it at the beginning of next year if @lykhonis don't want to merge it or doesn't respond

@lukehutch
Copy link

@iazel Can you please open the issue tracker on your repo? I have a few changes I may want to submit (e.g. I want to limit the crop region to a given minimum size -- currently only max zoom scale is supported as a constraint). It looks like I can submit PRs but not issues.

@iazel
Copy link
Author

iazel commented Jun 23, 2023

@lukehutch done, now it is possible to both create issues and discussions

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.

3 participants