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

Add a secondary control panel to video detail fragment #4534

Merged
merged 9 commits into from
Jan 17, 2021

Conversation

Stypox
Copy link
Member

@Stypox Stypox commented Oct 16, 2020

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

This PR readds the Share, Open in browser and Play in kodi buttons to the video detail fragment interface, after many users gave negative feedback about the segregation of those buttons inside the player. In particular, I added another row of controls (below the one containing Add to, Background, Popup and Download), which is shown only when the description is expanded. In #4039 there have been many other solutions proposed, but unfortunately the scrollable-list-of-buttons one would be a nightmare to implement properly while still making it look good on every device.

This PR also removes the description from then dropdown menu and moves it to a tab alongside related streams and comments.

Since tabs were difficult to find out about in the previous version, since some small and almost invisible dots were shown, this PR changes the look of the bottom tab layout.

TODO

  • Add TooltipTexts to the tabViews
  • Fix tabs visible when in fullscreen
  • Animate secondary controls toggle dropdown button

Fixes the following issue(s)

Fixes #4039

APK testing

@opusforlife2 @Younes-L @sanityormadness @WeAreJMJ @nbmrjuhneibkr @cyber199 could you test this apk? Would it suit you?
app-debug.zip

Due diligence

@opusforlife2
Copy link
Collaborator

That looks great!

@nbmrjuhneibkr
Copy link

Still not as good as the top menu bar that existed before v0.20. What was the reason for removing it, anyway?

Way too much space is occupied just by two buttons, look at all that emptiness between them. My suggestion for improving this is to put icons and text in the same row.

This doesn't fix another regression introduced in 0.20: previously top menu bar also contained video quality selection, which is now inaccessible without starting playback first. As I said here #4039 (comment) ,

if I know that I want to play video in a quality that is different from the default, why would I want to start playing it in the default quality first (wasting limited mobile data)?

@opusforlife2
Copy link
Collaborator

Yes! My beloved Share button! It is so good to tap you again!

@opusforlife2
Copy link
Collaborator

if I know that I want to play video in a quality that is different from the default, why would I want to start playing it in the default quality first (wasting limited mobile data)?

@nbmrjuhneibkr There is a Limit quality setting for mobile data, no? (I feel like this has been discussed before.)

@wb9688
Copy link
Contributor

wb9688 commented Oct 16, 2020

Didn't we agree on a swipeable thingy? Imho this implementation wastes to much space (based on looking at the screenshot).

@opusforlife2
Copy link
Collaborator

I think that will be fixed by adding more buttons.

@nbmrjuhneibkr
Copy link

There is a Limit quality setting for mobile data, no?

There is, but it is a global setting that automatically applies to all videos. You can't deny that previous implementation offered more choice to the user.

@avently
Copy link
Contributor

avently commented Oct 16, 2020

First of all, thank you for trying to find a solution for this problem.
This PR makes the description in very bottom of the page which hurts usability. The solution proposed somewhere was to add a new button in the row near the download button or (my choice) to replace a download button in some kind of "More" button which after click shows second line like you did in this PR.
It needed to prevent always shown second row which needed only one-two times in month (in my case). I often read description so it's important to make it wasting as less space as possible. Some guys have a small screen so it's important for them too.
Don't you think it's a better choice to show the "More" button?

@avently
Copy link
Contributor

avently commented Oct 16, 2020

Didn't we agree on a swipeable thingy?

This solution is great too. The only thing that needs to be done is a horizontal scroll view. Or similar thing that allows pages like android launcher

@opusforlife2
Copy link
Collaborator

What about shifting the 4 action buttons to the left slightly and reducing the space between them, to make space for an expandable arrow button like we have for the description? It could be of the same size as the description arrow.

@Younes-L
Copy link

Still not as good as the top menu bar that existed before v0.20. What was the reason for removing it, anyway?

Way too much space is occupied just by two buttons, look at all that emptiness between them. My suggestion for improving this is to put icons and text in the same row.

This doesn't fix another regression introduced in 0.20: previously top menu bar also contained video quality selection, which is now inaccessible without starting playback first. As I said here #4039 (comment) ,

if I know that I want to play video in a quality that is different from the default, why would I want to start playing it in the default quality first (wasting limited mobile data)?

In some ways this is better, because the buttons are placed lower making them more accessible and easy to reach compared to the action bar in 0.19.8 and older.

That empty space you mentioned could be used to readd the quality selector, what do you think ?

@TobiGr
Copy link
Member

TobiGr commented Oct 17, 2020

What about shifting the 4 action buttons to the left slightly and reducing the space between them, to make space for an expandable arrow button like we have for the description? It could be of the same size as the description arrow.

Having too many collapsible UI elements looks clumsy. I'd go for the swipeable solution as discussed earlier this week. (we really need the tours....)

@Stypox
Copy link
Member Author

