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

Share/Open with newpipe" add enqueue choice for players #7266

Closed

Conversation

litetex
Copy link
Member

@litetex litetex commented Oct 16, 2021

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

Successor to #5850 (please take a look at that PR for the basic description).

This is a new PR because it's not possible for a maintainer to modify the old one and I otherwise had to request so many tiny changes which would not be easily doable.

TL;DR - changes on top:

  • Fixed/Reformatted code
  • Refactored UI in the background (nearly looks the same)
  • Some minor fixes (theme not working etc)

Before/After Screenshots/Screen Record

grafik grafik

NPEnqueOptionDemo.mp4

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

chouhanyang and others added 4 commits October 17, 2021 00:03
* Better code structure (methods)
* Reformatted code
* Improved naming
* Refactored UI
  * Removed useless layouts
  * Increased clickable advanced options area
  * Use a layout for possible future advanced options
  * Reworked margins so that there are no gigantic distances between the components
  * Instead of adding e.g. the drop-down icon programmatically manage it the same as in video-detail
  * Animated the drop-down icon
* Removed unused icon
* Fixed light theme not working with the icon
* Added tooltip so users better know what the "Prioritize stream queuing" means (sadly only works for Android 8+)
@litetex litetex changed the title Open with newpipe enqueue choice Share/Open with newpipe" add enqueue choice for players Oct 16, 2021
@litetex litetex marked this pull request as ready for review October 16, 2021 22:36
@litetex litetex added feature request Issue is related to a feature in the app GUI Issue is related to the graphical user interface labels Oct 17, 2021
@opusforlife2
Copy link
Collaborator

Should it be a separate toggle?

I think it should be the default behaviour. If something is already playing, Newpipe should always enqueue a video link shared to it instead of playing it directly and potentially causing information loss (of an existing video or queue).

@litetex
Copy link
Member Author

litetex commented Oct 23, 2021

@opusforlife2
I oppose this.
My use-case: You are listening to a music-video in the background. Then you want to watch a video a friend has send you via messenger XY.
I think it's better to then play the actual video instead of enqueuing it.

@opusforlife2
Copy link
Collaborator

The user might have arrived at their currently open stream by clicking on links after links, and may not remember the name of the video or the channel.

Then there should be a way to avoid the unintentional or accidental data loss (the currently playing video that would be removed). The currently playing video should be shifted to the 2nd position in the queue, or something similar.

Or, the newly opened video should open as a new page on the backstack, so that when the user finishes watching it, they can then just tap Back and resume the earlier video.

Actually, this 2nd suggestion could remove a whole class of problems. Just add externally opened links to the backstack too. You will never lose your place in the app.

@litetex
Copy link
Member Author

litetex commented Oct 23, 2021

The user might have arrived at their currently open stream by clicking on links after links, and may not remember the name of the video or the channel.

Uhm wouldn't that be visible in the "History"?
I would also stick with the default behavior of the YT app and many other "single page applications" in that case (which is override currently playing video) because I think that's what people expect the most.

Then there should be a way to avoid the unintentional or accidental data loss (the currently playing video that would be removed). The currently playing video should be shifted to the 2nd position in the queue, or something similar.

Or, the newly opened video should open as a new page on the backstack, so that when the user finishes watching it, they can then just tap Back and resume the earlier video.

Actually, this 2nd suggestion could remove a whole class of problems. Just add externally opened links to the backstack too. You will never lose your place in the app.

I think introducing this here would blow up the scope of the PR.
Maybe we can add this behavior in the future 😄

Also note that there have been a lot of discussions in #5850.
I just recreated/took over the PR due to the above mentioned problems.

@opusforlife2
Copy link
Collaborator

Uhm wouldn't that be visible in the "History"?

Not everyone has history enabled.

I think introducing this here would blow up the scope of the PR.
Maybe we can add this behavior in the future 😄

Sure, no problem.

@litetex
Copy link
Member Author

litetex commented Oct 23, 2021

Maybe we need some kind of behavior like suggested for the back button in another PR.

Anyway I think there are so many options which can happen when someone wants to open a already playing video, so I think we have to do a major rework sooner or later:

e.g.:

