-
Notifications
You must be signed in to change notification settings - Fork 254
feat: enable sharing multiple files to Kore #1043
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
Conversation
The new function should act identically, but is not conflacted with URL conversion, and avoids a potential duplicate `intent.getData();` call.
the context being used here does not have to be the application context. Shortening this name leads to more self-documenting code (such as when context == some Activty, as it sometimes is), and is much shorter, to boot.
before this, the music queue would _always_ be cleared, even if the user choose the "Queue on Kodi" share action. This is hardly what you expect when you press a button named "queue". This also brings Kore's behavior more in line with similar code (e.g. startPlaylistIfNoActivePlayers in LocalMediaFileListFragment). Some of the language here was editied to be a bit less video-centric, as well.
I also think throwing around ! in your code makes it harder to read
This else is also hit when the intent contains mixed content. Not the most elegant solution for mixed content, but the original behavior of this block was clearly: "Don't know what to do? Must be a video I guess". This retains that spirit, for better or worse. Other, slightly overkill option is to extend the PLAYLISTID types (Kodi's API recognizes -1, which Kodi code defines as 'NONE').
@@ -178,32 +195,6 @@ private Uri getUrlInsideIntent(Intent intent) { | |||
return null; | |||
} | |||
|
|||
private Uri getShareLocalUriOrHiddenUri(Intent intent) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function has basically been "inlined" into the new extractUriFromIntent()
, and is hence redundant.
I also don't really like how it casts to URLs, then back to URIs, and then later in the code, back to a URL again.
finish(); | ||
return; | ||
} | ||
|
||
String url = toPluginUrl(videoUri); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the video centric language was unneeded imo
|
||
if (contentUri == null) { | ||
Bundle bundle = intent.getExtras(); | ||
contentUri = (Uri) bundle.get(Intent.EXTRA_STREAM); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed this to intent.getParcelableExtra(Intent.EXTRA_STREAM)
, because I think that is the preferred variant nowadays.
(source: https://developer.android.com/training/sharing/receive#handling-content)
This was the original convention in this file (tho not necessarily in the codebase), probably to avoid overly long lines. I'm happy to revert this change, and have all .get() calls not open a newline.
I'll fix the conflicts in a bit! (I've been using this for a while now, works well so far) |
Looks good, thanks for this. Squashing and merging as usual |
This PR enables sharing multiple audio/video/image files to Kore. Previously, sharing multiple files to Kore required a share action per file.
Use cases
My main use case is sharing ("streaming") whole albums from a music player app — previously, I had to queue every song one by one, which quickly got tiresome. An example of an app that can share whole albums is Auxio.
Additionally, I can see this PR being useful for creating slideshows from your camera roll.
Notable changes
Sorry for the long diff, but this change called for some refactoring. The real meat is in the first two commits, so its wise to start there. The rest are revertible cosmetic tweaks (e.g. renaming vars) and minor follow up fixes.
getData()
), which frankly was a bit all over.HttpApp
to make read permission forcontent://
URIs more persistent, usingcontext.grantUriPermission
. In my tests, this read permission is now only revoked when Kore is killed. Having this permission be more persistent is essential for the use case of this PR: without it, Kore would hit permission errors after just one song.applicationContext
to justcontext
inHttpApp
(see ca2aa4a). This aligns with actual usage. In fact, it's precisely because we pass theActivity
context instead of the application context that the current code works (this blogpost discusses URI permissions in greater detail)OpenSharedUrls
, that proved really helpful: itsonSuccess
callback now signals "hey, I'm done with all the URLs", so that we avoid runningfinish()
prematurely.Discussion
OpenSharedUrls
such that it explicitly handles collections of URIs of different types. This seems overkill, though.UNKNOWN
playlist type (equal to-1
), and handle that. There is precedent for anUNKNOWN
type both in the API and Kodi'splaylistTypes.h
.LocalMediaFileListFragments
, which I tried to extract into a shared class. I'm not sure why I eventually abandoned this approach (maybecontent://
URIs were more difficult to convert toLocalFileLocation
than I initially expected), but I'm happy to reconsider this or other approaches.OpenSharedUrl
(singular) action, if desired. But frankly, that code would see no use currently.Future work