Added DestinationPathName to DownloadFile · Pull Request #94 · SimonSimCity/Xamarin-CrossDownloadManager

ghost · 2018-07-18T11:58:16Z

SimonSimCity · 2018-07-18T12:37:37Z

Thanks for the support here, but it only fixes half of the problem ... I'll need to take a look at how this can be fixed permanently also when using the temporary location ...

ghost · 2018-07-18T12:54:07Z

Maybe I didn't look that clear to the code and also without looking at the code now: but doesn't this also fill the new property with the location when it's a temporary one? This PR is actually not a solution for the referenced issue, since that's a bit more complicated.

Edit:
No, you're right. The temporary location does not get set.

Edit2:
I've looked in the implementation for Android. There is a way to get the local path with downloadManager.GetUriForDownloadedFile, as you did in your sample app; but it only gets set after the download has been completed. We don't know when the download has been completed and the DownloadManager has no events to hook into. I don't think it's appropriate to add a while loop (also because the question raises where in the code that should be). We can update the docs and decide that when using a temp location, you don't get the path in the new property.

It's not that great, but it's at least something.

The reason for this PR is actually because I'm showing a notification with action when the download has been completed. The action opens the file, but for that I need the have the file path where the file was located. Because the PathNameForDownloadedFile gets set initially and in a seperate method, I don't have a way to get that file path. I can invoke the call manually, however, but as I have some logic that renames the file whenever it already exists, I get an incorrect file path.

SimonSimCity · 2018-07-19T06:49:45Z

You're right in that the native Android download manager doesn't have a way to get notified if a property has changed. That's why I've implemented a timer to update the properties of all running downloads.

In this way we could just set the property after updating the status.

Xamarin-CrossDownloadManager/DownloadManager/Plugin.DownloadManager.Android/DownloadManagerImplementation.cs

Lines 167 to 171 in a5a2860

    
           case DownloadStatus.Successful: 
        
               downloadFile.StatusDetails = default(string); 
        
               downloadFile.Status = DownloadFileStatus.COMPLETED; 
        
               RemoveFile (downloadFile); 
        
               break;

On Android I have to set the path before the download starts, which is the reason why it's downloaded directly to the desired destination. If the download is finished, publish the destination you get from the native download manager.

On iOS there is a method which gets called when the download is finished and it also gives you the full destination path. Here's the excerpt of the code where I move the file to the desired destination after a successful download:

Xamarin-CrossDownloadManager/DownloadManager/Plugin.DownloadManager.iOS/UrlSessionDownloadDelegate.cs

Lines 83 to 93 in a5a2860

    
           var destinationPathName = Controller.PathNameForDownloadedFile?.Invoke (file); 
        
           if (destinationPathName != null) { 
        
               success = MoveDownloadedFile (file, location, destinationPathName); 
        
           } 
        
           // If the file destination is unknown or was moved successfully ... 
        
           if (success) { 
        
               file.Status = DownloadFileStatus.COMPLETED; 
        
           } 
        
           Controller.RemoveFile (file);

Here you either have to publish destinationPathName or location if the destinationPathName was empty.

As it's very likely that UWP will stick around for quite a while, it'd be good if you could also find a way there. Maybe this excerpt from the samples already helps you getting on track:

Xamarin-CrossDownloadManager/Sample/UWP/MainPage.xaml.cs

Lines 81 to 86 in 6833954

    
           case DownloadFileStatus.COMPLETED: 
        
           case DownloadFileStatus.FAILED: 
        
           case DownloadFileStatus.CANCELED: 
        
               downloadBtn.Content = "Downloading finished."; 
        
               ((DownloadFileImplementation)downloadFile).DownloadOperation.ResultFile.DeleteAsync(); 
        
               break;

To be consistent, the variable on the IDownloadFile instance should be updated before the status is set to DownloadFileStatus.COMPLETED.

Please let me know if you disagree in any of the details I wrote. I let you know this because I won't have the time to test these changes ... Please test it and report back. I'll merge it when the changes are useful for all the cases.

…emporary locations.

ghost · 2018-07-19T09:32:17Z

Sounds good. Could you review the last commit I made?

Android
When testing on a Samsung Tab S2, I've gotten content://downloads/all_downloads/{randomnumber} back, where {randomnumber} is actually a random number.

iOS
When testing on the iPhone Simulator, I got the following:
file:///Users/{USERNAME}/Library/Developer/CoreSimulator/Devices/{SOME_GUID}/data/Containers/Data/Application/{SOME_GUID}/Library/Caches/com.apple.nsurlsessiond/Downloads/{REVERSED_DOMAIN_NAME_OF_APP}/CFNetworkDownload_kyJT1R.tmp

UWP
For UWP I took a different approach. At first I set it in ProgressChanged when the status was set to completed, but that was too late. So now I'm setting it in the StartDownloadAsync method, where you are setting the file.

I got the following back:
C:\Workspace\git\Xamarin-CrossDownloadManager\Sample\UWP\bin\x86\Debug\AppX\10MB.zip

How about that?

… temporary location.

ghost · 2018-07-19T09:49:30Z

I've added another commit which only sets the DestinationPathName with the DownloadManager.GetUriForDownloadedFile result when it is null, since that one is always giving a content:// result. Something I don't want, since I'm using a path I allocated using PathNameForDownloadedFile.

SimonSimCity

Looks good! There's just a quick change on the iOS implementation and a request for change on the UWP version ...

SimonSimCity · 2018-07-19T09:44:43Z

DownloadManager/Plugin.DownloadManager.iOS/UrlSessionDownloadDelegate.cs

            var success = true;
            var destinationPathName = Controller.PathNameForDownloadedFile?.Invoke (file);
            if (destinationPathName != null) {
+                file.DestinationPathName = destinationPathName;


Please set the path after the file has been moved. This way if a person only listens on a change on DestinationPathName his closure will be triggered when the file is available there.

Good one. Will do.

SimonSimCity · 2018-07-19T09:47:44Z

DownloadManager/Plugin.DownloadManager.UWP/DownloadFileImplementation.cs

                file = await folder.CreateFileAsync(downloadUrl.Segments.Last(), CreationCollisionOption.GenerateUniqueName);
            }

+            DestinationPathName = file.Path;


This would now be the only instance where this value is set before the download is started ... can we get this consistent? All other platforms should also report the destination when the download is complete (but before the status is updated).

Another question is: Could you please check and confirm how this behaves if the app crashes? Does the download still continue or does it (unlike in the implementation on iOS or Android) stop?

If it continues, is this path still available if the download finishes then?

I tried, but the ProgressChanged method actually kicked in after the Status property changed... Though I just noticed that way my mistake, lol. Fixed.

Do you have a way for me to crash the app whilst running?

This should create a null-reference exception. Put it at the right place and it will get the app to crash 😉

object o = null; DateTime d = (DateTime)o;

I know, but that instantly kills the process since Visual Studio doesn't want to continue. I thought you had a way to crash and keep the process running. Or was that the question? Well then no, the app crashes and the download does not complete. At least in Debug mode.

I published the app with the Release configuration and side-loaded it to see whether the download would continue or stop. I added to the code that it would create a text file containing the download location, but it never shows up. So I guess it's safe to assume that the answer is no.

SimonSimCity · 2018-07-19T09:55:30Z

DownloadManager/Plugin.DownloadManager.Android/DownloadManagerImplementation.cs

            switch ((DownloadStatus)cursor.GetInt (cursor.GetColumnIndex (Android.App.DownloadManager.ColumnStatus))) {
            case DownloadStatus.Successful:
-                downloadFile.StatusDetails = default(string);
+                if (string.IsNullOrEmpty(downloadFile.DestinationPathName))


Good catch! 😅

I've found I can also do
downloadFile.DestinationPathName = cursor.GetString(cursor.GetColumnIndex("local_uri"));
instead of the if statement and line 70.

I get a file:// uri back when using a custom location, content:// for temp locations. What do you think?

Oh and your comment had to be in the Mac file, didn't it? :P

I thought the condition to be good to prevent triggering the change multiple times, but you've already done that in the IDownloadFile implementation...

I think the new way you've added here, using cursor.GetString(cursor.GetColumnIndex("local_uri")) is the one which is best to use.

Oh so that's what you meant. The setter checks if the value is the same as the new one, so pretty much I guess.

SimonSimCity · 2018-07-19T09:57:03Z

CHANGELOG.md

@@ -1,5 +1,9 @@
 ## Changes

+### 1.3.7


Please call it Unreleased for now. It will at least be 1.4.0 as this is a feature, but let's see what we'll get in at this version.

Sure thing. Will you be releasing this as a preview, though? I want to be able to use this feature 😄

iOS: Set DestinationPathName after the file has moved. Android: Changed the way the file path is received. Changed version in changelog.

ghost · 2018-07-19T10:28:46Z

See last commit for the Code Review changes. Feel free to redo the review. Code quality first!

SimonSimCity · 2018-07-19T11:29:17Z

Looks very good, Thanks!

I'll have a look at what else I will take into this release and then push it out. Please bare with me if it takes a while since I'll be on holiday for 2 weeks from today on and this is a project purely developed in my spare-time 😊

ghost · 2018-07-19T11:47:33Z

Thank you.

I understand. For now I'll just use my own DLLs and switch to the new NuGet release whenever you are ready. Have a great holiday!

Tom van de Graaf added 2 commits July 18, 2018 10:16

Added DestinationPathName to DownloadFile

36ee72c

Updated changelog.

9f02ecb

Changed the moment of assignments of DestinationPathName to support t…

db148c1

…emporary locations.

Android: Set DestinationPathName to the given location or else to the…

700b258

… temporary location.

SimonSimCity requested changes Jul 19, 2018

View reviewed changes

UWP: Set DestinationPathName after the file has been downloaded.

54f070a

iOS: Set DestinationPathName after the file has moved. Android: Changed the way the file path is received. Changed version in changelog.

SimonSimCity merged commit 1da1241 into SimonSimCity:develop Jul 19, 2018

SimonSimCity mentioned this pull request Aug 8, 2018

Open downloaded file inside the application #93

Closed

Conversation

ghost commented Jul 18, 2018

Uh oh!

SimonSimCity commented Jul 18, 2018

Uh oh!

ghost commented Jul 18, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SimonSimCity commented Jul 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jul 19, 2018

Uh oh!

ghost commented Jul 19, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SimonSimCity left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SimonSimCity Jul 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Jul 19, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SimonSimCity commented Jul 19, 2018

Uh oh!

ghost commented Jul 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

ghost commented Jul 18, 2018 •

edited by ghost

Loading

SimonSimCity commented Jul 19, 2018 •

edited

Loading

ghost commented Jul 19, 2018 •

edited by ghost

Loading

SimonSimCity Jul 19, 2018 •

edited

Loading

ghost commented Jul 19, 2018 •

edited by ghost

Loading