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 enqueue background choice for share service. #5850

Closed
wants to merge 1 commit into from

Conversation

chouhanyang
Copy link

Add a new menu item in share menu to enqueue into background player.

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

  • Add string resources for the new menu item.
  • Add logic in menu/dialog creation.
  • Add handler to enqueue stream into background player.
  • Screenshot:
    device-2021-03-11-171859

Fixes the following issue(s)

APK testing

https://github.com/SavantOne/NewPipe/releases/download/v0.20.11-patch/NewPipe-v0.20.11-patch.apk

Due diligence

@Stypox
Copy link
Member

Stypox commented Mar 18, 2021

Thank you! :-D
Instead of "Enqueue in the background" the button should show "Enqueue", it should be shown only when there is already a player open and when pressed it should enqueue automatically to the correct player. You may take a look at the functions in org.schabi.newpipe.player.helper.PlayerHolder, in particular isPlaying() and getType(). You may also take a look at #4425 and at this snippet it introduced.

@AudricV AudricV added feature request Issue is related to a feature in the app GUI Issue is related to the graphical user interface labels Mar 18, 2021
@chouhanyang
Copy link
Author

@Stypox Thanks for the quick response.

Before I make changes, just want to sync up with you to make sure that I fully understand what you're suggesting as I'm catching up with your previous changes. I have 2 points of confusion which I'd like to run by you.

  1. Showing "Enqueue" only when there's a player open doesn't seem working for the use case I mentioned. Consider the following scenario:

    • Freshly install NewPipe App.
    • Share a stream from YouTube App (no "Enqueue" options now).
    • Select "Background player" and click "Always" on share menu.
    • Continue to share more streams from YouTube App.
    • No share menu or "Enqueue" option will be seen because "Always" suppress the share menu entirely. Ooops!
    • Now I'm trapped.

    It is not clear to me how this UI would work for people who chose "Background Player" + "Always". Are you suggesting that we show share menu unconditionally (with enqueue option) when there's a stream currently playing even after "Always" has previously been clicked?

  2. As you pointed out in Replace specific enqueue options with one #4425, if there's currently a stream playing, the enqueue option should automatically choose the correct player to enqueue. However, I saw you completely gutted the enqueue function in getResultHandler() in this commit. That was the reason why I only added enqueue background in the first place with minimal impact.

    Now assuming we can enqueue automatically to any opened player per your suggestion, wouldn't it be even better to avoid "Enqueue" option completely and maintain currently simple UI?

On top of that, it would be nice to hear high level goals you have in mind so that we are on the same page. Maybe something like:

  • Keep UI minimal. Better less options than more options.
  • Automatically enqueue if there's already a player open.

These are just my guesses and would like you hear your thoughts. Cheers!

@Stypox
Copy link
Member

Stypox commented Mar 19, 2021

Sorry for not being clear, it's difficult to decide on the best behaviour. Here is a poll, please vote on this comment using the emoji before the option descriptions (i.e. 🚀 for 1, 👀 for 2.1, 🎉 for 2.2, 😄 for 2.3, 👍 for 3). Feel free to point out other pros or cons.

1 🚀 Automatic enqueue when selecting "Play on XX" and there is a player already open:

  • ✔️ it doesn't introduce more options
  • ✔️ it takes less time to share many urls to NewPipe and enqueue them all automatically
  • ❌ it's difficult to find out about
  • ❌ it can be confusing, since it may seem like nothing happened if the stream was enqueued but the user is not aware of it

2 Adding an "Enqueue" action in the share menu that shows up only if there is a player already open and that player is compatible with the shared stream type; all other actions replace the current queue (as it is done now):

  • ✔️ it introduces a new button in the share menu, but it's just a single one and that's consistent with the long-press menu
  • ✔️ it is clearer what each button does
  • 🟡 it would be difficult to define what that button should do if it is set as default, something is shared and either there is no player open or the opened player is incompatible with the shared stream type; here are three options for that case:
    • 2.1 👀 show the share menu and let the user decide (but hide the "Always" button in that case):

      • ✔️ does not introduce more options and imo just feels natural
      • ❌ wastes a little time (negligible in my opinion) for the first opened stream in a row
    • 2.2 🎉 add an option to settings to choose which action to do when "Enqueue" can't be done:

      • ✔️ the most time is saved, since the choice is always automatic
      • ❌ introduces one more option to find out about
    • 2.3 😄 show stream info:

      • ✔️ no other options needed
      • ❌ no way to choose directly the action to perform

3 👍 Add a switch in settings named "Always enqueue shared stream if a player is already open":

  • ✔️ is logically simple and easy to implement
  • ✔️ the most time is saved, since the choice is always automatic
  • ❌ it requires adding a new setting
  • ❌ it's difficult to find out about

In my opinion the best option is 2.1, since I consider the con negligible. What do you think? Vote! @chouhanyang @TobiGr @opusforlife2 @B0pol @mhmdanas @TiA4f8R @vkay94 @TacoTheDank @XiangRongLin @MD77MD @s1awek @shivasagarrao @wilzbach @T5000 @Bruceforce @ShareASmile @89z @thinsoldier @atmosfar @SameenAhnaf

@Stypox
Copy link
Member

Stypox commented Mar 19, 2021

@89z I didn't include that option since there are too many open questions which would either require a new setting or be unintuitive for new users:

  • a specific enqueue button would need to be added for each player (maybe you only need background, but other people could need the others)
  • what should the app do if there is no player open? Enqueue to what?
  • what instead should happen if there is already a player of a different type open? Change player or enqueue to that player?

I think your use case would be suited well-enough by the 2nd option, either 2.1 or 2.2.

  • 2.1 with Enqueue set as default/Always, for the first link you open while the player is closed you get prompted what to do with it (it would be background player for you), from then on each link you open automatically enqueues without any further action.
  • 2.2 with Enqueue set as default/Always, for the first link you open while the player is closed a default player you can choose in settings is started (it would be background player for you), from then on each link you open automatically enqueues to the original player without any further action.

You may also like option 1, which is the same behaviour as older NewPipe versions

@Stypox
Copy link
Member

Stypox commented Mar 19, 2021

