Change default(string) to string.Empty, updated for Android 9 SDK and enabled support for http #108
Conversation
Update latest
…c for http download enabling. Changes the default(string) to string.Empty since default(string) made them null and not ""
There was a problem hiding this comment.
Congrats 🎉 Not bad for your first pull-request. Really appreciate that you took the time.
I've left some annotations here in the code-review, which I want you to elaborate on.
Have you also had a look at the other platform implementations? And what about the other - not so obvious - situations? E.g. what does a newly created instance of a DownloadFileImplementation have as value? Doesn't it also have null? If you start there, iOS and UWP would also need some work. To give you the next hint: I sometimes forgot resetting the value for StatusDetails when changing the status 😅
On a side note: I personally prefer a distinct separation of concerns in commits. Think of it like this: Using a comma or the word and should get you thinking. Your commit might be solving more than one problem.
This last issue will help you a lot when refactoring and you're asked to split a feature apart which you have developed. E.g. now I could've deployed the empty-string story without having to take in the changes related to the Android 9 SDK.
| @@ -414,6 +415,9 @@ Global | |||
| {D32EDF26-94F4-4F80-B26C-CA250A3F6D7D} = {11FE7FBB-8560-4D89-90F5-F218CF18A879} | |||
| {FEE6A3A2-9CDD-473C-8587-6C13241BD03A} = {11FE7FBB-8560-4D89-90F5-F218CF18A879} | |||
| EndGlobalSection | |||
| GlobalSection(ExtensibilityGlobals) = postSolution | |||
There was a problem hiding this comment.
What is this change for? What does it help - what does it do?
There was a problem hiding this comment.
I haven't change anything regarding this, VS 2017 v15.9.9.9 is the one to blame.
It could be added when I set the Android project to deploy.
| @@ -16,8 +16,9 @@ | |||
| <GenerateSerializationAssemblies>Off</GenerateSerializationAssemblies> | |||
| <AndroidUseLatestPlatformSdk>false</AndroidUseLatestPlatformSdk> | |||
| <DevInstrumentationEnabled>True</DevInstrumentationEnabled> | |||
| <TargetFrameworkVersion>v7.0</TargetFrameworkVersion> | |||
| <AndroidTlsProvider></AndroidTlsProvider> | |||
| <TargetFrameworkVersion>v9.0</TargetFrameworkVersion> | |||
There was a problem hiding this comment.
Will this not require everyone, using this package, to upgrade his app to at least this version? I'm sometimes quite concerned to do this, not to lock people in a position where they're forced to update if they want to continue using my plugin.
I haven't been involved in the Xamarin development lately and don't know their best practices. Please write your comment here.
There was a problem hiding this comment.
I dunno best practices for this, but I have checked the docs: https://docs.microsoft.com/en-us/xamarin/android/app-fundamentals/android-api-levels?tabs=windows
We recommend that you always compile with the latest available Target Framework version. Doing so provides you with helpful warning messages for any deprecated APIs that might be called by your code. Using the latest Target Framework version is especially important when you use the latest support library releases – each library expects your app to be compiled at that support library's minimum API level or greater.
What API calls you make, determinate how low you can go on the "Minimum android version", but it still compiles at eg 9.0.
I have some knowledge about this, but not at all an expert 👍
| <application android:label="Download Example"></application> | ||
|
|
||
| <!-- usesCleartextTraffic must be enabled for Android 9 and downloads using http --> | ||
| <application android:label="Download Example" android:usesCleartextTraffic="true"></application> |
There was a problem hiding this comment.
Is this only needed for Android 9 when trying to download content via HTTP, or does it also apply for older Versions of Android - or other protocols?
There was a problem hiding this comment.
Below Android 9 the defaults to android:usesCleartextTraffic="true".
Android 9 is the first project that sets android:usesCleartextTraffic="false" as default.
A downloadmanager that can't handle http downloads isn't very handy in my eye. But using http is considered as a safety risk.
| @@ -21,7 +21,7 @@ Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "DownloadExample.Droid", "Sa | |||
| EndProject | |||
| Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "DownloadExample.iOS", "Sample\iOS\DownloadExample.iOS.csproj", "{3BFA0D23-9172-46AF-BF9A-84F1E35BB969}" | |||
| EndProject | |||
| Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "DownloadExample.UWP", "Sample\UWP\UWP.csproj", "{D32EDF26-94F4-4F80-B26C-CA250A3F6D7D}" | |||
| Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "UWP", "Sample\UWP\UWP.csproj", "{D32EDF26-94F4-4F80-B26C-CA250A3F6D7D}" | |||
There was a problem hiding this comment.
I don't think this was supposed to go in here 😉
There was a problem hiding this comment.
I don't have UWP installed on the computer I opened it on.
So I should probably not included changes in sln for the pull.
I don't have anything else than Android on my dev machine atm. And I no UWP/mac/iphone available for development. Limited resources on my laptop to install it.
I agree! As a newbie in the Github I have much to learn there :)
That isn't a problem for me, the changes to Android 9 is what is available at my dev machine atm. |
|
Thanks for getting back to me on this. Would you be able to at least work down the other use-cases where I have considered publishing pre-releases and I had it working for quite a while, but then the automated CI instance broke and I gave up on it. See 564b05a |
This is my first pull request ever, so be kind if this isn't right 👍
Regarding the string issue discussed her #105 (comment)
This simple change will ensure that StatusDetails always returns a string.
Change eliminate the need for null-checking of strings while using library.