Cleanup Swiper Code and Have Controller Expose Swipe Position

caseycrogers commented 10 months ago

Video of the example app post changes: https://github.com/appinioGmbH/flutter_packages/assets/10675231/bcb2fef9-8309-4682-893c-29698aac8650 Excuse the weird jumpy artifacts. That's from my video recording freaking out, it's not visible in the app.

I'm going to use Appinio Swiper for my app. I wanted to add some features to it and I refactored code as I went. This kept going until I just did a more or less full rewrite of the internal functions of the swiper. I've tried to keep external behavior as consistent as possible with previous swiper behavior, but there may be a couple things I've missed.

Features added:

The controller now exposes all relevant card state (index, card offset+rotation, etc)
The controller now has animateTo to arbitrarily animate the top card's offset

High level changes made:

The index is now only ever changed after an activity is completely finished
Added more documentation to new and existing public methods/rewrite existing docs to match flutter/flutter conventions
Lots of minor formatting here and there, like avoiding complex ternaries
Avoid redundant state variables as much as possible-replace them with getters
Related to above, switched to a Position + attach system for communicating between the controller and the widget state. This mimics flutter/flutter conventions for things like ScrollController, allows for bidirectional controller<>state communication, and allowed me to remove any cases where an action was triggered via magic number

The resulting code should be much easier to read/maintain! And it lays groundwork for future features.

I know this is a huge rewrite, so I get the PR is a bit overwhelming. I hope you guys will consider it. I was quite aggressive with a lot of changes so I'm happy to chat and dial back some of the things I did if it's what it takes to get the PR merged!

I'm seeing this PR, not sure if its still alive: https://github.com/appinioGmbH/flutter_packages/pull/160 I'll get some pretty nasty merge conflicts with it. But lets cross that bridge when we come to it!

khanmujeeb687 commented 10 months ago

Hey @caseycrogers , Thank you so much for your contribution. I will review your PR today, it's great that you have already merged it with this one. Thanks!

caseycrogers commented 10 months ago

Hey @caseycrogers , Thank you so much for your contribution. I will review your PR today, it's great that you have already merged it with this one. Thanks!

Actually I haven't yet merged it with the one you referenced, happy to do so before you review I'd you'd like. Though I suspect the merge conflicts are so bad that I'd be better off just hand copying the changes over.

khanmujeeb687 commented 10 months ago

Hey @caseycrogers , Thank you so much for your contribution. I will review your PR today, it's great that you have already merged it with this one. Thanks!

Actually I haven't yet merged it with the one you referenced, happy to do so before you review I'd you'd like. Though I suspect the merge conflicts are so bad that I'd be better off just hand copying the changes over.

Yeah there could be merge conflicts, but these are small changes so maybe you can either merge them or copy the changes. And then we can review and move forward with a new version when its ready.

khanmujeeb687 commented 10 months ago

Hey @caseycrogers , is this PR ready for merge?

caseycrogers commented 10 months ago

Hey sorry working on this now. Will have it ready in 1-2 hours.

khanmujeeb687 commented 10 months ago

Hey @caseycrogers , thank you so much! That would be great!

caseycrogers commented 10 months ago

Apologies for the delay. I have manually copied over content from #160. I did not do a merge because the merge conflicts were complicated enough that a manual job seemed easier.

This PR is ready for review and merging!

One note: I did not very thoroughly update the readme according to the changes here as I figured you all might want more control over that process and/or you might request changes of this PR that'd change the readme and I didn't want to waste work, so that still needs to be done.

khanmujeeb687 commented 10 months ago

Apologies for the delay. I have manually copied over content from #160. I did not do a merge because the merge conflicts were complicated enough that a manual job seemed easier.

This PR is ready for review and merging!

One note: I did not very thoroughly update the readme according to the changes here as I figured you all might want more control over that process and/or you might request changes of this PR that'd change the readme and I didn't want to waste work, so that still needs to be done.

Hey @caseycrogers , Thank you so much for updating this PR. I will just start the review and hopefully these changes will be available in the next version.

martin1119 commented 10 months ago

@khanmujeeb687 Do you know when the new version will be published? I saw that there will be a lot of useful features in it, so I'm really excited about it. Do you have an approximate date? There are some important updates I have to make so it would be really helpful if you could give me an approximate date.

khanmujeeb687 commented 10 months ago

@khanmujeeb687 Do you know when the new version will be published? I saw that there will be a lot of useful features in it, so I'm really excited about it. Do you have an approximate date? There are some important updates I have to make so it would be really helpful if you could give me an approximate date.

Hey @martin1119 , New version will be available this week. I am also excited for this to get released but for a nice version upgrade we have to review and test everything.This PR is in review and since this has so many breaking changes and also some features were lost in this PR so I am right now adding them back and looking forward to release a new version this week. Thanks!

martin1119 commented 10 months ago

@khanmujeeb687 Hi, could you please provide an update on when you expect to release the new version? I understand that such updates can be delayed, so it would be very helpful if you could inform me about the expected release date. This will allow me to reorganize my app development accordingly. Thank you very much, and have a nice day!

caseycrogers commented 10 months ago

Would love to see this in too! I have a couple features I'd love to add (biggest one is I want the ability to animate in the new card when card count increases) but I'm waiting until this is in to do so.

@khanmujeeb687 I defaulted to being pretty opinionated with my PR. As such I removed a handful of features that were complicating the code and didn't make sense anyway (eg the swiper ever rejecting an undo-the dev is in full control of when undos are triggered so they can just handle undo logic themselves, no point in having it internal to AppinioSwiper). But I get how you guys may not want to go that route-happy to jump back on this a bit and add back in any specific things I took out that you all want back.

khanmujeeb687 commented 10 months ago

Hey @martin1119 , @caseycrogers new version of Appinio swiper has been published.

martin1119 commented 10 months ago

@khanmujeeb687 Awesome. You're amazing!

appinioGmbH / flutter_packages

Cleanup Swiper Code and Have Controller Expose Swipe Position #175