Conversation
fire-light42
left a comment
fire-light42
left a comment
There was a problem hiding this comment.
Interesting idea which would be very useful for livestream viewers.
However, this pull request contains two big issues:
1. The code is over-engineered
It only uses its own logic instead of integrating with the app. ZappingContext, ZappingSession, ZappingSessionStore, etc is way too much and way to complicated. The UI is also fully customized.
I would suggest re-using the in-video episodes popup to select other livestreams. This reduces the complexity and aligns with what users already expect. I also suggest offloading the zapping logic to a custom VideoGenerator. This removes complex logic within an already complex player.
2. Views are not in XML
If you need a custom view then please create XML files instead of creating the view programmatically.
| scaleType = ImageView.ScaleType.CENTER_CROP | ||
| setBackgroundColor(context.colorFromAttribute(R.attr.boxItemBackground)) | ||
| } | ||
| val scrim = View(context).apply { |
There was a problem hiding this comment.
All throughout the pull request you create layouts programmatically. This gives a working result, but is very difficult to inspect, maintain and review. Please use XML (or Compose) to inflate the view, and only use Kotlin to fill the view (i.e. change visibility, set texts, set images...).
| * Gives specialised players a chance to consume TV channel-style DPAD input while the | ||
| * controls are hidden. Normal player navigation remains unchanged when this returns false. | ||
| */ | ||
| protected open fun handleLiveChannelKey(keyCode: Int): Boolean = false |
There was a problem hiding this comment.
The current implementation fully hooks the up and down buttons on TV which makes normal navigation in livestreams impossible.
| val currentIndex: Int, | ||
| ) { | ||
| init { | ||
| require(channels.isNotEmpty()) { "ZappingContext requires at least one channel" } |
There was a problem hiding this comment.
This will lead to app crashes. Use debugAssert instead.
| if (!enabled) toggleZappingList(false) | ||
| } | ||
|
|
||
| private fun isZappingEnabled(): Boolean { |
There was a problem hiding this comment.
Use AppSettings(context).player.zappingEnabled instead
|
Thanks for the review. I've reworked the implementation around I've also adjusted the TV navigation so the normal player focus behavior is preserved, while channel switching is only handled when the player controls are hidden. The latest version has been manually tested with touch controls and Android TV DPAD/focus navigation, and both the build and unit tests pass. Could you please take another look when you have time? One small question: we're currently using test builds with our community for this feature. If the PR is accepted, roughly when would it become available to regular CloudStream users? |
This branch has not been deployed
2 participants
Summary
LiveZappingGeneratorto keep zapping logic separate from the core player.Validation
:app:assembleStableDebug— passed:app:testStableDebugUnitTest— passedAI assistance was used during implementation and iteration. The resulting code was manually reviewed and tested before this pull request.