Skip to content

Add ability to share stream URL externally#3449

Closed
jadkik wants to merge 8 commits intoTeamNewPipe:devfrom
jadkik:share-stream-url
Closed

Add ability to share stream URL externally#3449
jadkik wants to merge 8 commits intoTeamNewPipe:devfrom
jadkik:share-stream-url

Conversation

@jadkik
Copy link
Copy Markdown

@jadkik jadkik commented Apr 17, 2020

What is it?

  • Bug fix (user facing)
  • Feature (user facing)
  • Code base improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

  • add additional menu item in video detail page to share the URL that would be played in external player

Fixes the following issue(s)

Testing apk

debug.zip

debug.zip (updated 2020-05-10)

Agreement

Copy link
Copy Markdown
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.

Shouldn't you share the url for the currently selecred stream?

@jadkik
Copy link
Copy Markdown
Author

jadkik commented Apr 19, 2020

You are right, I'm not very familiar with the code base yet.

I also moved and renamed the menu item and placed it below "Open in browser".

How do you handle translation? Should I add a placeholder entry for the menu item in all strings.xml files or is it managed separately?

@wb9688
Copy link
Copy Markdown
Member

wb9688 commented Apr 19, 2020

How do you handle translation? Should I add a placeholder entry for the menu item in all strings.xml files or is it managed separately?

You should only touch the values one, like you currently do. The rest will be handled by Weblate.

@jadkik jadkik marked this pull request as ready for review April 19, 2020 20:28
@jadkik jadkik marked this pull request as draft April 19, 2020 20:30
@jadkik
Copy link
Copy Markdown
Author

jadkik commented Apr 19, 2020

Sorry, marked this as ready for review, but I just noticed it still breaks with SoundCloud items because getSelectedVideoStream() fails.

Will look into it later to try and find a proper fix.

@jadkik jadkik marked this pull request as ready for review May 10, 2020 01:26
@jadkik jadkik requested a review from Stypox May 10, 2020 01:29
@jadkik
Copy link
Copy Markdown
Author

jadkik commented May 10, 2020

Updated to work with audio and video streams. Tested it with some Soundcloud links, YouTube links and MediaCCC link.

Copy link
Copy Markdown
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.

Sorry for the delay, I forgot this PR needed to be reviewed.
Code looks good, and most importantly, it works :-D
Once the two small issues I pointed out are fixed, this is ready to go

Comment thread app/src/main/java/org/schabi/newpipe/fragments/detail/VideoDetailFragment.java Outdated
Comment thread app/src/main/res/values/strings.xml Outdated
@jadkik jadkik force-pushed the share-stream-url branch from fefd2f7 to e4a8dd6 Compare June 2, 2020 19:54
Stypox
Stypox previously approved these changes Jun 3, 2020
Copy link
Copy Markdown
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.

I tested again, on Android 4.4, on Android 7.0 and on Android 10. Everything seems to work fine. Thank you :-D
After the discussion about link/url is settled and if @TobiGr or @wb9688 agree, I'd merge this.

B0pol
B0pol previously approved these changes Jun 3, 2020
Copy link
Copy Markdown
Member

@wb9688 wb9688 left a comment

Choose a reason for hiding this comment

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

Just these really small things

Comment thread app/src/main/java/org/schabi/newpipe/fragments/detail/VideoDetailFragment.java Outdated
Comment thread app/src/main/java/org/schabi/newpipe/fragments/detail/VideoDetailFragment.java Outdated
Comment thread app/src/main/java/org/schabi/newpipe/fragments/detail/VideoDetailFragment.java Outdated
@wb9688
Copy link
Copy Markdown
Member

wb9688 commented Jun 3, 2020

@Stypox: The reason I said URL is that I think we use URL instead of link throughout the app

@jadkik jadkik dismissed stale reviews from B0pol and Stypox via f838697 June 3, 2020 21:08
@opusforlife2
Copy link
Copy Markdown
Collaborator

Well, there is also the chance that people confuse stream to mean live stream, because that's the usual connotation.

I suggest "Share media URL" to be unambiguous about what the option does.

I'd prefer "Share video/audio URL" based on whether it's SoundCloud or one of the others, though. It's extra code, but worth it, if it can be implemented.

Copy link
Copy Markdown
Member

@wb9688 wb9688 left a comment

Choose a reason for hiding this comment

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

Your lines are too long now…

@jadkik
Copy link
Copy Markdown
Author

jadkik commented Jun 3, 2020

I'd prefer "Share video/audio URL" based on whether it's SoundCloud or one of the others, though. It's extra code, but worth it, if it can be implemented.

I assume it wouldn't be hard to have both Share video URL/Share audio URL and hide one (or show both where applicable). It looks like it would be a few simple changes in setupActionBar or updateMenuItemVisibility?

@B0pol
Copy link
Copy Markdown
Member

B0pol commented Jun 3, 2020

How can people know the resolution is audio free? With the external player setting there is that mute button, but no such thing here

@Stypox
Copy link
Copy Markdown
Member

Stypox commented Jun 4, 2020

How can people know the resolution is audio free?

The selector shows whether the resolution has audio or not

@B0pol
Copy link
Copy Markdown
Member

B0pol commented Jun 4, 2020

Only if you've set external player in settings, yes that's what I was saying. Otherwise people won't know

@Stypox
Copy link
Copy Markdown
Member

Stypox commented Jun 10, 2020

Oh right, I see. I don't think there is much that can be done, a completely new ui would be needed because always showing the muted state would probably be confusing. I'd leave it as it is currently for now, in order not to complicate things too much.

@Stypox Stypox mentioned this pull request Jul 11, 2020
@TobiGr TobiGr force-pushed the dev branch 2 times, most recently from 679bc75 to 2aeccc0 Compare March 16, 2021 08:24
@litetex litetex marked this pull request as draft October 1, 2021 17:07
@litetex
Copy link
Copy Markdown
Member

litetex commented Oct 1, 2021

No progress since a year, no GitHub actions build and merge conflicts.

Closing this for now. Feel free to reopen it when there is progress again.

@litetex litetex closed this Oct 1, 2021
@jadkik
Copy link
Copy Markdown
Author

jadkik commented Oct 1, 2021

@litetex I'm willing to resolve the conflicts and get this merged but I need feedback from the reviewers about how to proceed.

@dandare1980
Copy link
Copy Markdown

@litetex I'm willing to resolve the conflicts and get this merged but I need feedback from the reviewers about how to proceed.

@jadkik would you consider reopening this pull request? There's still interest in seeing this feature in NewPipe.

@ShareASmile
Copy link
Copy Markdown
Collaborator

ShareASmile commented Aug 29, 2023

@dandare1980 Let members of the TeamNewPipe decide about it as rewrite of the app is on the cards.

@jadkik would you consider reopening this

if jadkik agrees to work on it again then
technically only a team member/ maintainer having rights to do that can reopen this PR because it was closed by a member/maintainer.

@opusforlife2
Copy link
Copy Markdown
Collaborator

@jadkik I apologise for your PR getting ignored. It's been 2 years, but are you still interested in this feature?

If so, maybe it would be better to wait for the rewrite, because it will be sure to contain massive changes to the player code. Maybe that makes it easier or harder to obtain and show the direct stream URL, I don't know.

@ShareASmile Thanks for bringing this to our attention!

@radasbona

This comment was marked as off-topic.

@jadkik
Copy link
Copy Markdown
Author

jadkik commented Oct 14, 2023

@jadkik would you consider reopening this pull request?

@jadkik I apologise for your PR getting ignored. It's been 2 years, but are you still interested in this feature?

I haven't used my Kodi/RPi setup in a few months, so I don't immediately need this myself anymore.

If so, maybe it would be better to wait for the rewrite, because it will be sure to contain massive changes to the player code.

I haven't kept up with the developments here - so I wouldn't know. I'll try to rebase this and see how messy it gets.

@jadkik How would this PR handle the separate A/V streams btw ? Caz external players can only grab a single stream source.

Good point, I do not know. I think I had made it pick the first option or some kind of preferred option based on existing code. I was building on top of what the "play in external player" did, so if that didn't work with some videos it would also fail when sharing that stream URL.

@ShareASmile ShareASmile added the feature request Issue is related to a feature in the app label Jul 23, 2025
@boernsen-development

This comment was marked as duplicate.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants