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 play queue button to video details fragment #8424

Conversation

HybridAU
Copy link
Contributor

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

There is a feature (bug?) in Android 11 and up where media controls can be completely dismissed. The problem is the only way to get the the play queue is clicking on the media controls notification.

I originally opened #8419 to add the button to the main menu, but after feedback I've moved it to the video fragment.

  • Adds "Play Queue" button to the VideoDetailFragment (small player bar at the bottom of the screen)

Before/After Screenshots/Screen Record

  • Before:
    before

  • After:
    after

Fixes the following issue(s)

This PR might help address #2477, #5235 and #7208 although not exactly what they are looking for.

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

@SameenAhnaf
Copy link
Collaborator

SameenAhnaf commented May 20, 2022

#3515 should be linked too.

@TacoTheDank
Copy link
Member

TacoTheDank commented May 21, 2022

From a cursory glance, I still don't think that's the greatest location, but perhaps that's just me.

Anyway, I need to ask: where is it that you're getting the vector drawables from? It doesn't mesh well with the other icons; I don't think it's Google-designed. I'd recommend using Google-designed vector drawables where possible.

In fact, there's already a list drawable in the app: ic_list.xml. Please use this one instead.

@HybridAU
Copy link
Contributor Author

HybridAU commented May 21, 2022

Hey @TacoTheDank thanks for taking a look at this PR 🙂

I've switched to ic_list.xml that looks nicer, I drew the vector files in Inkscape and imported them in Android Studio. I'm curious when you say "doesn't mesh well" do you just mean aesthetically they are a little off (I won't be offended, I've never considered myself a graphic designer) or was there something technical like they don't render properly because of something I did or didn't do when I was importing them?

I'm not super happy with the icon location, but I'm not sure of a better place. I liked it in the main menu (my original PR) because I mainly use NewPipe as a music player. But I know that's not everyone's use case and space on the main menu is valuable.

Also I've found I introduced a bug, if I start a video playing, go to the play queue and remove the video from the play queue, I can go back to the main screen and the video details fragment is still there. If I click on the play queue icon now it crashes the app. I think I need to hide the video details fragment when the last item is removed from the queue. I'll work out how to do that and then I'll update this PR.

@triallax triallax added feature request Issue is related to a feature in the app GUI Issue is related to the graphical user interface labels May 22, 2022
@litetex
Copy link
Member

litetex commented May 22, 2022

So before this is added we should clarify if we even want this.

As far as I can see none of the following issues is somehow fixed:
#2477 - Asks for the playqueue activtiy on the main screen (inside a tab)
#5235 - Asks for a swipe up queue. Similar to YT's swipe up comments.
#7208 - Asks to open the playqueue instead when opening the videodetails?

I think this would only kind of address #3515.


Currently I don't think the button inside the mini player is the best solution.
IMHO we would need something like this:
grafik
inside the VideoDetails and not in the mini player.
Some ideas from my side:

  • Maybe an additional button here inside the actions?
    grafik
  • Or maybe adding that a long press of the playqueue button leads to the PlayQueue activtity?
  • Or maybe somewhere inside the PlayQueue section of the player?
    grafik

Anyway as #3515 has currently 5 upvotes I would postpone this because there is currently more important stuff to be done.

@Stypox
Copy link
Member

Stypox commented May 22, 2022

I've switched to ic_list.xml that looks nicer, I drew the vector files in Inkscape and imported them in Android Studio. I'm curious when you say "doesn't mesh well" do you just mean aesthetically they are a little off (I won't be offended, I've never considered myself a graphic designer) or was there something technical like they don't render properly because of something I did or didn't do when I was importing them?

I don't think he meant anything about the icon looking "bad": it just looks a little bit off with respect to other icons because it is not material-design styled like all other google icons which follow strict standards.

I would postpone this because there is currently more important stuff to be done.

I agree: I also don't think this is really a solution and we first need to decide together how to solve the problem at a deeper level.

I am closing this for now, but thank you @HybridAU for your work anyway :-). If you still want to play around with making the queue more accessible, you could try to implement #2477, which personally I think is the more elegant and fitting solution for everyone. But be advised that this is only my opinion and if you work on that your work might be rejected because others disagree.

So maybe you could try to summarize the problems people have in the various issues and make a comment explaining the possible ways (with pros and cons) a solution could be achieved. See this comment, for example: #4534 (comment)

@MD77MD

This comment was marked as off-topic.

@M1CK431
Copy link

M1CK431 commented May 30, 2022

I would like to thanks you @HybridAU for your work. You exactly did what I need and wait for more than 2 years now: adding a simple button solving an issue which haven't any workaround at this time. I follow your instruction to get the apk but didn't find it. Is there another way to get it? That button is really game changing for me since I don't have any access to notifications...

@litetex @Stypox Ok, you have your opinion about priority and what should be the finale solution, I respect.
However, what it the drawback to merge this excessively simple PR and revert it once the "perfect" solution will be implemented/available? Especially since fixing that long standing issue isn't even planned?

@Roy-Orbison
Copy link

@HybridAU Since you clearly have the ability to work with the code, would you please consider the suggestion I made? It seems about the same complexity as your PR, should work on non-touch devices, and would be a decent solution until #2477 or #5235 are implemented.

@Stypox
Copy link
Member

Stypox commented May 30, 2022

However, what it the drawback to merge this excessively simple PR and revert it once the "perfect" solution will be implemented/available?

The drawback is that that button is not in the correct place and clutters the UI, for example making the title of the video less visible and confusing people who don't use a queue. The button in the drawer was a better solution in my opinion, though still not the best.

@M1CK431
Copy link

M1CK431 commented May 30, 2022

@Stypox Ok there is (a few) less space for video title (but probably still enough to identify the video I guess, since on the screenshot the difference is five letters so not a big deal). Not sure to well understand "confusing people who don't use a queue". Isn't that part of the UI only visible when the user add at least one item in the queue? Also, what is "the drawer"?

@MD77MD
Copy link

MD77MD commented Jun 2, 2022

the drawer is the main menue ☰ ... however #8424 is much much better... i hope from stypox to reconsider

here is why:
the problem with implementing it in main menue is it would be easily blocked... because the side menu is only accessible from the main page and would be blocked the moment you open anything in newpipe

so you will have to go back to the main page to open queue which not right... queue should be easliy accessed regardless to whats opened in NewPipe

that's why @HybridAU closed the main menue pr and opend this one

please refer to #8419 (comment), #8419 (comment)

@M1CK431
Copy link

M1CK431 commented Jun 25, 2022

For other people which, like me, really need an in app access to the background player, here is the debug apk build of this PR: app-debug.zip

Hope it helps. I will try to rebase and provide an updated apk after futur releases.

@Stypox I really can't understand your point of view, honestly.

Your first arg is about "clutters the UI" but it was not a problem for existing "play/pause" and "clear" buttons... and now it's a big deal for that new "playlist" button? 🙄 If "clutters the UI" is really the issue, so remove "play/pause" and "clear" buttons and keep only that new "playlist" button, it will maximize the space for the video title while keeping an access to the background player controls (including "play/pause" and "clear"... and many more)!

Your second arg about "confusing people who don't use a queue", sorry it's not possible at all since that part of the UI is only displayed when the user add something in the queue (and because of that, we can be sure it's the right place for that new button, at least far better compare to the drawer).

You stand yourself that the "perfect" solution (aka background player in a tab) isn't even planned due to priority, but please take also into account that some user are already waiting for more than 2 years for an access to the background player from within the app (for various reasons). If not merging that PR, how long will it take until this user need is addressed, even in a basic manner?

So please please please, reconsider this PR as an acceptable compromise, waiting for a better solution 🙏 🕯️

@Stypox
Copy link
Member

Stypox commented Jul 13, 2022

@M1CK431 I get it that many people need this, and thank you for providing some thoughtful criticism.

I noticed that this PR always shows the queue button independently of whether the player is open or not, which is not the correct behavior. The queue should be openable only if there actually is one ;-)

After thinking a little bit more about it:

  • button in the drawer:
    • pro: fits in a nice place, since the drawer is made exactly for actions that bring the user to other places in the app
    • con: more difficult to implement properly, since the MainFragment does not have knowledge of whether the player is open or not, so that would need to be added
  • button in the bottom sheet:
    • pro: simpler to access?
    • pro: simpler to implement, since VideoDetailFragment already knows whether the player is open or not
    • con: it clutters the UI (I think the play/pause button should stay there, since it is a basic action, while opening the queue from there just doesn't feel right and would be unclear for most users, but this is just my opinion)
  • queue tab in the main page ([Feature request] Add a tab in Main Page for the Play Queue #2477), the final solution:
    • pro: inserts nicely in the current app structure (though we should decide what to do if the player is closed... just show a "There is no player open" message and don't let the user do anything?)
    • pro: allows users to hide it if they don't want it
    • con: takes some time to implement, but most of the code should already be there in PlayQueueActivity, which just needs to be turned into a fragment. After Refactor player and extract UI components #8170 will become a little bit simpler to take care of.

@M1CK431 do you have the knowledge and time to try to implement #2477? Otherwise yeah, maybe you are right that in this case it better to just merge a temporary solution. I don't know if "button in the drawer" is implementable properly with the current player structure, but maybe after #8170 will become simpler. So yes, you can go with "button in the bottom sheet" and I will accept the PR, even though I don't like the position of the button too much.

@MD77MD
Copy link

MD77MD commented Jul 13, 2022

thank you stypox

@M1CK431
Copy link

M1CK431 commented Jul 18, 2022

Hello @Stypox, sorry for late reply.

First of all, thanks A LOT for your message 🤗

The last time I wrote something in java was a long time ago and it was desktop oriented (a deadly simple exercice for a friend studies). I have absolutely no experience in mobile app development.
Also, despite I'm a curious person who love to learn new things, I will not have enough time to work on this, at least in the next months (not before winter).

Sorry for that, I well know it's not ideal 😞

I just build the latest master with that PR and here is the APK (inside a zip, to please github): app-debug.zip
Perhaps you could give it a try before thinking about this PR merge? I'm a personally using it since weeks now and it does the job very well 🤩

@Stypox
Copy link
Member

Stypox commented Jul 20, 2022

Just tried it, it crashes when I follow these steps:

  • tap on a video and play it in the main player
  • tap on the X in the notification (this will close the player but leave the video detail fragment open)
  • minimize the video detail fragment
  • press on the new queue button
  • NewPipe crashes because there is no player open for which to show a queue

@M1CK431
Copy link

M1CK431 commented Jul 20, 2022

Thanks a lot @Stypox for the time you spent trying that APK.

You are right, I just reproduce it even quicker:

  • tap on a video
  • scroll down from the video preview
  • press the new queue button
  • NewPipe crashes

Also, previously I was writing:

Your second arg about "confusing people who don't use a queue", sorry it's not possible at all since that part of the UI is only displayed when the user add something in the queue

But finally I was wrong because there is a situation where that part of the UI is visible while the queue is empty. Thanks to clarify that.

@HybridAU could you please add a check to hide the new queue button when the queue is empty?

@HybridAU
Copy link
Contributor Author

HybridAU commented Aug 2, 2022

No worries, I'll take a look when I get a chance and update this PR.

@Gabr-F
Copy link

Gabr-F commented Sep 1, 2022

I noticed that this PR always shows the queue button independently of whether the player is open or not, which is not the correct behavior. The queue should be openable only if there actually is one ;-)

@Stypox, a queue can be empty (and still exist), every single media player I remember to have ever used had a permanent queue window that was simply empty when nothing was queued!

What is extremely weird is the current state of having to jump through hoops, and finding "secret" commands, to open such a basic functionality as a media player's queue!

So while I strongly agree that by far the best, most intuitive solution is the tab bar*, if that requires more work, a temporary, less perfect solution is much better than nothing, especially for new NewPipe users! (which only learn about the "tapping the notification secret" by asking someone else)


* (which seems what #2477 is currently interpreted to be asking for, even though it speaks about things that don't exist anymore)

@HybridAU
Copy link
Contributor Author

HybridAU commented Sep 6, 2022

I like the idea of showing an empty queue more than the idea of hiding the button when the queue is empty.

I've updated my branch so that if you click on the play queue button when the queue is empty it won't crash and will instead just show the empty play queue page.

I'm not sure if I can re-open this PR or should I just create a new one?

@M1CK431
Copy link

M1CK431 commented Sep 6, 2022

@HybridAU thanks a lot! I will give it a try asap!
@Stypox I guess you will reopen this PR because we have all the history here?

@Stypox
Copy link
Member

Stypox commented Sep 6, 2022

Yes, you can reopen this PR

@MD77MD
Copy link

MD77MD commented Sep 6, 2022

I like the idea of showing an empty queue more than the idea of hiding the button when the queue is empty.

I agree with you... this is also great for consistency ... but i guess its would be better to try it for a week or so then give final verdict

@opusforlife2
Copy link
Collaborator

@Stypox You closed this PR, so it can't be opened by the author.

But I think a new PR should be opened instead, linking back to this PR.

@Stypox
Copy link
Member

Stypox commented Sep 8, 2022

Oh, I didn't know that. But yeah ok, go with a new PR then.

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 GUI Issue is related to the graphical user interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.