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

Various changes: thread-safety, allow overwriting files, progress for large uploads, isKindOfClass checks, improved SFSafarViewController auth #21

Closed
wants to merge 12 commits into from

Conversation

blach
Copy link
Contributor

@blach blach commented Apr 14, 2017

Here is a quick overview of various changes I made:

  • make TJDropbox more thread-safe by using serial operation queues
  • download: allow overwriting existing files
  • upload: allow overwriting existing files
  • add support for upload progress block for chunked (large) uploads
  • increase large file upload chunk size to 10 MB - same as used by the official Dropbox Objective-C SDK
  • add isKindOfClass checks when accessing parsed response objects to prevent crashes if the server returns unexpected values
  • Add support for default redirect url for SFSafariViewController-based authentication: it is not necessary (anymore?) to set up a web site that redirects to an app scheme. Instead, you can just directly use the "db-CLIENTIDENTIFIER" app scheme as the redirect URL for SFSafariViewControlle-based auth. The official Dropbox Objective-C SDK does the same. Use +tokenAuthenticationURLWithClientIdentifier: method to create the auth url and +accessTokenFromURL:withClientIdentifier: to parse the response.

You can find more details in the commit messages. It's up to you if you want to merge these changes, but they were necessary so I could use TJDropbox in my app Textastic.

blach added 12 commits April 9, 2017 18:09
…2Token:accessTokenSecret:appKey:appSecret:completion:

Make accessToken optional in +apiRequestWithPath:accessToken:parameters: and return NSMutableURLRequest instead of NSURLRequest
… authentication - tokenAuthenticationURLWithClientIdentifier: method

Add method to check wheter returned auth url is an error url (also happens when the user presses cancel on the Dropbox website)
…gate’s dictionaries

Use serial operation queue when accessing TJDropbox’s tasks hash table
Previously, multiple threads could access these at the same time, so it could be mutated while being read which would result in a crash.
…re trying to move the downloaded file into place. Previously, the move operation would fail if the file already existed.
…server ever returned an unexpected result, accessing the parsed objects would otherwise result in a crash.
@blach
Copy link
Contributor Author

blach commented Apr 14, 2017

Sorry, I don't know why some commits appear again in this pull request although you already merged them. I'm new to this pull request thing, so I probably did something that I should have done differently.

@timonus
Copy link
Owner

timonus commented Apr 27, 2017

I like these changes. Mind if I open a PR myself and modify them a bit before merging?

@blach
Copy link
Contributor Author

blach commented Apr 27, 2017

Sure, or just change it after merging so that my commit messages do not get lost?

@SquaredTiki
Copy link

Just curious what the status of the changes in this PR are? Having overwrite support is of interest to me and I've had a quick look over the others changes too, especially thread-safety which get 👍 from me.

@blach
Copy link
Contributor Author

blach commented Jun 18, 2017

FWIW, I've been using this code in production in my iOS code editor app Textastic and had no problems with it so far. The upgrade from the v1 API was seamless.

@timonus
Copy link
Owner

timonus commented Jun 19, 2017

Arg, sorry for the delay on this. Changes look good, I've only got minor things I'd change about it. Will merge so others can use it :)

Thanks for contributing!

@timonus
Copy link
Owner

timonus commented Jun 19, 2017

Alrighty, this has been merged in a3057b5... d7af788

I've opened some follow up tasks for this

I think this definitely warrants a new version once this is all done. Will start working on these one by one.

@timonus timonus closed this Jun 19, 2017
@timonus
Copy link
Owner

timonus commented Jun 19, 2017

Also added Textastic to the list of apps using it in 161d29c, happy this was useful to you!

@blach
Copy link
Contributor Author

blach commented Jun 19, 2017

Thanks for sharing this library!

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