@Stypox I think, both 2.1 (suggested in #4779) and 3 (suggested in #5840) should be added. Users are given the flexibility to choose from automatic or manual popup, background and full screen video stream. Why not the same ability for "Enqeue"?

Because Enqueue relates to the player currently open, having an Enqueue that changes the player is inconsistent and we already decided to remove that for long press menus.

@opusforlife2
Copy link
Collaborator

@Stypox I am in favour of having a separate background play service (#4460). In that case, having a separate Enqueue button for the Background player would make the most sense.

@Stypox
Copy link
Member

Stypox commented Mar 19, 2021

@SameenAhnaf why wouldn't your sister also be suited by 2.1? And I disagree that adding complexity is a good thing, none of the people I suggested NewPipe to would be so passionate to spend a day nitpicking their preferred settings, and this is not just my opinion, I think it's also written in material guidelines.

@opusforlife2 that's totally a different issue though

@opusforlife2
Copy link
Collaborator

Would it not be less work in the future if this were to be implemented keeping that in mind? Or am I counting eggs before they hatch?

@opusforlife2
Copy link
Collaborator

@SameenAhnaf Please give constructive feedback only. And calm down! It's just an app. Not worth arguing over.

@s1awek
Copy link

s1awek commented Mar 19, 2021

I don't know if this thread is the right time and place, but I have noticed an issue with this patch.
Steps to recreate the problem:

  1. Close the application and make sure it is not running in the background
  2. Add video from YT via share menu (background playback widget should appear)
  3. Click an application icon on the desktop to open it

At this point, the application for me most often stops working and shuts down without any warning.

@opusforlife2
Copy link
Collaborator

@SameenAhnaf Such is the way of software development. Sometimes developers do things that some users don't agree with.

The best thing the user can do to empower themselves is to learn programming and implement whatever they want without any obstruction. The second best thing is to provide alternative solutions to the described problem. Keep proposing ideas until the developers agree with one, and if you run out of ideas, then wait for someone else to propose yet more ideas.

In any case, whatever you do, make sure you never hurt the developers in your communications. Even in an extreme case where you find that the next app update is just a splash screen and nothing works. It's volunteer work.

@opusforlife2
Copy link
Collaborator

@s1awek If you encounter this bug only on this PR's apk, then this is definitely the right place to report it. Make sure to provide a crash report if you get one.

@s1awek
Copy link

s1awek commented Mar 19, 2021

@opusforlife2 there is no option to send crash raport, the app just stops working without any message or warning.

@opusforlife2
Copy link
Collaborator

@s1awek Alright. In that case, describing the steps is good enough.

@Stypox
Copy link
Member

Stypox commented Mar 19, 2021

@SameenAhnaf no problem, I don't think you were too bothering. ;-)
I agree we should always do polls, and we also should have done them in the past, it's just something we never thought about doing before.
Could you further explain your usecase and why 2.1 wouldn't work for you?

@chouhanyang
Copy link
Author

@Stypox Thanks for the elaboration. After weighing those options, I was wondering something like 2+3. Here are my observations:

  • We need to have a player type (video/popup/background) selected so we know which type of player to open by default if none running.
  • We don't want to add 3 extra enqueue options for each player type. However, this can be solved with a single switch/checkbox as you mentioned in 3.
  • Since this "enqueue" concept is to be bound for the currently-opened player. The switch/checkbox should be named as "Enable stream queue" or something like that, and it should be orthogonal/independent to the player type options. It is a feature to turn on/off, as opposed to a verb "enqueue" implying immediate action.
  • We want this new switch/checkbox to be easily found. However, we don't want basic users to stumble on it by accident. This can be solved by adding a "Advanced Options" drawer guarding the switch/checkbox. Basic users usually won't go great length to open advanced options if they don't know what they are doing.

Here's my mock-up UI:
device-2021-03-20-000231

In this dialog design:

  • Advanced options are closed by default, as well as the "Enable stream queue" is disabled by default. So shouldn't be a problem for basic users today.
  • "Always" is only enabled when a player type is selected. And it will work as of today by playing stream immediately so basic users won't be confused.
  • Annoying advanced users (like me!) would like to use "Background player" + "Enable stream queue" + "Always". So this share dialog will never be shown again. It will first try to enqueue to the currently-opened player if exists per your suggestion. And then, fallback to the chosen player type selected if none opened.

I think this can be a good compromise. Let me know what you guys think.

@chouhanyang
Copy link
Author

Thank you for your compliment and suggestions @SameenAhnaf.

However, "Same action for next streams" should be added instead of "Enable stream queue". Because, we may want to "Add to playlist" (suggested in #5858) or "Download" multiple videos at the same time. Enqeue option seems to be solely for playback as the meaning suggests so. This same action will continue until the app is closed on the background.

Indeed, the wording can be further improved. The point that @Stypox keeps bringing up is that a stream may be enqueued to different player depending on which one is currently-opened. Also, the stream that users choose to share at the moment would be affected immediately too, as opposed to next streams. Some alternatives could better describe it meant for the switch/checkbox like:

  • Prioritize stream queuing (meaning try to enqueue a stream first if possible).
  • Attempt stream queuing (same, but less geeky).
  • Try enqueue stream first (plaintext).

Appreciate any help to make the UI intuitive, since I'm not native English speaker. Also, to make sure we are on the same page about what this piece of UI really means. Cheers!

@MD77MD
Copy link

MD77MD commented Mar 20, 2021

@chouhanyang

im with stypox, a simple enqueue button (the same from long press menu) is better then a specific enqueue in background because it would work with all players.

anyway in a different topic, i would like your opinion (and everyone else for that matter) on adding an option to the long press menu - called "start playing from here" - that would only show up within playlists. the reason is at current setup, it is very hard to start playing a playlists other then from the beginning. (try it to have a better understanding). although there are some workarounds, but at the end they are workarounds and not proper solutions.

if you guys think its a good idea, then i would be happy to open a new issue to further discuss it.

@chouhanyang
Copy link
Author

@MD77MD Thank you for your input.

Would you mind elaborating your use case step by step because I am not familiar with the main UI and unable to see the "enqueue" button in the menu from long press menu as of today? Also, it would be helpful if you could provide screenshots so that I can try to replicate how people interacting with the UI as of today. :)

Plus, could you also tell me more about how a specific enqueue in background is better working with all players comparing to my proposal with a enqueue switch/checkbox in the context of sharing URL from outside NewPipe UI? I thought I covered this use case as @Stypox pointed out. I keep scratching my head for a while but still have hard time envision what people have in mind. Cheers!

@MD77MD
Copy link

MD77MD commented Mar 21, 2021

@chouhanyang
ok, so this is the best i was able to do on my phone, hopefully its clear enough.

My suggestion is basically stypox 2.1 option with some useful tweaks. so when no video is playing, the share menu should look like usually
IMG_20210321_043534

but once a video is playing, the share menu would look like this (sorry for the weird editing 😋... )
IMG_20210321_043040

it would contain 4 options :

  1. video information: of the shared video, this will allow you to see the video info such description, comments... etc. without disturbing the player mode or cutting the video playing. also this would avoid losing your already queued videos.
  2. enqueue: would just enqueue the shared video no matter what mode the player is in (pop up, background or normal mode).
  3. Download
  4. add to a playlist

This is the most simplistic but attendee to all use cases. however you can use it to build upon it any further improvements

@chouhanyang
Copy link
Author

@MD77MD Thank you for your awesome mock-ups. Now I have better grasp of what you and @Stypox meant for hiding enqueue option if no player is running.

However, I'm still not getting answers from your design regarding my use case. Particularly, with "Background player" + "Always" chosen in your first menu, and then the second menu would be permanently dismissed. Therefore, your second menu would have no chance to be shown:

  • Freshly install NewPipe App.
  • Share first stream from YouTube App. Share menu pops up. (no "Enqueue" options now per your menu 1).
  • Select "Background player" and click "Always" on share menu.
  • Continue to share more streams from YouTube App.
  • No share menu or "Enqueue" (your menu 2) option will be seen because "Always" suppress the share menu entirely. Ooops!
  • Now I'm trapped.

Does this flow make sense based on your design? Hopefully I'm not making circles. Cheers!

@XiangRongLin
Copy link
Collaborator

@SameenAhnaf I have the repeat what 89z said, please don't just at 10+people. I can see that Stypox is already working on this PR. If additional opinions are needed he will mention us. Or you can selectively get the opinions of single people, not from this many people at once

@chouhanyang
Copy link
Author

Folks,

I spent last weekend to play around the code as @Stypox suggested. And I found a problem with current implementation. Mainly, PlayerHolder.getType() always return null when the main UI view is not activated and background player is running. Invoking NavigationHelper.playOnBackgroundPlayer() doesn't help as the background video will keep running in background without real "player". I believe it is just partial UI fragment running as a service. It took me several hours to figure this out, however finding a way to fix this is beyond my knowledge how the background services and UI interact with each other. More help is needed here.

At this point, sadly to say I was unable to implement the popular design 2.1 because we are unable to determine what is the "currently-running player". Furthermore, I believe it would incur several UI weirdness down the road, as I still don't get any direct answers from this thread to help me confident enough for 2.1 implementation regarding my use case.

So I took the liberty to implement the UI I suggested previously with extra switch in the settings.

You can find the release below if you like to play around:
https://github.com/SavantOne/NewPipe/releases/tag/PR5850

Share dialog:
device-2021-03-21-152437

Setting in Settings > Content
device-2021-03-21-151918

Basically, the switch will change it back to old NewPipe behavior as we don't have a reliable PlayerHolder.getType() working. Suddenly, the current minimal fix with "Enqueue background" doesn't seem so bad if you guys prefer a temporary fix before something better can be implemented. So I will leave the decision to the core team.

@Stypox Let me know which way you'd like to proceed. Do you want me to send PR implementing the switch or you want to continue with the temporary fix "Enqueue background"? You can see SavantOne@1a45843 for my comments regarding PlayerHolder.getType() not working. Thanks!

@chouhanyang
Copy link
Author

Folks, after this long thread, I feel a bit overwhelmed by sheer number the design proposals and it seems we are losing focus on the original goal we are trying to achieve, that is to provide a temporary enqueue from share URL. If this PR gets larger, there will be less chance to get it given its complexity. What helps I'm asking is:

  • Let's focus on the small goals. For larger design feature, break it down to small ones. Baby steps please.
  • On top of your design proposal, explain what problem you are solving with step by step use cases. This is crucial so that devs can follow your steps and feel the pain.
  • Stick on a design feature when you propose or challenge people's design with use cases. Laser focus on the small problem we are trying to solve.
  • Provide direct and concise answers regarding the questions to your design, which greatly avoid miscommunications of your proposal. And be patient when explaining to devs.
  • Keep it simple so we won't all lose steams!

My knowledge to this project is limited. So appreciate if you could guide me through step by step and treat me like a dumb. Plus, remember the goal for this PR is just a step stone toward a better future designs. Thx.

@SameenAhnaf I don't understand what's the "Always ask" menu you're referring to. For the code, NewPipe could be running as an service in background, so it is not clear to me what is the definition of NewPipe open/close. I'd be happy to discuss this with you offline. However I think it is already off-topic from this thread. Cheers!

@chouhanyang
Copy link
Author

@SameenAhnaf Thanks for the clarification.

Based on my humble experiences, the "always" button doesn't mean always ask. It means always use the select choice as default action. The design pattern is pretty common on android platform:

choices

We could argue the pros and cons for our UI design, however, it is impossible to change all other apps all together. That is why following the common pattern, IMO, is preferred which prevents confusing users. On the other hand, given choice to improve the wording, I'd replace "Always ask" in the settings dialog to "Ask by default" or "Reset to default", which better describe its true behavior.

Back to your design suggestions, I would recommend telling us what is the use case and scenarios based on your personal experiences. We could imagining endless ways to design the UI, however, it is most valuable to know what difficulties and experiences people have based on their own user experience.

Take me for example, I open YouTube app first. Find video streams, and keep enqueuing them. I'd repeat this 20+ times everyday, so always "Enqueue background" is crucial to me because I hate to see the share dialog every time because I have only one choice "Enqueue background". On the other hand, for the basic users, clarity should be prioritized to them when they share an URL. Especially, a visible response is important as an confirmation. IMO, there are different focuses for basic users and advanced users:

  • Basic Users:

    • Focus on UI clarity. No secret background operations without visible responses.
    • Limit options for most common usage with a hint to advanced options.
    • Okay to spend few clicks to make sure basic users understand what they choose, and avoid tricky concepts like queue.
  • Advanced Users:

    • Focus on efficiency. No unnecessary UI pops as they already know what they are doing.
    • Provide comprehensive list of settings for advanced users to tweak.
    • Provide shortcuts, saving clicks, and anything to help them get to the end result quickly.

Given this guideline, always asking advanced users for enqueue is less ideal. I would have to repeat this 20+ clicks everyday if you could imagine. On the other hand, I do think current UI to open players directly is good design for basic users as they expect immediate actions rather than UI efficiency.

There is also a way to bridge these two groups users through "gamification". Take my favorite game FF7 remake for example, after people play the game and gather magic stones, and they will be able to see more options along with growing EXP level. You can even set shortcuts to quickly perform magic quickly for example:

gamification

In short, I would rather see a polling on different use cases how people use NewPipe in terms of sharing URLs, as opposed of UI design patterns. These use cases enable us to establish a way evaluating a pieces of UI, e.g. how many use cases does this design cover? To some extends, voting on the final design may not be super helpful as there are other often obstacles while implementing down the road. Like in this case, there's no way to distinguish player type without main UI view open. Oopos!

So, I'd rather hear your story. How you use it everyday step by step, and what are the UI pain points you encounter in your day to day live. Hopefully this makes sense. Cheers!

@triallax

This comment has been minimized.

@ThisIsPaulDaily ThisIsPaulDaily mentioned this pull request May 24, 2021
@rancidfrog
Copy link

rancidfrog commented May 29, 2021

@jmbreuer always ask is useless at the moment, because there is no enqueue options, and the workaround of "show info" has been removed in latest release.(show info only appears if there is more than 1 video in queue)
So queuing has been removed completely until this is merged.

@rancidfrog
Copy link

@ThisIsPaulDaily
Same.
Not sure why it was removed in the first place, nor why readding enqueue option is taking so long.

Please add an option to queue.
Personally I use Share a lot from browser and would like to queue to background.
I also have always ask setting enabled.
But, really any option is good. Whether showing enqueue option or queuing automatically when something is already playing.
Both are great if they queue properly

@rancidfrog
Copy link

#5850 (comment)
This seem to complicated, and will allow bugs due to incomplete usecases.

I believe @Stypox suggestions are best.

  • 🚀 Automatic enqueue when selecting "Play on XX" and there is a player already open
  • Adding an "Enqueue" action in the share menu

Also, thus option should override both/either above

  • 👍 Add a switch in settings named "Always enqueue shared stream if a player is already open"
    I.E.
    if(!switchQueue){
    //use above logic to enqueue
    }
    Else //enqueue automatically since switch setting is enabled/true

@ThisIsPaulDaily

This comment has been minimized.

@rancidfrog
Copy link

Is Queue being added to any milestones; i.e. 20.7?
Is there anything specific holding the readdition of queuing logic to app?
At least having a queue in share dialog until you have time to take a closer look into proper implementation, will be useful to users

@thinsoldier

This comment has been minimized.

@triallax

This comment has been minimized.

Copy link

@ThisIsPaulDaily ThisIsPaulDaily left a comment

Choose a reason for hiding this comment

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

This is my informal review. I see no issues in the changes provided here. While some might feel phrasing of text can change, I feel that this ticket has been open since March and has potential benefits to users and should be merged.

@litetex
Copy link
Member

litetex commented Oct 13, 2021

@chouhanyang

Could you rebase the PR. There are merge conflicts.
Tried to fix them myself but I have no permissions.

@litetex litetex added the waiting for author If the author doesn't respond, the issue will be auto-closed. Otherwise the label will be removed. label Oct 13, 2021
final boolean prioritizeEnqueue = preferences.getBoolean(
getString(R.string.prioritize_enqueue),
false);
// BUG: getType() always return null if main view is not open.
Copy link
Member

Choose a reason for hiding this comment

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

Please check after rebasing if this is still a thing 😄

Copy link
Author

Choose a reason for hiding this comment

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

Just rebased the code and updated the repo. Please let me know if there's anything I can do on my side. Thanks!

@litetex

This comment has been minimized.

@chouhanyang
Copy link
Author

@chouhanyang Build does not compile.

@litetex Oops, sorry about that, I didn't realized that the NavigationHelper interface has been changed. Now I have updated the code, and successfully build apk with it. Also did some basic testing to make sure it still works. Also now passed CI checks.

Let me know if there more testing I can do here. Thanks!

@github-actions github-actions bot removed the waiting for author If the author doesn't respond, the issue will be auto-closed. Otherwise the label will be removed. label Oct 15, 2021
@litetex
Copy link
Member

litetex commented Oct 16, 2021

I tried to add multiple improvements to the PR by myself but it's not possible for a maintainer to modify the PR and I otherwise had to request so many tiny changes which would be very resource-intensive.

Because of these reasons I opened a new PR: #7266

Again thank you for the PR!

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.