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

Improve player UI and navigability for Android TV #7963

Merged
merged 3 commits into from
Mar 15, 2022

Conversation

Stypox
Copy link
Member

@Stypox Stypox commented Feb 27, 2022

What is it?

  • Bugfix (user facing)
  • Feature (user facing)
  • Codebase improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

  • As demonstrated in Remove large-land player layout: not actually used #7894 (comment) the in-player play queue controls were not reachable on Android TV; this PR fixes that.
  • While fixing the issue above I noticed controls kept showing up at random below the play queue. I also fixed that.
  • As requested in Remove large-land player layout: not actually used #7894 (comment) I made it so that player popup menus (e.g. quality) now use correct dark background theming, instead of setting a fixed dark color, so that when you touch on an item you now get the ripple or the focused color, instead of no feedback at all.
  • The above issue also required me to use androidx PopupMenus, otherwise strange things would happen on API 19 (it took me an hour to figure this out...)

I tested everything both on emulated phone API 19 and on emulated Android TV API 29, and everything worked well.

Fixes the following issue(s)

APK testing

The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR.

Due diligence

@Stypox Stypox changed the title Android tv player Improve player UI and navigability for Android TV Feb 27, 2022
@litetex litetex added the Android TV Issue is related to Android TV label Feb 27, 2022
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@peat80
Copy link

peat80 commented Feb 27, 2022

Great improvements. Works fine on real androidtv too. 👌

@peat80
Copy link

peat80 commented Feb 27, 2022

Should probably fix this recently opened issue too:

implement repeat/loop for Android TV #7935

@litetex

This comment was marked as resolved.

@Stypox
Copy link
Member Author

Stypox commented Feb 28, 2022

@litetex I think what he meant is that this PR also fixes that issue (and it indeed does, even though it doesn't provide the exact solution requested there)

@townkat
Copy link

townkat commented Mar 6, 2022

@peat80, @Stypox, i cannot find how i can use repeat/loop on this developement version on my Android 9 tv, also this dev version crashes after few seconds on some long clips for me

@peat80
Copy link

peat80 commented Mar 6, 2022

Click to expand

NewPipe android-tv-player_20220307_004734

@townkat press the marked button in the player ui and you will find out. 😀
Buttons were always there but without this pr they are not focusable with remote.

Have you an example url for a crashing clip to try? I personally did not experience any crashes yet while using this pr.

@townkat
Copy link

townkat commented Mar 7, 2022

Click to expand

IMG_20220307_190916

i do not have that button as you can see, also i still have the release version installed too, but the screenshoot is from the dev version

i am searching now for a problematic clip to link, seems to work fine now, if i find one i will post the link

@peat80
Copy link

peat80 commented Mar 7, 2022

@townkat
There has to be more than one video in the queue for this button to appear. (It would make no sense to show when there is only one video. 😂)
If you enqueue a playlist or more than one video it should appear.
(Go to a playlist for example and press "play all" and the button should appear in the player ui.)

So if you have the "auto-enqeue next stream" setting disabled and you start a video it does not show. (Like in your screenshot. 😁)

@townkat
Copy link

townkat commented Mar 10, 2022

@peat80
ohh, thanks alot for helping,
it sems to work this way,
but i still think "repeat current clip" should be in the tempo/pitch window, unrestricted by using a playlist, it is not intuitive how it is now, but still good that the function is present at least

another observation as this bug fix seems to be about selecting interface items, i still cannot return to the playback bar to restore the playing clip, with the remote, after browsing some channel or search or go to menu/settings etc. with a clip playing, you know, the bar on the bottom with the current playing clip

thank you so much

@litetex litetex added the GUI Issue is related to the graphical user interface label Mar 14, 2022
@litetex
Copy link
Member

litetex commented Mar 14, 2022

but i still think "repeat current clip" should be in the tempo/pitch window, unrestricted by using a playlist, it is not intuitive how it is now, but still good that the function is present at least

another observation as this bug fix seems to be about selecting interface items, i still cannot return to the playback bar to restore the playing clip, with the remote, after browsing some channel or search or go to menu/settings etc. with a clip playing, you know, the bar on the bottom with the current playing clip

Please open new issues for these problems (if there aren't already some open 😉)

@litetex litetex self-requested a review March 14, 2022 17:16
Copy link
Member

@litetex litetex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM

Did a quick test - couldn't find any problems
The testers also said that everything is ok → approved

@litetex litetex merged commit 00e4631 into TeamNewPipe:dev Mar 15, 2022
@Stypox Stypox mentioned this pull request Apr 16, 2022
12 tasks
@Stypox Stypox deleted the android-tv-player branch August 4, 2022 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Android TV Issue is related to Android TV GUI Issue is related to the graphical user interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

implement repeat/loop for Android TV
4 participants