-
Notifications
You must be signed in to change notification settings - Fork 8
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
use findActivity everywhere #15
base: dev
Are you sure you want to change the base?
Conversation
This only works if the surrounding Context is from an Activity, but when embedding in NewPipe proper, we are in a Fragment. So the cast will fail. Instead, let’s use the util function we already had that iterates through the `ContextWrapper`s until it finds an activity, and use that everywhere.
This works better with `ContextWrapper`s. I moved the relevant static functions into extension functions. We should think about whether we should just require `Activity` to be set on `Context` everywhere, because right now we require it on some of the code, but have defaults for other parts.
375ab60
to
4e4bb47
Compare
* | ||
* @param default: the default value if there is no activity | ||
* @param block: the block to call with the activity | ||
*/ |
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.
Consider adding @hide here, so this function does not show up on the KDoc documentation.
/** Get the [Activity] from local context. Assumes the activity exists! | ||
* @return the activity | ||
* @throws NullPointerException if there is no Activity | ||
*/ |
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.
Consider adding @hide here, so this function does not show up on the KDoc documentation.
} | ||
|
||
|
||
@Composable |
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.
Consider adding @hide here, so this function does not show up on the KDoc documentation.
@@ -298,10 +298,6 @@ class NewPlayerImpl( | |||
mutableErrorFlow.emit(e) | |||
} | |||
} | |||
|
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.
Useful cleanup, but if I'm nitpicky this code change should not be part of this PR.
I don't care too much about this though :P. However, I don't know how other devs feel about this. I'd say do it as you feel best with.
@@ -58,15 +58,14 @@ internal suspend fun buildMediaSource( | |||
): MediaSource { | |||
when (streamSelection) { | |||
is SingleSelection -> { | |||
val mediaItem = toMediaItem(streamSelection.item, streamSelection.stream, uniqueId) |
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.
Same here.
val mediaItemWithMetadata = addMetadata(mediaItem, streamSelection.item) | ||
return toMediaSource(mediaItemWithMetadata, streamSelection.stream) | ||
} | ||
|
||
is MultiSelection -> { | ||
val mediaItems = ArrayList(streamSelection.streams.map { | ||
toMediaItem( | ||
streamSelection.item, |
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.
Same here.
@@ -23,7 +23,7 @@ package net.newpipe.newplayer.logic; | |||
import net.newpipe.newplayer.data.StreamSelection | |||
import net.newpipe.newplayer.NewPlayer | |||
|
|||
interface StreamExceptionResponse |
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.
Same here.
When adding NewPlayer to NewPipe, we call it from a Fragment, which will not have a direct Activity in Context, but a wrapper class. We already had a function for that but did not call it everywhere. This fixes that.
We should probably assume we can find an Activity everywhere in the player UI. Later.
Also adds two minor changes.