Skip to content
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

Player overlay redesign #251

Open
wants to merge 29 commits into
base: master
from

Conversation

@Florianisme
Copy link
Contributor

Florianisme commented Jan 3, 2020

Addresses #146
VIdeo Player overlay will be redesigned making it match the vanilla Android TV style.
Existing features such as subtitles and audio track control should not be affected by this PR.

@thornbill

This comment has been minimized.

Copy link
Member

thornbill commented Jan 4, 2020

I know you had asked in the other PR... I am definitely good with improving the design of the playback controls. I do think subtitles and language should be top level controls. Less common options should be fine in a sub menu.

@Florianisme

This comment has been minimized.

Copy link
Contributor Author

Florianisme commented Jan 4, 2020

I'm currently using the leanback overlay and this is what it looks like:
Screenshot_1578150909

Seeking also works and preview thumbmails can be easily added later if the server supports it.
Custom actions (an action is a button like rewind or play/pause) are also no problem (see the language select icon on the right).
What do you think about it?

@nielsvanvelzen

This comment has been minimized.

Copy link
Member

nielsvanvelzen commented Jan 4, 2020

I like it, how does it look for chapter selection?

@Florianisme

This comment has been minimized.

Copy link
Contributor Author

Florianisme commented Jan 4, 2020

@nielsvanvelzen not yet implemented but I should be able to just implement it like the audio track switching from my last commit

Edit: Oh, did you mean that we should show the chapters when seeking?

@Florianisme

This comment has been minimized.

Copy link
Contributor Author

Florianisme commented Jan 4, 2020

Next task is migrating the Live TV controls to the new overlay and start removing the old obsolete code and layout

@Florianisme

This comment has been minimized.

Copy link
Contributor Author

Florianisme commented Jan 5, 2020

All Live TV buttons (except for the record button) were merged to the new overlay.
This is what it looks like in TV and normal movie mode:
Screenshot_1578250529
Screenshot_1578250577
Oh and the popup menus are working aswell.
Moving the buttons to different positions is obviously no issue.

@ThatNerdyPikachu

This comment has been minimized.

Copy link

ThatNerdyPikachu commented Jan 5, 2020

Question: why is there a "full screen" button there, and what does it do? (This is also in the old overlay.)

@AndreasGB

This comment has been minimized.

Copy link
Contributor

AndreasGB commented Jan 5, 2020

I think it adjusts whether the video is zoomed in to get rid of black bars, but I have never used it either.

@ThatNerdyPikachu

This comment has been minimized.

Copy link

ThatNerdyPikachu commented Jan 5, 2020

Oh, right. That is what it does, just tried it. Thanks!

@Florianisme

This comment has been minimized.

Copy link
Contributor Author

Florianisme commented Jan 6, 2020

Question: why is there a "full screen" button there, and what does it do? (This is also in the old overlay.)

Yeah, maybe we could swap that out with a less misleading icon such as this (it's literally called "aspect-ratio"):
aspect-ratio

@AndreasGB

This comment has been minimized.

Copy link
Contributor

AndreasGB commented Jan 6, 2020

I think that would make sense if this is what other TV apps are using. I can't check right now since I'm not at home.

@dkanada

This comment has been minimized.

Copy link
Member

dkanada commented Jan 6, 2020

Is there no button to switch to the next channel? Also, that three dot button doesn't seem to look that nice, isn't the material icon different than that?

@Florianisme

This comment has been minimized.

Copy link
Contributor Author

Florianisme commented Jan 6, 2020

Let's move this general discussion to the issue #257

@Mantu120

This comment has been minimized.

Copy link

Mantu120 commented Jan 9, 2020

@Florianisme
Can you add a "Chapter Selection" button after "Audio Track Selection" button?
It will be good for selecting Chapters for Movie file.

@Florianisme

This comment has been minimized.

Copy link
Contributor Author

Florianisme commented Jan 10, 2020

@Mantu120 yes, it'll have the same behavior as before

@Florianisme

This comment has been minimized.

Copy link
Contributor Author

Florianisme commented Jan 10, 2020

I think everything works now so far. So I guess extensive testing can begin :)

