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

More generic target file in DownloadRemoteFileOperation constructor #161

Open
gerritbeuze opened this issue Aug 24, 2018 · 5 comments
Open

Comments

@gerritbeuze
Copy link

Hi,
The DownloadRemoteFileOperation currently takes (String remotePath, String localFolderPath) as parameters. These are then concatenated in getTmpPath().
As a result the temp file directory structure matches the path in remotePath.
This is not very convenient if you want to download to a different directory structure (as I do).
If there were a new constructor (String remotePath, File tempFile) that would simply take the temp file as defined by the using client, it would be much more generic. The existing constructor can be expressed in this by using DownloadRemoteFileOperation(remotePath, new File(localFolderPath, remotePath));
I thik this would make the interface much more generic to use and would save me from copying the class for just this modification.

Thanks in advance,

@AndyScherzinger
Copy link
Member

@tobiasKaminsky what do you think, since you probably know more about the lib than I do?

@tobiasKaminsky
Copy link
Member

@gerritbeuze sorry for coming back this late.
I like the idea. Do you have time to adjust this and create a PR?

@pasniak
Copy link

pasniak commented Dec 24, 2018

I hit the same issue. Is it acceptable to make this change non-backward compatible? I.e force the API user to provide full local path to the file (which will be created) instead of folder? Such change would make code simple: constructor would be
public DownloadRemoteFileOperation(String remotePath, String localFilePath)
instead of public DownloadRemoteFileOperation(String remotePath, String localFolderPath)

@pasniak
Copy link

pasniak commented Dec 26, 2018

It is possible to override run() in a class inheriting from DownloadRemoteFileOperation
but mGet is inaccessible when I try to return a result:
result = RemoteOperationResult(isSuccess(status), /*this.mGet*/ null)

@tobiasKaminsky
Copy link
Member

The current state of this library/class is to only download the file and store it into a temp place.
Once this is succeeded the tempFile can be moved by the calling app to the desired location, like you can see here with NC files app:
https://github.com/nextcloud/android/blob/43ce90355993d8937377336a02de160c3bd5ebe4/src/main/java/com/owncloud/android/operations/DownloadFileOperation.java#L172-L183

Especially for failing downloads, it is a good idea to have it downloaded first into a temp folder, or?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants