Refactor existing callbacks into interface #1575
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
DavidFair
force-pushed
the
Introduce_interface_notifiable_playback_controller
branch
from
97127f6
to
dfa9f8b
Compare
|
I've taken the copy from |
I'm not sure what you mean, are you talking about the latest commit (dfa9f8b)? Blocked just means "this needs another PR first", which in this case is #1574. Both because you explicitly say so and because this PR shares changes with it. The label just helps us keep track of it so they get reviewed and merged in the intended order. So this will get unblocked when the other PR is merged |
mueslimak3r
reviewed
Apr 5, 2022
app/src/main/java/org/jellyfin/androidtv/ui/playback/VideoManager.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/jellyfin/androidtv/ui/playback/PlaybackController.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/jellyfin/androidtv/ui/playback/PlaybackController.java
Show resolved
Hide resolved
Many call-backs already exist between the VideoManager and PlaybackController. But they are created on an ad-hoc basis in various methods. By extracting these out to a method it means we can: - Remove the boilerplate of @OverRide methods in PlaybackController - Easily refactor these into a seperate class long-term - Add new handlers - Pass parameters around in the future - Not have everything dispatch to a simple onEvent. This also makes extending the interface easier where we have exoplayer only notifications (such as onPlaybackSpeedChanged). Since the VLC code can simply call the notifier without going through a PlaybackListener
DavidFair
force-pushed
the
Introduce_interface_notifiable_playback_controller
branch
from
dfa9f8b
to
d6866c3
Compare
Annotate the parameters as @nonnull for our new subscribe method, and init which is just adjacent too
mueslimak3r
approved these changes
Apr 8, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Builds on #1574
Changes
Many call-backs already exist between the VideoManager and
PlaybackController. But they are created on an ad-hoc basis in various
methods.
By extracting these out to a method it means we can:
This also makes extending the interface easier where we have exoplayer
only notifications (such as onPlaybackSpeedChanged). Since the VLC code
can simply call the notifier without going through a PlaybackListener
--
I'm not super happy with
PlaybackControllerNotifiableso I'm open to any/all suggestions on this front, maybe something along the lines ofVideoManagerSignals(coming from a Qt background)