search

liveservices / LiveSDK-for-iOS

LiveSDK library for integrating with Live Connect
MIT License
138 stars 84 forks source link

Download should be to disk - never to memory! #49

Closed danielgindi closed 9 years ago

sylverb commented 9 years ago

Thanks for this patch ! Maybe setting up a NSOutputStream would be better as uploading is using a NSInputStream ?

aclev commented 9 years ago

Another good change, however it has breaking changes. If we merge this pull requests developers will have to change their progress delegate methods to no longer take a NSData object, as well as creating a file and reading from the DownloadOperations destinationDownloadPath property. Is it possible to have the download to disk without removing the download to memory methods?

danielgindi commented 9 years ago

Not all breaking changes are bad.

This one is a GOOD one.

There's no way that a developer currently uses the download functions, as you can see in issues people are having memory problems in uploading too.

Making this download to memory is a joke in the first place, and people who are actually using this SDK will have to make some small changes if they want to upgrade- and that's good.

If you still pass NSData in the delegates you will still have the same problem.

There are some major design issues in Live iOS SDK, not only with downloading/uploading, and it's time to address them. Keeping the problematic parts won't do any good.

sylverb commented 9 years ago

It may be useful to get content directly to NSData if it's a content that you want to present to the user (image, documents,...). Even NSData has a method to fill data from a NSURL ! Having to use a temporary file would add unneeded complexity in that case !

danielgindi commented 9 years ago

That's not a complexity. You just said NSData has a method to fill from an NSURL, in the rare case that you need one. In most cases the downloaded file is too big to keep in memory. Even an image "that you want to present to the user" should be in a temporary file, and let the system optimize the memory usage (i.e. The OS may remove from memory when UIImage is not visible)

Anyway: By downloading to NSData - you make sure that you crash many apps trying to download files that are a few megabytes in size. By downloading to a temporary file - you make sure you can download any file, any size.

It's very clear which way is the preferred one.

On Tue, Jan 6, 2015 at 9:12 AM, Sylver Bruneau notifications@github.com wrote:

It may be useful to get content directly to NSData if it's a content that you want to present to the user (image, documents,...). Even NSData has a method to fill data from a NSURL ! Having to use a temporary file would add unneeded complexity in that case !

— Reply to this email directly or view it on GitHub https://github.com/liveservices/LiveSDK-for-iOS/pull/49#issuecomment-68833710 .

sylverb commented 9 years ago

NSData is indeed for small files, downloading big files has to be done using a stream or directly to a file, I'm 100% agree with you ! I'm using your patch in my multi-protocol file manager application and updating was the matter of few line to change : https://github.com/sylverb/NAStify/commit/f871544a826db9a8dc565bfa6027c4c51781c1d0

But IMO the SDK should let the choice to the developer, if there is no need to write data in flash, why would we force him to do so ? If the app wants to show some content to the user, this content will be in memory at one point, the developer is responsible for checking that the system will have enough memory to handle the file to present to the user ...

Anyway there is a lack of constancy in this SDK :

To have the most universal SDK (for developers), I would add :

Anyway the current SDK with your patch is good for my needs ;)

danielgindi commented 9 years ago

Yes as I said - There's a major design problem in this SDK. Upload methodologies are not consistent with download... What we should really have is the basic working only with NSStreams, and then have "convenience" methods for files and NSData.

On Tue, Jan 6, 2015 at 10:50 AM, Sylver Bruneau notifications@github.com wrote:

NSData is indeed for small files, downloading big files has to be done using a stream or directly to a file, I'm 100% agree with you ! I'm using your patch in my multi-protocol file manager application and updating was the matter of few line to change : sylverb/NAStify@f871544 https://github.com/sylverb/NAStify/commit/f871544a826db9a8dc565bfa6027c4c51781c1d0

But IMO the SDK should let the choice to the developer, if there is no need to write data in flash, why would we force him to do so ? If the app wants to show some content to the user, this content will be in memory at one point, the developer is responsible for checking that the system will have enough memory to handle the file to present to the user ...

Anyway there is a lack of constancy in this SDK :

  • For uploading there is a method to upload from NSInputStream (allowing to upload from a file) but you can't upload directly some NSData content
  • For downloading there is a method to download to NSData but you can't download directly to a file

To have the most universal SDK (for developers), I would add :

  • A method to download to NSOutputStream (without removing the download to NSData method)
  • A method to upload from NSData

Anyway the current SDK with your patch is good for my needs ;)

— Reply to this email directly or view it on GitHub https://github.com/liveservices/LiveSDK-for-iOS/pull/49#issuecomment-68840297 .

aclev commented 9 years ago

As I mentioned before, this is a great change and I agree that we need to implement downloading to disk. While I feel that this change is important at this time I am not ready to accept breaking changes. Remember that there are many other users out there and we have to make sure that we do not break them and force them to update code. Having the NSData convenience methods is a great idea as long as they are modeled after the current NSData download methods.

aclev commented 9 years ago

I'm closing the pull request pending further changes. At this point we are not ready to accept breaking changes.