MobileChromeApps / cordova-plugin-zip

Zip plugin for Cordova apps
Other
168 stars 207 forks source link

Progress Event Callback for Unzip #17

Closed adangel closed 10 years ago

adangel commented 10 years ago

This adds an optional event that is triggered during unzip. With this callback you can e.g. feed a progressbar widget and show the progress while unzipping.

bshepherdson commented 10 years ago

This commit LGTM in JS and Android, but I'm concerned that we're patching an upstream distribution (minizip). That should ideally be patched upstream and then integrated here. On the other hand, minizip is distributed in source form, but not exactly open source - there's no Github or similar page, just a source bundle.

So I think we're okay with patching our local version. I'll mark it clearly as having been patched from the original minizip in a follow-up commit, in keeping with the minizip license.

So, the only remaining step is for you to sign the CLA; see CONTRIBUTING.md. Also, let me know what name you used to sign it.

mmocny commented 10 years ago

Yeah @adangel this is an awesome patch, thanks a bunch!

@shepheb I found: https://github.com/nmoinvaz/minizip which maybe isn't the canonical source (?) but has been patched fairly actively.

mmocny commented 10 years ago

.. that looks like the canonical source. Its used from zLib and points to http://www.winimage.com/zLibDll/minizip.html in the docs.

mmocny commented 10 years ago

..also: cannot we just get the file size without patching the zip library? Seems we only really only need to patch the objective c wrapper: https://github.com/soffes/ssziparchive to emit the progress events in the main loop.

adangel commented 10 years ago

I'm not so fluent in objective c, so that's why I added the filesize directly into the zip library. I first tried to do it one layer above (in SSZipArchive.m) but I was not able to cast the pointer to the correct struct to execute the seek/tell on the file handle - as access to the file handle seems to be private to unzip.c... In theory it should also be possible to call the stdio seek/tell functions - if these can be used in objective c (or maybe objective c itself provides a io api?) We would need to open and close the file and only afterwards then let the file be opened by minizip.

Hm... it should be possible with NSFileHandle: https://developer.apple.com/library/ios/documentation/cocoa/reference/foundation/Classes/NSFileHandle_Class/Reference/Reference.html#//apple_ref/doc/uid/TP40003659

mmocny commented 10 years ago

Thats more or less what I am thinking: get the size before handing it off to minizip. Perhaps see: http://stackoverflow.com/questions/815471/how-to-get-the-file-size-given-a-path

Then, send the patch up to SSZipArchive (https://github.com/soffes/ssziparchive). (And if it they aren't open to accepting it, we can just patch it here).

Thanks so much for digging in to this!

adangel commented 10 years ago

Sure. Let me know, if I should change something or whether you'll do it. Btw - I've signed the CLA with "Andreas Dangel" and adangel_at_users.sourceforge.net

mmocny commented 10 years ago

@adangel I am unlikely to get to it soon. Since you've done the heavy lifting already, I think you should submit a PR to them and take the credit :)

Though, if you do not want to work on this further, let me know and I'll get to it eventually.

Thanks so much for the help!

adangel commented 10 years ago

I've updated the pull request. The filesize is now determined by using NSFileManager. Minizip can stay unmodified.

cemerson commented 10 years ago

@adangel You sir are the man - Thanks for writing the progress callback logic! My iOS project gets REALLY fussy when i add/remove this plugin but I finally got it installed w/your progress branch and it works perfect on iOS (will test Android tomorrow).

mmocny commented 10 years ago

Re: Android, this patch needs a rebase since Andrew updated the android implementation quite a bit since branch. I'm trying to do that now, but want to make sure @adangel gets proper attribution for his contribution. Hope it'll be landed by tomorrow.

mmocny commented 10 years ago

Pushed a remote branch so you can start testing. I don't currently have tests for zip plugin locally, so didn't want to push it remotely into I write those, as well as test with App Harness.

Will merge into master when thats done (and if you wanted to try my rebase for correctness now just clone the progress branch)

mmocny commented 10 years ago

I have added autotests to zip plugin. They are ugly, but show progress working nicely.

I'll merge now into master and leave confirming we didn't break app harness as a TODO.

mmocny commented 10 years ago

Merged.