"Open action" setting

  • Show info
  • Play on a player
    • Type of player
      • video
      • background
      • popup
    • Use already active player rather than changing to above selected type
      • yes
      • no
    • What should happen with the already playing video?
      • override/throw away (currently the case - default)
      • finish playing it and enqueue the inbound video (get's introduced by this PR)
      • finish playing it and enqueue the inbound video at first position
      • play inbound video and resume playing (the currently playing video) afterwards
  • Download
  • Always ask

Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

Do we really need to hide the enqueue option under an "Advanced options" menu? The space needed for the single switch is the same as that needed for the advanced options menu, and the amount of data on the screen is also the same, so I wouldn't do it like this. I'd just have the switch there and call it a day: people who don't know what it is or who don't use it can ignore it without any problem, but people who want to use it save one click.

By the way, I still don't get why we can't just add an "Enqueue" entry when a player is active and use the currently active player. This way the user does not have to remember which player he is currently on each time he wants to enqueue. In the settings this could be modeled in the menu as "Enqueue / play on main player", "Enqueue / play on audio player", "Enqueue / play on popup player".

app/src/main/res/xml/video_audio_settings.xml Show resolved Hide resolved
app/src/main/res/layout/dialog_share_context.xml Outdated Show resolved Hide resolved
@@ -0,0 +1,78 @@
<?xml version="1.0" encoding="utf-8"?>
<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to use a ConstraintLayout to prevent view nesting?

app/src/main/java/org/schabi/newpipe/RouterActivity.java Outdated Show resolved Hide resolved
litetex and others added 2 commits November 22, 2021 20:21
Co-authored-by: Stypox <stypox@pm.me>
Co-authored-by: Stypox <stypox@pm.me>
@litetex litetex marked this pull request as draft November 22, 2021 19:34
@litetex
Copy link
Member Author

litetex commented Nov 22, 2021

#7266 (review)
I agree with most of the statements.

This issue has to go back to the drawing board...
Set it's state to draft.

@SameenAhnaf

This comment has been minimized.

@offshade
Copy link

offshade commented Dec 15, 2021

Is there a version with this option / enqueue available? This is great and much needed.

@opusforlife2
Copy link
Collaborator

Is there a version with this option / enqueue available? This is great and much needed.

@offshade While logged in, go to the Checks tab of this PR, then click on Artifacts. You can download the zipped APK from it. It will install a debug version of Newpipe which won't interfere with the main version. You can delete it after the main version gets this feature and resume using that.

@offshade
Copy link

Is there a version with this option / enqueue available? This is great and much needed.

@offshade While logged in, go to the Checks tab of this PR, then click on Artifacts. You can download the zipped APK from it. It will install a debug version of Newpipe which won't interfere with the main version. You can delete it after the main version gets this feature and resume using that.

This is so awesome! Thank you very very much.
Is there a thread for this version in case it has updates etc?

@offshade
Copy link

offshade commented Feb 7, 2022

Version doesn't work anymore.

@opusforlife2
Copy link
Collaborator

@offshade Wait for the PR to be rebased.

@offshade
Copy link

offshade commented Feb 12, 2022

@offshade Wait for the PR to be rebased.

I will wait for anything, although being clueless about technical stuff, I have no idea what you just said... 😂 Hope any update will be posted here.

@rancidfrog

This comment was marked as duplicate.

@litetex
Copy link
Member Author

litetex commented Feb 20, 2022

Will this be merged?

In the current state not.

This issue has been raised for a while now.
Is it so hard to just have an extra enqueue option in dialog?

Read the above comments.

I believe in previous iteration it used to be available,

No.

@ThisIsPaulDaily
Copy link

The enqueue option could be the non-default dialog option if you are concerned about background versus foreground player antics. This would force the user to make an informed decision to set always open this way. This has been broken since version 18 or something when background player lost the ability to enqueue separately.

Also, addressing the history comment as a work around, it isn't a work around because I'll often enqueue multiple videos and they won't be marked in my history unless they play.

@MD77MD

This comment was marked as abuse.

@ghost
Copy link

ghost commented May 3, 2022

I'm confused with the status of this PR. Isn't this already worked upon?

@Stypox
Copy link
Member

Stypox commented May 3, 2022

Yes, but there is currently no agreement on how the UI should look and behave

@offshade
Copy link

Any updates in this issue? This version doesn't work anymore. Anything with enqueue option available?

@MD77MD
Copy link

MD77MD commented Feb 9, 2023

any chance for this to be reviewed.?

@opusforlife2
Copy link
Collaborator

This PR is quite outdated in its current state. If someone wants to resume this, it can be reopened.

@offshade

This comment was marked as spam.

@opusforlife2

This comment was marked as resolved.

@offshade

This comment was marked as resolved.

@opusforlife2

This comment was marked as resolved.

@ThisIsPaulDaily
Copy link

I would really appreciate this getting fixed and it broke the way I used NewPipe.

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.

Add Enqueue button in "Always Ask" Share Menu Queuing background videos outside of app
9 participants