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 "add to playlist" option in the share menu #7194

Merged
merged 8 commits into from
Oct 13, 2021

Conversation

KalleStruik
Copy link

@KalleStruik KalleStruik commented Oct 2, 2021

[!] Before this can be merged, there is a small theming issue that needs to be fixed. When creating a new playlist from the share dialog it uses the wrong color for the buttons (See screenshot 4). I have been unable to find the cause of this bug, so any help in fixing it is appreciated. Fixed

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 an "Add to playlist" option in the incoming share dialog.

Before/After Screenshots/Screen Record

  • Before:
    image
    image

  • After:
    image
    When no playlists exist:
    image
    When a playlist exists:
    image
    image

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

@triallax triallax added feature request Issue is related to a feature in the app GUI Issue is related to the graphical user interface labels Oct 2, 2021
@KalleStruik KalleStruik marked this pull request as ready for review October 3, 2021 12:26
@KalleStruik
Copy link
Author

KalleStruik commented Oct 3, 2021

The text of the dialog buttons is now visible
image

@litetex
Copy link
Member

litetex commented Oct 3, 2021

In my opinion "always ask" should be at the end and not in between.

@KalleStruik
Copy link
Author

Completely agree
image

@litetex litetex self-requested a review October 9, 2021 16:21
@litetex
Copy link
Member

litetex commented Oct 9, 2021

@KalleStruik
Seems to work very good as far as I can see.

However I applied some fixes regarding the underlying code.
There are many issues which somehow got into the code and now affect this PR.

What I did:

  • Moved dismiss listener into abstract superclass
  • Removed converter methods as they are barely used
  • Removed "copy everything from Add- to CreatePlaylistDialog"-method
  • Fixed naming
  • Create one method createCorrespondingDialog in PlayListDialog that covers all use cases and doesn't requires so much code when implementing.
  • Added some documentation

I also added a little hint/toast-notification that adding to the playlist may take a moment:
grafik
Already fixed typo: May take a moment

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.

I found one little problem:

The cursor seems to have the wrong color in the "New Playlist"/CreatePlaylistDialog.

Looks like the "colorAccent" is missing.

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.

Everything works as expected 👍

LGTM

@ThisIsPaulDaily

This comment has been minimized.

@litetex

This comment has been minimized.

@ThisIsPaulDaily
Copy link

While I can appreciate that my quick typing asking to pull #5850 ahead of this might seem off-topic and out of left field. They are very similar in code that is being touched and purpose for why this feature is being added.
This issue #7194 is solving is directly a result of working around not having #5850.
What is happening here is the person will start playing a playlist in popup and then add new videos to the playlist to allow for continuous streams.

#5850 is enqueue in the background restored which was a feature up until like version 19.

@litetex

This comment has been minimized.

@ThisIsPaulDaily
Copy link

You highlighted four things I said and I can explain.
This PR and 5850 touch similar sections of code and will conflict.

Binge watching similar videos is a breeze is what the old enqueue functionality allowed.

The last part was a gripe to fix what was broken before trying to add new features. 5850 is the consolidation of several tickets which all had the same root cause of removing the queue.

Is there something inherently wrong with #5850 that prevents it from being reviewed and pushes this PR ahead?

@litetex
Copy link
Member

litetex commented Oct 13, 2021

@ThisIsPaulDaily

This PR and 5850 touch similar sections of code and will conflict.

#5850 already has conflicts:
grafik

And yeah that happens. We rebase the issue, fix the conflicts and everything will be fine.

Binge watching similar videos is a breeze is what the old enqueue functionality allowed.

???

The last part was a gripe to fix what was broken before trying to add new features. 5850 is the consolidation of several tickets which all had the same root cause of removing the queue.

#5850 handles issue #4779 which is an enhancement/new feature. If it was so in an old version then it seems to have been removed on purpose or was a not recognized...

Update: That's even written in #4779 "Until newPipe 0.20.2, while playing a video in background or pop-up mode, sharing another video from others apps to newPipe would enqueue them directly. However this is changing in the upcoming release because it was determined to be bad UX #4562..."

Is there something inherently wrong with #5850 that prevents it from being reviewed and pushes this PR ahead?

It's not reviewed and I don't want to rebase and review an already okay and up-to-date PR (this one).

I'm merging this now as I can see no logical arguments that are speaking against that.

@litetex litetex merged commit 4af49ee into TeamNewPipe:dev Oct 13, 2021
This was referenced Nov 30, 2021
@litetex litetex mentioned this pull request May 1, 2022
5 tasks
@goyalyashpal goyalyashpal mentioned this pull request Sep 11, 2022
6 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 GUI Issue is related to the graphical user interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add "Add to playlist" option in "Always ask" share menu and "Preferred 'Open' Action"
5 participants