@Florianisme Florianisme marked this pull request as ready for review Jan 10, 2020
@nielsvanvelzen nielsvanvelzen self-requested a review Jan 10, 2020
Copy link
Member

nielsvanvelzen left a comment

The screenshots you provided look awesome and most of the code looks good too. I've got some suggestions for the code, feel free to explain why you don't agree with them. I only reviewed the code at this point, I will test it later on my devices.

import org.jellyfin.androidtv.R;
import org.jellyfin.androidtv.playback.overlay.CustomPlaybackTransportControlGlue;

public class AdjustAudioDelayAction extends CustomAction {

This comment has been minimized.

Copy link
@nielsvanvelzen

nielsvanvelzen Jan 10, 2020

Member

A lot of actions have the exact same code with only the name and icon changed. Wouldn't it be better to create an IconAction which accepts the icon as parameter in the constructor? This way you can re-use that class for all simple actions.

Maybe even add it to the CustomAction class so you don't even need to extend it.

import org.jellyfin.androidtv.R;
import org.jellyfin.androidtv.playback.overlay.CustomPlaybackTransportControlGlue;

public class RecordAction extends CustomAction {

This comment has been minimized.

Copy link
@nielsvanvelzen

nielsvanvelzen Jan 10, 2020

Member

(addition to the previous suggestion for the action classes) adding a second icon parameter to the (to-be-made) constructor could trigger the active/inactive behavior. When the second icon is null it will always use the first one.

This comment has been minimized.

Copy link
@Florianisme

Florianisme Jan 10, 2020

Author Contributor

Good point, but as you can see, I check which type of action is clicked in the onCustomActionClicked method in the CustomPlaybackTransportControlGlue class. But maybe I can instead use some sort of enum (eg. ACTION_SUBTITLES) which I pass in the IconAction object on initialization which I can later check against.


import java.util.List;

public class CustomActionClickedHandler {

This comment has been minimized.

Copy link
@nielsvanvelzen

nielsvanvelzen Jan 10, 2020

Member

I don't really like how this one class is responsible for all click handlers, I'd rather see them added to their respective action class.

notifyActionChanged(action);
}

public void onCustomActionClicked(Action action, View view) {

This comment has been minimized.

Copy link
@nielsvanvelzen

nielsvanvelzen Jan 10, 2020

Member

See my previous comment on the way these handlers are implemented. Would be better if you could just do action.onClicked(); or something like that

This comment has been minimized.

Copy link
@Florianisme

Florianisme Jan 10, 2020

Author Contributor

Good point!
But then I will have to keep an action class for every action I add.


import androidx.leanback.widget.PlaybackSeekDataProvider;

public class CustomSeekProvider extends PlaybackSeekDataProvider {

This comment has been minimized.

Copy link
@nielsvanvelzen

nielsvanvelzen Jan 10, 2020

Member

I'm not really sure what this class does. Is it providing the possible "seek positions" to the player? And if so, does this mean that whatever I watch it can only seek to 30 positions? Which would mean that if I watched a movie of 30 hours I would skip one hour every time?

This comment has been minimized.

Copy link
@nielsvanvelzen

nielsvanvelzen Jan 10, 2020

Member

I just confirmed this myself while testing. I think adding stepts for every x seconds would be better.

This comment has been minimized.

Copy link
@Florianisme

Florianisme Jan 10, 2020

Author Contributor

Yes, I also think I saw a code example of a quick seeker, which means the faster you click (or the longer you hold to the right), the faster you seek

This comment has been minimized.

Copy link
@nielsvanvelzen

nielsvanvelzen Jan 16, 2020

Member

Can you update this one so it works better for movies / long episodes? 30 steps is enough for a 30 minute show but it isn't for a 3 hour movie. The behavior you mentioned you saw in an example would be awesome to have

@nielsvanvelzen

This comment has been minimized.

Copy link
Member

nielsvanvelzen commented Jan 10, 2020

So far it looks very good. Some things I noticed while testing in my development emulator:

  • When seeking to the end the player crashes (can't really find a good stacktrace for this right now)
  • When choosing a chapter pressing the LEFT or RIGHT button at the edge (so left when first item is selected) causes the player controls to show in the background
  • One particular show in my library was unable to play, the player instantly crashed when opening. I tried it multiple times (restarting the app too). At some point it suddenly worked somehow. stacktrace.txt

edit: I also see a lot of unused imports, try to use the "Optimize Imports" quickfix for this in Android Studio.

@Florianisme

This comment has been minimized.

Copy link
Contributor Author

Florianisme commented Jan 10, 2020

First of all, thanks a lot for the comprehensive review!

When seeking to the end the player crashes (can't really find a good stacktrace for this right now)

I cant reproduce it responsibly but when I do it says that
stacktrace.txt ExoPlayer was null so maybe there is something conflicting with your recent PR and these changes.

When choosing a chapter pressing the LEFT or RIGHT button at the edge (so left when first item is selected) causes the player controls to show in the background

Yes I also noticed that before but I havent found out why it worked before when the old overlay was basically behind the chapter selection aswell.

One particular show in my library was unable to play, the player instantly crashed when opening. I tried it multiple times (restarting the app too). At some point it suddenly worked somehow. stacktrace.txt

Maybe the check is too early before everything is initialized properly. I thought about only adding the title, current pos and all the actions after getting a callback from the CustomPlaybackOverlayFragment when everything is done (kinda like the old updateDisplay() method in there)

edit: I also see a lot of unused imports, try to use the "Optimize Imports" quickfix for this in Android Studio.

I never notice that, will do :)

@nextlooper42

This comment has been minimized.

Copy link
Contributor

nextlooper42 commented Jan 14, 2020

I hope that audio and subtitle button will remain onscreen all time, every non english person use them tbh :D

@Florianisme Florianisme requested a review from nielsvanvelzen Jan 16, 2020
Copy link
Member

nielsvanvelzen left a comment

Sorry for the late review, I thought you were still making some changes. I left a comment on the seekbar implementation.
I noticed you haven't changed anything in the actions implementation. It does work but I think it could be improved a bit (see open discussions for those).

@Florianisme

This comment has been minimized.

Copy link
Contributor Author

Florianisme commented Jan 17, 2020

No, I was waiting for your response regarding the single action items.

See my previous comment on the way these handlers are implemented. Would be better if you could just do action.onClicked(); or something like that

If I do that I'll have to keep a class per action. If not all actions can be merged into a single action class..

@nielsvanvelzen

This comment has been minimized.

Copy link
Member

nielsvanvelzen commented Jan 17, 2020

You can add the click function as parameters. In Java this can be done like this:

public interface ActionClickHandler {
    void onClicked(Context context);
}

public class TestAction extends CustomAction {
    public TestAction(Context context, ActionClickHandler onClick) {}
}

In Kotlin it could look like this:

class TestAction(
        private val context: Context,
        private val onClick: (context: Context) -> Unit
) extends CustomAction()

If you do it like this all actions can be implemented "inline" and don't need their own classes.

@Florianisme

This comment has been minimized.

Copy link
Contributor Author

Florianisme commented Jan 17, 2020

@nielsvanvelzen I know what an interface is but I won't implement the logic for clicking on an action in its initialization. That would be a big mess.

@nielsvanvelzen

This comment has been minimized.

Copy link
Member

nielsvanvelzen commented Jan 17, 2020

In that case I'd suggest creating separate action classes with their own click implementation.

@thornbill thornbill added this to In progress in v0.12.0 Jan 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
v0.12.0
  
In progress
8 participants
You can’t perform that action at this time.