-
-
Notifications
You must be signed in to change notification settings - Fork 142
fix(android): load KMP files from app-external sources #16392
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
base: master
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -111,36 +111,51 @@ | |||||||
| <data | ||||||||
| android:mimeType="application/x-keyman-package" | ||||||||
| android:scheme="content" /> | ||||||||
| <data | ||||||||
| android:mimeType="application/vnd.keyman.kmp+zip" | ||||||||
| android:scheme="content" /> | ||||||||
| </intent-filter> | ||||||||
|
|
||||||||
| <!-- | ||||||||
| Capture file open requests (pathPattern is honoured) where no | ||||||||
| MIME type is provided in the Intent. An Intent with a null | ||||||||
| MIME type will never be matched by a filter with a set MIME | ||||||||
| type, so we need a second intent-filter if we wish to also | ||||||||
| match files with this extension and a non-null MIME type | ||||||||
| (even if it is non-null but zero length). | ||||||||
| --> | ||||||||
| <intent-filter android:priority="50"> | ||||||||
| <action android:name="android.intent.action.VIEW" /> | ||||||||
|
|
||||||||
| <category android:name="android.intent.category.BROWSABLE" /> | ||||||||
| <category android:name="android.intent.category.DEFAULT" /> | ||||||||
|
|
||||||||
| <data android:scheme="file" /> | ||||||||
| <data android:scheme="content" /> | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does this do?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Matches both file:/// and content:// URIs. Often times, what we get between apps appears to be the It's an OR, not an AND - allowing us to share the host and pathPattern entries for both protocols.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure what this is fixing though -- does this impact the underlying reported issue? |
||||||||
|
|
||||||||
| <data android:host="*" /> | ||||||||
| <data android:pathPattern="/.*\\.kmp" /> | ||||||||
| <data android:pathPattern=".*\\.kmp" /> | ||||||||
| <data | ||||||||
| android:mimeType="application/vnd.keyman.kmp+zip" | ||||||||
| android:scheme="content" /> | ||||||||
|
|
||||||||
| <!-- | ||||||||
| Backup MIME intents | ||||||||
| --> | ||||||||
| <data | ||||||||
| android:mimeType="application/vnd.keyman" | ||||||||
| android:scheme="content" /> | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not a registered mime type.
Suggested change
|
||||||||
| <data | ||||||||
| android:mimeType="application/octet-stream" | ||||||||
| android:scheme="content" /> | ||||||||
| <data | ||||||||
| android:mimeType="application/x-keyman-package" | ||||||||
| android:scheme="content" /> | ||||||||
|
jahorton marked this conversation as resolved.
Outdated
|
||||||||
| <data | ||||||||
| android:mimeType="application/vnd.keyman.kmp+zip" | ||||||||
| android:scheme="content" /> | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Repeated
Suggested change
|
||||||||
| </intent-filter> | ||||||||
|
|
||||||||
| <!-- | ||||||||
| Capture file open requests (pathPattern is honoured) where a | ||||||||
| (possibly blank) MIME type is provided in the Intent. This | ||||||||
| filter may only be necessary for supporting ES File Explorer, | ||||||||
| which has the probably buggy behaviour of using an Intent | ||||||||
| with a MIME type that is set but zero-length. It's | ||||||||
| impossible to match such a type except by using a global | ||||||||
| wildcard. | ||||||||
| --> | ||||||||
| Capture file open requests (pathPattern is honoured) where no | ||||||||
| MIME type is provided in the Intent. An Intent with a null | ||||||||
| MIME type will never be matched by a filter with a set MIME | ||||||||
| type, so we need a second intent-filter if we wish to also | ||||||||
| match files with this extension and a non-null MIME type | ||||||||
| (even if it is non-null but zero length). | ||||||||
| --> | ||||||||
| <intent-filter android:priority="50"> | ||||||||
| <action android:name="android.intent.action.VIEW" /> | ||||||||
|
|
||||||||
|
|
@@ -149,25 +164,8 @@ | |||||||
|
|
||||||||
| <data android:scheme="file" /> | ||||||||
| <data android:host="*" /> | ||||||||
| <data android:mimeType="*/*" /> | ||||||||
| <data android:pathPattern="/.*\\.kmp" /> | ||||||||
| </intent-filter> | ||||||||
| <intent-filter> | ||||||||
| <action android:name="android.intent.action.VIEW" /> | ||||||||
|
|
||||||||
| <category android:name="android.intent.category.DEFAULT" /> | ||||||||
| <category android:name="android.intent.category.BROWSABLE" /> | ||||||||
|
|
||||||||
| <!-- http:// and https:// protocols --> | ||||||||
| <data | ||||||||
| android:host="*" | ||||||||
| android:pathPattern="/.*\\.kmp" | ||||||||
| android:scheme="http" /> | ||||||||
| <data | ||||||||
| android:host="*" | ||||||||
| android:pathPattern="/.*\\.kmp" | ||||||||
| android:scheme="https" /> | ||||||||
| </intent-filter> | ||||||||
|
|
||||||||
| <intent-filter> | ||||||||
| <!-- keyman:download// deep linking to https://keyman.com/keyboards/ --> | ||||||||
|
|
@@ -204,7 +202,7 @@ | |||||||
|
|
||||||||
| </intent-filter> | ||||||||
|
|
||||||||
| <intent-filter android:priority="50"> | ||||||||
| <intent-filter android:priority="50" android:autoVerify="true"> | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is this change for?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://developer.android.com/training/app-links/verify-applinks We probably don't need to implement this because we have another
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, please make sure you call out changes like this in the PR description because it's definitely not related to the core purpose of the PR, and it is a significant change!
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, can you please split this into a separate PR for test and review? There's a fair bit of detail there that we should be careful about changing and testing. |
||||||||
| <!-- KMAPro should also be able to handle /keyboards/install links and convert to /go/package/download --> | ||||||||
| <action android:name="android.intent.action.VIEW" /> | ||||||||
|
|
||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,12 +33,13 @@ public static boolean isPermissionOK(Activity activity) { | |
| // API 30-32 | ||
| permissionsOK = Environment.isExternalStorageManager() || | ||
| checkPermission(activity, Manifest.permission.READ_EXTERNAL_STORAGE); | ||
| } else { | ||
| } | ||
|
|
||
| else { | ||
| // API 33+ | ||
| // We had to remove these MEDIA permissions from AndroidManifest.xml so these will end up failing | ||
| // https://support.google.com/googleplay/android-developer/answer/14115180?hl=en | ||
| permissionsOK = permissionsOK && checkPermission(activity, Manifest.permission.READ_MEDIA_IMAGES); | ||
| permissionsOK = permissionsOK && checkPermission(activity, Manifest.permission.READ_MEDIA_VIDEO); | ||
| // No special permissions are needed. | ||
| // - https://developer.android.com/reference/android/Manifest.permission#READ_EXTERNAL_STORAGE | ||
| // - https://stackoverflow.com/a/73630987 | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's just delete the empty else statement. We can have a note in the commit history for the change. |
||
|
|
||
| return permissionsOK; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.