This repository has been archived by the owner on May 15, 2024. It is now read-only.
GH-250 Implement Launcher API #356
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… on rethrowing instead of swallowing the exception
…ssentials into feature/issue-250
…rce uri validity and exception behaviour across platforms.
…ssentials into feature/issue-250
✅ Validation status: passed
For more details, please refer to the build report. Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report. |
✅ Validation status: passed
For more details, please refer to the build report. Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report. |
Redth
previously approved these changes
Jul 9, 2018
jamesmontemagno
suggested changes
Jul 9, 2018
| static Task PlatformOpenAsync(Uri uri) | ||
| { | ||
| var intent = CreateIntent(uri.ToString()); | ||
| Platform.AppContext.StartActivity(intent); |
There was a problem hiding this comment.
How will this work with the intent system? Does the top need to be cleared?
There was a problem hiding this comment.
The application is launched on top of the current application. The back button returns to the calling app. I don't think the top has to be cleared, it works well on android.
|
|
||
| static async Task PlatformOpenAsync(Uri uri) | ||
| { | ||
| await UIApplication.SharedApplication.OpenUrlAsync(new NSUrl(uri.ToString()), new UIApplicationOpenUrlOptions()); |
There was a problem hiding this comment.
Should we make the string to NSUrl and extension method somewhere?
There was a problem hiding this comment.
Probably, I guess many other people use uri.ToString(). Having conversion operations via extension methods for every platform is advisable. I would suggest putting this in a new pr though.
| { | ||
| public static partial class Launcher | ||
| { | ||
| public static Task<bool> CanOpenAsync(string uri) => PlatformCanOpenAsync(new Uri(uri)); |
There was a problem hiding this comment.
should we do some validation of the string here? Usually we check for null or whitespace, throw an exception and then call the platform code.
| { | ||
| public static Task<bool> CanOpenAsync(string uri) => PlatformCanOpenAsync(new Uri(uri)); | ||
|
|
||
| public static Task<bool> CanOpenAsync(Uri uri) => PlatformCanOpenAsync(uri); |
There was a problem hiding this comment.
same here, but for null.
|
|
||
| static async Task PlatformOpenAsync(Uri uri) | ||
| { | ||
| await WinLauncher.LaunchUriAsync(uri); |
There was a problem hiding this comment.
no need to make this async, we should just just return the Task in a =>
| static Intent CreateIntent(string uri) | ||
| { | ||
| var androidUri = AndroidUri.Parse(uri); | ||
| var intent = new Intent(Intent.ActionView, androidUri); |
There was a problem hiding this comment.
This could all be done on one line:
static Intent CreateIntent => new Intent(Intent.ActionView, AndroidUri.Parse(uri));
| { | ||
| static Task<bool> PlatformCanOpenAsync(Uri uri) | ||
| { | ||
| var intent = CreateIntent(uri.ToString()); |
There was a problem hiding this comment.
should this be uri.ToString? Should it be the actual AbsolutePath?
There was a problem hiding this comment.
You are correct, I read through the microsoft docs and changed it. AbsolutePath should always be used when you want an escaped string, and we do. I also changed it on iOS. UWP needs an uri either way so it will work there.
|
I adressed all the change requests @jamesmontemagno |
…ssentials into feature/issue-250
✅ Validation status: passed
For more details, please refer to the build report. Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report. |
✅ Validation status: passed
For more details, please refer to the build report. Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report. |
jamesmontemagno
previously requested changes
Jul 10, 2018
| CanLaunchCommand = new Command(CanLaunch); | ||
| } | ||
|
|
||
| async void OnLaunchBrowser() | ||
| { | ||
| await Launcher.OpenAsync("https://github.com/xamarin/Essentials"); |
There was a problem hiding this comment.
I don't think we want the sample to open the browser as that is what the Browser API is for and should guide developers down that route :) Even though technically it can be.
There was a problem hiding this comment.
You are correct, but I can not think of a uri example which works on every platform and is not covered by existing or by a PR API
| async void OnLaunch() | ||
| { | ||
| await Launcher.OpenAsync(LaunchUri); | ||
| } | ||
|
|
||
| async void OnLaunchMail() | ||
| { | ||
| await Launcher.OpenAsync("mailto:"); |
There was a problem hiding this comment.
Is it possible that these throw an exception at all? Perhaps we should create a generic method that tries to launch it and then handle the exception here.
There was a problem hiding this comment.
It is a valid uri, so there will be no exception. iOS definitely has a mail app, uwp does too. Android has some mail app 99% of the time, you could probably have no mail app instaled if you use a gApps free os. Either way, no exception would be thrown and the intent would just be unhandled.
|
|
||
| public static Task<bool> CanOpenAsync(Uri uri) => PlatformCanOpenAsync(uri); | ||
| public static Task<bool> CanOpenAsync(Uri uri) | ||
| => uri != null ? PlatformCanOpenAsync(uri) : throw new ArgumentNullException(nameof(uri)); |
There was a problem hiding this comment.
Let's make this a non-expression bodied method and also fill in the full argument null exception for devs like the others.
|
|
||
| public static Task OpenAsync(Uri uri) => PlatformOpenAsync(uri); | ||
| public static Task OpenAsync(Uri uri) | ||
| => uri != null ? PlatformOpenAsync(uri) : throw new ArgumentNullException(nameof(uri)); |
| { | ||
| await WinLauncher.LaunchUriAsync(uri); | ||
| return WinLauncher.LaunchUriAsync(uri).AsTask(); |
There was a problem hiding this comment.
Can be an expression bodied member here too.
✅ Validation status: passed
For more details, please refer to the build report. Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report. |
✅ Validation status: passed
For more details, please refer to the build report. Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report. |
5 tasks
Redth
pushed a commit
that referenced
this pull request
Jul 25, 2018
* GH-250 Implement Launcher API (#356) * Initial commit for Launcher Api as discussed in Issue 250 * Fixed and refactored android launch code, fixed unit tests. Fixed namespaces. * Refactored code, fixed uwp launcher. Added unit test * Added sample launcher page and viewmodel. * Fixed launcher Page typo * Refactored launcher code. after testing exception behaviour I decided on rethrowing instead of swallowing the exception * Refactored access modifiers. Also changed exception behaviour to enforce uri validity and exception behaviour across platforms. * Updated docs, stripped even more code. * Added validation. Adressed all other changes as required in pr * Removed dead code * Adressed last issues from pr * Cleanup launcher * cleanup iOS tests * Early morning typos
jamesmontemagno
added a commit
that referenced
this pull request
Jul 31, 2018
* GH-343: Take into consideration VerticalAccuracy on iOS Altitude (#344) * Fix for #343 Only set altitude if it's available (VerticalAccuracy is negative when it isn't according to iOS doc) * Use: (double?) * dotnet foundation CLA broken link (#349) * Update ns-Xamarin.Essentials.xml * Update README.md (#340) * GH-365 Change to generic EventHandlers * Clean up unit tests. * GH-363 Enable C# 7.2 (#364) * GH-363 Enable CD 7.3 * Update Xamarin.Essentials.csproj * GH-367 Change Ui to UI & Ac to AC (#374) * GH-367 Change Ui to UI * Ensure proper Ui for Android * Fix gryoscope * Rename Ac to AC * Update framework indexes * Create Product Feedback page for Docs (#369) * Create PRODUCT-FEEDBACK.md * Update PRODUCT-FEEDBACK.md * Fixed a typo from "chanaged" to "changed" * Fixed a typo from "chanaged" to "changed" * Fixed a typo from "chanaged" to "changed" * GH-337 Android Low Pass Filter (#354) * Implemented low pass filter as requested by issue 337 * Added value to sample page. * Updated docs for ApplyLowPassFilter * Enabled ApplyLowPassFilter Switch on Android, disabled on all other platforms * Moved LowPassFilter to shared, removed unnecessary usings. * Updated Docs * Sample Project Tweaks and Optimizations (#353) * Make sure orientation sensor stops on sample. * Fix up all sensors page to start and stop correctly * Only check permission if unknown status * Update to latest sdk.extras * fix back button press on TTS page. Check for null. * Add samples sln for CI/CD * Update readme and samples sln * Don't use code sign key for simbuilds * let's try this gain * Update to projects * Cleanup sample config * bump sdk extras * Add in readme for install with helpful information. Bump MSBuild.Sdk.Extras (#392) * GH-368: Allow for unicode in host names and escape query (#393) * Allow for unicode in host names and escape query Previously we were passing the the url as is to each platform and letting it deal with how to parse into the platform specific url. This handles unicode domain names (IDN Mapping aka PunyCode) for the domain, but also reconstructs the url based on the parsed System.Uri from the original string, and so the PathAndQuery will be the % escaped version which will account for unicode characters in it as well. Finally, on iOS and Android we are using AbsoluteUri to end up with the fully IDN mapped and % encoded URL to pass to the system URL types. * Extract Escape Uri logic into its own method More testable. * Add uri escaping tests * Improve uri escape tests * Update default iOS DeviceTests target Also don’t allow picking a sim that’s unavailable (this was a bug since we were checking for contains `available` which of course `unavailable` also contains!) * Update iOS DeviceInfo.Model to return specifid hw.machine for more control. (#395) via: https://github.com/xamarin/xamarin-macios/blob/bc492585d137d8c3d3a2ffc827db3cdaae3cc869/tests/linker/ios/link%20sdk/LinkSdkRegressionTest.cs#L603-L631 Added tests * GH-380 Use Internal Preferences for Secure Storage consistency. (#386) * Use Internal Preferences for Secure Storage consistency. Save if we created key pre-M so we always use pre-M if device upgrades. * Fix logic for pre-m key check The logic was slightly off, I think this fixes it, but would be good to have another set of eyes... 1. We check to see if the device is pre-M (if it does _not_ have `M`, or if we already set the fact it's pre-M in the preference - aka from a previous install before an upgrade of the OS) 2. If we aren't pre-M, we can't use Symmetric Key from Keystore 3. If we make it down to using Asymmetric Key, we set the pre-M preference to `true` to persist the value for future invocations, which will make it 'stick' in the event of a pre-M to M+ OS upgrade. * Address feedback on key naming. * Added test for secure storage to simulate upgrade From API < 23 to API >= 23 after storing data with an asymmetric key and then moving to a platform supporting symmetric keys. * Ensure we always set flags when using specified keygen * GH-388 If activity is required and null throw null exception with information. (#396) * If activity is required and null throw null exception with information. * Remove currentactivity and update getcurrentactivity. * GH-250 Implement Launcher API (#356) (#405) * GH-250 Implement Launcher API (#356) * Initial commit for Launcher Api as discussed in Issue 250 * Fixed and refactored android launch code, fixed unit tests. Fixed namespaces. * Refactored code, fixed uwp launcher. Added unit test * Added sample launcher page and viewmodel. * Fixed launcher Page typo * Refactored launcher code. after testing exception behaviour I decided on rethrowing instead of swallowing the exception * Refactored access modifiers. Also changed exception behaviour to enforce uri validity and exception behaviour across platforms. * Updated docs, stripped even more code. * Added validation. Adressed all other changes as required in pr * Removed dead code * Adressed last issues from pr * Cleanup launcher * cleanup iOS tests * Early morning typos * Name Alignment for BrowserLaunchMode (#408) * Rename BrowserLaunchType to BrowserLaunchMode * update nuget * Rename parameter * GH-287 OpenMaps Implementation (#361) (#404) * GH-287 Maps Implementation (#361) * Implemented maps as mentioned in branch name issue. also fixed a spelling mistake * Added samples * Added doc stubs * Added comments for map types * Refactored code * Formatting of docs * Added tests * Changed coordinates to display something recognisable * Added uri escaping, thus removing duplicate code. also had to turn off a warning due to inconsistent style cop and vs warning behaviour. * Removed ref until in is added in c# 7.2 * Adressed issues in pr * Updated launch code * Adressed method to onliner * Updated docs * Removed ClearTop intent, added sample with adress * Rename to align names. Added extensions and additional paramaters. * Update tests * Throw if options are null. * Add overload for lat/long without options * Enable multi-target for 25,26,27 (#411) Alight nuget versions to xamarin.forms. * No More Shared Projects (#410) * Change to multi-targeted project for device tests :) * Cleanup scripts * Fix build error due to preprocessor directive (#417) * Sync Sensor Speeds & Reset UWP to 0 (#419) * Restore the default report interval to release resources while the sensor is not in use * Cleanup sensor reading times. Make them all the same and only in 1 place Compass is still unique * Create unique preference storage for each feature. (#434) * Create unique preference storage for each feature. * update storage per @Redth * Update version
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of Change
Implemented Launcher as specified at #250
Bugs Fixed
-None
API Changes
Launcher:
Added:
Changed:
Behavioral Changes
PR Checklist