Skip to content

Thumbnail during upload#43

Merged
AndyScherzinger merged 7 commits intomasterfrom
thumbnailDuringUpload
Jul 13, 2016
Merged

Thumbnail during upload#43
AndyScherzinger merged 7 commits intomasterfrom
thumbnailDuringUpload

Conversation

@tobiasKaminsky
Copy link
Member

No description provided.

@AndyScherzinger
Copy link
Member

Checked the code LGTM 👍

mAccount = account;
}

public ThumbnailGenerationTask(FileDataStorageManager storageManager, Account account){
Copy link
Member

Choose a reason for hiding this comment

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

you might wanna have a look on NonNull annotation in android :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Denotes that a parameter, field or method return value can never be null.

But it is the other way, the parameter should never be null, but it can.

Copy link
Member

Choose a reason for hiding this comment

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

So handling null parameter via throwing exception seems a bit odd

Copy link
Member Author

Choose a reason for hiding this comment

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

It is adapted from ownCloud ;)
This should not happen as far as I have seen the code, so I would leave it this way.

@tobiasKaminsky tobiasKaminsky added this to the Nextcloud App 1.2.0 milestone Jun 22, 2016
@przybylski
Copy link
Member

👍

@AndyScherzinger
Copy link
Member

👍

@AndyScherzinger AndyScherzinger force-pushed the thumbnailDuringUpload branch from dea553e to 5ad7225 Compare July 9, 2016 19:06
@tobiasKaminsky
Copy link
Member Author

Please review again.
It was not working if "file is kept in original folder" is used.

}

mFile = params[0];
if (params.length == 2){
Copy link
Member

Choose a reason for hiding this comment

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

This is wrong on so many levels... but i'll let it slide since it is adopted from ownCloud

Copy link
Member Author

Choose a reason for hiding this comment

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

As I am always eager to learn/code better, please help me and show a better solution.
First I wanted to create a new inner class that holds both values and can be passed to the task. But it was too much work ;-)

Copy link
Member

Choose a reason for hiding this comment

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

So, to be merged to master or wait for @przybylski's coding ninja skills?

Copy link
Member

Choose a reason for hiding this comment

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

Push it, Ill make a PR

Copy link
Member

Choose a reason for hiding this comment

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

Sweet! Always a pleasure pushing things forward with you guys! 💯

@przybylski
Copy link
Member

👍

@AndyScherzinger
Copy link
Member

AndyScherzinger commented Jul 13, 2016

Code LGTM - unfortunately I can't test it right now since instant upload is broken for Android N
(Thanks Google...) 👍

@AndyScherzinger
Copy link
Member

LGTM

@AndyScherzinger AndyScherzinger merged commit 7fb1c1d into master Jul 13, 2016
@AndyScherzinger AndyScherzinger deleted the thumbnailDuringUpload branch July 13, 2016 19:49
@AndyScherzinger
Copy link
Member

Merged 🚀

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