Stypox commented Oct 17, 2020

Way too much space is occupied just by two buttons, look at all that emptiness between them.

I don't think there is too much emptiness between them, at the end of the day even if some space is wasted, that happens only when the description is open, and it would not bother otherwise

This doesn't fix another regression introduced in 0.20: previously top menu bar also contained video quality selection, which is now inaccessible without starting playback first.

I agree this should be fixed, and I will fix it by making use of this same menu, just not in this PR ;-)

Didn't we agree on a swipeable thingy? Imho this implementation wastes to much space (based on looking at the screenshot).

As I said that would be a nightmare to implement, due to too many nested views listening for scrolls and touches. Also, adding a quality selector to a scrollable layout would just look bad, while making space for it in the current layout would not be a problem.

Don't you think it's a better choice to show the "More" button?

I don't think that's a viable solution, as it would require removing the very-used download button and making that part of the ui asymmetric, which would probably look bad.

I'd go for the swipeable solution as discussed earlier this week.

Mmmh ok, if you all agree that's the way to go then I will try implementing it, though I don't know if I will manage to yield a great result

@ghost
Copy link

ghost commented Oct 18, 2020

@Stypox Very Nice :D

As for suggestions - It would be nice to have the two buttons exposed all the time.

Also I saw that you would also be adding the resolution selection in the new area :) So exposing it always would make it easier to use.

In regards to swiping gestures, you would need a clear visual indication of the feature which would mean hiding the the download button :(

@ghost
Copy link

ghost commented Oct 18, 2020

Pushing the description down doesn't affect usability imho. Because it's hidden by default.

@Stypox
Copy link
Member Author

Stypox commented Oct 31, 2020

What do you think about making the description a tab, side by side with comments and related streams?

@opusforlife2
Copy link
Collaborator

@Stypox Would have to see a mock up or test apk to decide.

@Stypox
Copy link
Member Author

Stypox commented Nov 1, 2020

I moved the description to a tab and I really like this solution. What do you all think? app-debug.zip
@opusforlife2 @Younes-L @sanityormadness @WeAreJMJ @nbmrjuhneibkr @cyber199

@Stypox
Copy link
Member Author

Stypox commented Nov 1, 2020

The description tab could even contain more information, such as the meta information added by @TobiGr and the various other metadata provided by services

@opusforlife2
Copy link
Collaborator

Initial impression: I like it.

But if we are going this route, then we absolutely must have tab headers, just like the Main Activity. Description should be the first tab.

@daufinsyd
Copy link

daufinsyd commented Nov 26, 2020

Hi there,
I'm trying the linked apk and so far so good; I'm happy you added the feature back !
One thing, the popup asking what should do when sharing a link from another app doesn't show all options (cf screenshot).

Is that intended ?
Thanks !

@Stypox
Copy link
Member Author

Stypox commented Nov 27, 2020

@daufinsyd that's unrelated from this PR and was changed recently, after unified ui merged the player with the details view. So now for videos only one of "Video player" and "Show info" will be shown based on the autoplay setting

@Stypox
Copy link
Member Author

Stypox commented Nov 27, 2020

Rebased, new apk: app-debug.zip

@daufinsyd
Copy link

Using it since 3 days (mainly listening to music in background), sharing to/from other apps, using queues and other stuff. Seems pretty stable to me. I'll come back if I find a bug.

@TobiGr
Copy link
Member

TobiGr commented Jan 16, 2021

I tested everything on phones, tablets and a TV emulator. Works great! However, I did not run an Android 4.4 emulator yet and I'd like to review the code one last time.

There is a small bug on TVs which allows to select items of the minimized player when the video description tab is shown and one is switching from the description to the related videos and vice versa. However, I cannot reproduce it in reliably and thus provide no further info.

Suggestion for a later update: when the next and related videos are hidden and tablets / TV, move the "share" and "open in browser" buttons in the first (and thus only) row next to the download button.

@Stypox Stypox force-pushed the secondary-controls branch 3 times, most recently from b04ed34 to e43c967 Compare January 16, 2021 17:41
@XiangRongLin XiangRongLin added the feature request Issue is related to a feature in the app label Jan 17, 2021
@Stypox
Copy link
Member Author

Stypox commented Jan 17, 2021

@TobiGr I rebased again (it was tough, this time) and added an animation for the secondary controls toggle dropdown button.

@TobiGr TobiGr merged commit 156adaa into TeamNewPipe:dev Jan 17, 2021
This was referenced Jan 18, 2021
@Stypox Stypox deleted the secondary-controls branch March 19, 2021 10:31
tossj pushed a commit to tossj/NewPipe-legacy that referenced this pull request Apr 20, 2021
…trols

Add a secondary control panel to video detail fragment
@skyGtm skyGtm mentioned this pull request Sep 24, 2021
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issue is related to a feature in the app
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression - Share and other buttons in video details