-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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 playlist name and video name in playlist sharing content #10427
Conversation
Thank you for the changes. Please add screenshots of the share sheets showing the state before and after your PR. |
043847e
to
ef68b77
Compare
@TobiGr Thanks for your inputs, updated the PR with screenshots. |
Thanks! |
@Stypox just commented on Matrix that a dialog before sharing is better than a setting. He is correct. So please do not add a setting but rather an AlertDialog with a checkmark which enables the user to add the video title and playlist name |
158ef8c
to
b5274a7
Compare
@TobiGr Added a confirmation dialog to the playlist sharing process, allowing users to choose between sharing playlist formats. Users can also choose to save their sharing format preference to skip the confirmation dialog. In addition, an option has been added to the settings to update the sharing preference. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you
app/src/main/java/org/schabi/newpipe/local/playlist/LocalPlaylistFragment.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/schabi/newpipe/local/playlist/LocalPlaylistFragment.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/schabi/newpipe/local/playlist/LocalPlaylistFragment.java
Outdated
Show resolved
Hide resolved
Thanks @TobiGr for your time and review. Updated the changes as requested. Let us wait for @opusforlife2 for inputs on the settings item. |
There's no good place to put it, unfortunately. But why do we need a setting for this at all? Just have the checkbox state remembered. That one extra dialogue doesn't make much of a difference. |
Hi @opusforlife2, Thanks for your feedback. I understand your concern about the extra dialog, but I believe it's important to give users the option to choose how they want to share their playlists. Some users may prefer the convenience of having a checkbox that remembers their preference, while others may want to be more deliberate about how they share their playlists. That said, I'm open to other suggestions. Here are a few options:
I'm happy to discuss these options further with you, or to implement any other solution that you think would be better. Regarding #9587, I agree that it would be great if someone could tackle it. I would be happy to look into it. Thanks |
I can't think of any obvious use cases where an average user would need to rapidly go through playlists and share them. At best this would be a one-off task. Apart from that, there could be an occasional desire to share an interesting playlist with a friend. This is why I suggested to go with:
Can you think of any important use cases that are frequent enough to justify an additional setting? |
64f2416
to
fc4ad42
Compare
Playlist_Sharing_Demo.mp4
Hi @opusforlife2, I think you make a good point. I can't think of any use cases where the average user would need to rapidly share playlists. I have updated the flow to always show the dialog and updated the video demo. WDYT |
Awesome! The flow looks great. One last thing. Instead of saying "Share with details", I think the button should say "Share with titles". That's a little more descriptive of its function. |
@opusforlife2 Thanks, updated. |
- Currently, only a list of videos separated by newline are added in the share content. - This makes it difficult to identify a specific video in a list of Urls. - Added Playlist name as the header and corresponding video name for each video url in following format. ``` My Playlist - Music1: https://media-url1 - Music2: https://media-url2 - Music3: https://media-url3 ``` Screenshot: | Before | After | | --- | --- | | <img src="https://github.com/TeamNewPipe/NewPipe/assets/87667048/fa7b27b2-91d6-491d-8f93-a8b447263de6" width=320> | <img src="https://github.com/TeamNewPipe/NewPipe/assets/87667048/e7ddde18-1d3a-4ccb-8c35-f5bf14e48bec" width=320> |
- Added a confirmation dialog for users to choose between sharing playlist formats. - Users can choose to save their sharing format preference to skip the confirmation dialog. - Added an option in the settings to update the sharing preference.
- Used string resources for the sharing content formats. - Updated shared preference key to `remember_local_playlist_sharing_option_key` - Updated dialog string values.
- Removed the setting item to update the playlist sharing preference. - Removed the option to save user's playlist sharing preference. Video demo: - https://github.com/TeamNewPipe/NewPipe/assets/87667048/5ac5ab6f-163d-4407-bd67-a078a8b55bbb PR Commit Message Add playlist name and video name in playlist sharing content - Currently, only a list of videos separated by newline are added in the share content. - This makes it difficult to identify a specific video in a list of Urls. - Used string resources for the sharing content formats. - Added a confirmation dialog for users to choose between sharing playlist formats. - Added Playlist name as the header and corresponding video name for each video url in following format. Playlist - Music1: https://media-url1 - Music2: https://media-url2 - Music3: https://media-url3 Screenshots: | Before | After | | --- | --- | | <img src="https://github.com/TeamNewPipe/NewPipe/assets/87667048/fa7b27b2-91d6-491d-8f93-a8b447263de6" width=320> | <img src="https://github.com/TeamNewPipe/NewPipe/assets/87667048/e7ddde18-1d3a-4ccb-8c35-f5bf14e48bec" width=320> | Video demo: - https://github.com/TeamNewPipe/NewPipe/assets/87667048/5ac5ab6f-163d-4407-bd67-a078a8b55bbb
- Updated string value for the dialog option. Video demo: - https://github.com/TeamNewPipe/NewPipe/assets/87667048/09962034-146a-4fa6-acbc-5bc58cba2d10 PR Commit Message Add playlist name and video name in playlist sharing content - Currently, only a list of videos separated by newline are added in the share content. - This makes it difficult to identify a specific video in a list of Urls. - Used string resources for the sharing content formats. - Added a confirmation dialog for users to choose between sharing playlist formats. - Added Playlist name as the header and corresponding video name for each video url in following format. Playlist - Music1: https://media-url1 - Music2: https://media-url2 - Music3: https://media-url3 Screenshots: | Before | After | Confirmation Dialog | | --- | --- | --- | | <img src="https://github.com/TeamNewPipe/NewPipe/assets/87667048/fa7b27b2-91d6-491d-8f93-a8b447263de6" width=270 height=580> | <img src="https://github.com/TeamNewPipe/NewPipe/assets/87667048/e7ddde18-1d3a-4ccb-8c35-f5bf14e48bec" width=270 height=580> | <img src="https://github.com/TeamNewPipe/NewPipe/assets/87667048/c073c41e-8b49-461b-95b2-cab5e7ffb692" width=270 height=580> | Video demo: - https://github.com/TeamNewPipe/NewPipe/assets/87667048/09962034-146a-4fa6-acbc-5bc58cba2d10
3472e49
to
2e198e6
Compare
Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I fixed the wrongly formatted string resources in 0758cd6 |
…Pipe#10427) - Currently, only a list of videos separated by newline are added in the share content. - This makes it difficult to identify a specific video in a list of Urls. - Used string resources for the sharing content formats. - Added a confirmation dialog for users to choose between sharing playlist formats. - Added Playlist name as the header and corresponding video name for each video url in following format. Playlist - Music1: https://media-url1 - Music2: https://media-url2 - Music3: https://media-url3 Co-authored-by: TobiGr <[email protected]>
What is it?
Description of the changes in your PR
Add playlist name and video name in playlist sharing content
the share content.
Urls.
playlist formats.
each video Url in following format.
Before/After Screenshots/Screen Record
Video demo:
Playlist_Sharing_Update_Demo.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. You can find more info and a video demonstration on this wiki page.
Due diligence
Note: I am aware of the NewPipe rewrite, and I would be open to updating my PR as per requirements.