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

Uploads from content uris #1643

Merged
merged 20 commits into from
May 23, 2016
Merged

Uploads from content uris #1643

merged 20 commits into from
May 23, 2016

Conversation

jabarros
Copy link
Contributor

No description provided.

}
// and what happens in case of error?; wrong target name for the upload
} catch (Exception e) {
Log_OC.e(TAG, "Error while trying to copy and upload a content schema type file ", e);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

schema type content, in this case ;)

@malkomich
Copy link
Member

All code seems OK, just that log detail ^^ 👍

@ghost
Copy link

ghost commented Apr 28, 2016

@jabarros

Thanks a lot for your contribution!
Contributions to the core repo require a signed contributors agreement http://owncloud.org/about/contributor-agreement/

Alternatively you can add a comment here where you state that this contribution is MIT licensed.

Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

1 similar comment
@ghost
Copy link

ghost commented Apr 28, 2016

@jabarros

Thanks a lot for your contribution!
Contributions to the core repo require a signed contributors agreement http://owncloud.org/about/contributor-agreement/

Alternatively you can add a comment here where you state that this contribution is MIT licensed.

Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

@malkomich
Copy link
Member

Cool, we can ping @jesmrec to test it when he gets less busy ;)

@ghost
Copy link

ghost commented May 12, 2016

@jabarros

Thanks a lot for your contribution!
Contributions to the core repo require a signed contributors agreement http://owncloud.org/about/contributor-agreement/

Alternatively you can add a comment here where you state that this contribution is MIT licensed.

Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

1 similar comment
@ghost
Copy link

ghost commented May 12, 2016

@jabarros

Thanks a lot for your contribution!
Contributions to the core repo require a signed contributors agreement http://owncloud.org/about/contributor-agreement/

Alternatively you can add a comment here where you state that this contribution is MIT licensed.

Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

@jesmrec
Copy link
Collaborator

jesmrec commented May 12, 2016

approved 👍

}
// and what happens in case of error?; wrong target name for the upload
} catch (Exception e) {
Log_OC.e(TAG, "Error while trying to copy and upload a schema type content file ", e);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will end in a silent error; we should show the Toast that was introduced in version 2.0.0, previous line number 786

@ghost
Copy link

ghost commented May 16, 2016

@jabarros

Thanks a lot for your contribution!
Contributions to the core repo require a signed contributors agreement http://owncloud.org/about/contributor-agreement/

Alternatively you can add a comment here where you state that this contribution is MIT licensed.

Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

@davivel
Copy link
Contributor

davivel commented May 18, 2016

@jabarros , let's refactor a bit.

Code in Uploader#uploadFiles() is better handling both file:// and content:// URIs pointing to files to upload. Let's put it in a common place so that FileDisplayActivity#requestSimpleUpload() takes advantage of it, and avoid this way duplicated code that evolves differently. We've suffered a lot of bugs in the past due to this.

@ghost
Copy link

ghost commented May 19, 2016

@jabarros

Thanks a lot for your contribution!
Contributions to the core repo require a signed contributors agreement http://owncloud.org/about/contributor-agreement/

Alternatively you can add a comment here where you state that this contribution is MIT licensed.

Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

1 similar comment
@ghost
Copy link

ghost commented May 19, 2016

@jabarros

Thanks a lot for your contribution!
Contributions to the core repo require a signed contributors agreement http://owncloud.org/about/contributor-agreement/

Alternatively you can add a comment here where you state that this contribution is MIT licensed.

Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

return;
}
}
ArrayList<Parcelable> mStreamsToUpload = new ArrayList<Parcelable>() {{
Copy link
Contributor

@davivel davivel May 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Local variable shouldn't have m prefix, you, copy-paster :P ;)

Copy link
Contributor

@davivel davivel May 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides, we should move this code out of the method, or change it to receive directly an ArrayList<Parcelable> object instead of an Intent data. See the caller code; in some situations requestSimpleUpload is called in a loop, what will result in a several UriUploader objects created and, more problematic, in several CopyAndUploadContentUrisTask

@davivel
Copy link
Contributor

davivel commented May 19, 2016

@jabarros , reviewed. Great job you did it there, let's give it the final shot.

@ghost
Copy link

ghost commented May 20, 2016

@jabarros

Thanks a lot for your contribution!
Contributions to the core repo require a signed contributors agreement http://owncloud.org/about/contributor-agreement/

Alternatively you can add a comment here where you state that this contribution is MIT licensed.

Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

1 similar comment
@ghost
Copy link

ghost commented May 20, 2016

@jabarros

Thanks a lot for your contribution!
Contributions to the core repo require a signed contributors agreement http://owncloud.org/about/contributor-agreement/

Alternatively you can add a comment here where you state that this contribution is MIT licensed.

Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

@ghost
Copy link

ghost commented May 20, 2016

@jabarros

Thanks a lot for your contribution!
Contributions to the core repo require a signed contributors agreement http://owncloud.org/about/contributor-agreement/

Alternatively you can add a comment here where you state that this contribution is MIT licensed.

Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

@jesmrec
Copy link
Collaborator

jesmrec commented May 23, 2016

Approved (again) 👍

@davivel
Copy link
Contributor

davivel commented May 23, 2016

Seen last changes by @jabarros . 👍

@davivel davivel force-pushed the uploads_from_content_uris branch from d8004af to 7225b9f Compare May 23, 2016 11:04
@ghost
Copy link

ghost commented May 23, 2016

@jabarros

Thanks a lot for your contribution!
Contributions to the core repo require a signed contributors agreement http://owncloud.org/about/contributor-agreement/

Alternatively you can add a comment here where you state that this contribution is MIT licensed.

Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

@davivel davivel merged commit 3cf03ec into master May 23, 2016
@davivel davivel deleted the uploads_from_content_uris branch May 23, 2016 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants