TeamNewPipe / NewPipe

A libre lightweight streaming front-end for Android.
https://newpipe.net
GNU General Public License v3.0
31.59k stars 3.07k forks source link

"Clean" URL when sharing (remove all parameters that are not video ID) #3247

Open woj-tek opened 4 years ago

woj-tek commented 4 years ago

It would be great, if NewPipe would only use video ID when sharing the link (v=…) instead of the whole URL that was used to open the video (often including source from which we got the video).

This is somewhat related to #1556

mauriciocolli commented 4 years ago

For some reason the originalUrl (the one that was used to get the info, possibly "dirty") was being used when sharing an item, it was introduced in this commit 1ae54f6f8c62a487bfd72c03c6edf431b4a221cc.

I tested it according to the commit description, and indeed, the url of Peertube videos are pointing to an api url and the original url (used to open it) is what works in this condition. I guess the committer forgot about other services?

B0pol commented 4 years ago

You should not remove timestamps from videos, eg watch?v=VIDEOID&t=60, this &t=60 should stay when sharing a video.

mauriciocolli commented 4 years ago

You should not remove timestamps from videos, eg watch?v=VIDEOID&t=60, this &t=60 should stay when sharing a video.

Interestingly, that's a side effect of this issue.

Though, a proper fix or even a dedicated shareUrl field would be much better.

woj-tek commented 4 years ago

@B0pol I'm not sure that timestamp should be included. IMHO it should be handled by #1556:

EDIT: correct ticket

gkeegan commented 4 years ago

@woj-tek Time stamp should definitely be included if it was in the original link. This is best for showing one part of a very long video.

woj-tek commented 4 years ago

I still this still should be optional - if you want to share particular timestamp you still would be able to by using #2000

EDIT: correct issue from #1556 to #2000

gkeegan commented 4 years ago

What? That doesn't mention timestamps at all.

B0pol commented 4 years ago

No, this is a feature. Try by yourself : go at 60 seconds, share, you'll have &t=60. Now go at 2 minutes, you'll see &t=120. It's intended, and should definitely not be changed until we have a proper share menu (where you can chose start time, use shortened or invidious…).

woj-tek commented 4 years ago

My bad - I mixed two issues. You already can share video with timestamp included (see #2000) so why original timestamp should be transferred to the person you want to share it to?

In my case, most of the time (always) I'd opt out of including timestamp in shared video.

opusforlife2 commented 4 years ago

I agree with @woj-tek that the primary share button should share a clean video link. We already have a time-stamped share button when playing the video.

More than that, it is just unexpected that the app would treat the share button as a clipboard of sorts, instead of generating a link on its own. I think most users would be surprised by this behavior.

opusforlife2 commented 4 years ago

Need help, guys. I'm trying to implement this, but I'm stuck. I can't find a way to check for the service being Youtube. If I could get that, I'd make an if else to share the original URL for other services and a clean URL for Youtube.

mauriciocolli commented 4 years ago

@opusforlife2 No need to check for services or anything, it should be done regardless, just return the clean url instead of what it's done currently (change getOriginalUrl to getUrl).

Now, if the clean url should include anything other than the essential is another story, it's probably better to introduce another field specifically for these use cases. Maybe open an issue in the extractor repository?

Notice that the issue with Peertube should be fixed as well if you do that.

opusforlife2 commented 4 years ago

Oh. I thought you said above that getOriginalUrl needed to stay because of Peertube.

Edit: You're sure making this change won't break things?

SydneyDrone commented 2 years ago

Is this issue still open for contribute or already solved by #3262?

opusforlife2 commented 2 years ago

@SydneyDrone It's open. Do you want to be assigned?

SydneyDrone commented 2 years ago

@opusforlife2 Yes, I'd like to, but I'm a little bit confused because it seems that your PR has already fixed this issue. Did I miss something?

opusforlife2 commented 2 years ago

@SydneyDrone I don't actually remember why it was closed. Ignore it and go ahead with a new PR, no worries.

SydneyDrone commented 2 years ago

@opusforlife2 OK, I'm working on it.

SydneyDrone commented 2 years ago
Screenshot_20220411_200209

As shown above, there are two share button. It seems that the upper one is for sharing URL with timestamp(which is implemented in #2271), and the lower one is for sharing clean URL(which is implemented in #3262).

Perhaps the only remaining work for me is writing tests?

opusforlife2 commented 2 years ago

@SydneyDrone See the problem statement:

It would be great, if NewPipe would only use video ID when sharing the link (v=…) instead of the whole URL that was used to open the video (often including source from which we got the video).

So the user has stuff like tracking parameters in mind. The basic share button and the timestamp share button should stay as they are. Everything else should be removed from the shared URL.

If that means that you only need to write tests, great!