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

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

Open
2 tasks done
woj-tek opened this issue Mar 18, 2020 · 20 comments
Open
2 tasks done

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

woj-tek opened this issue Mar 18, 2020 · 20 comments
Labels
feature request Issue is related to a feature in the app

Comments

@woj-tek
Copy link
Contributor

woj-tek commented Mar 18, 2020

  • I carefully read the contribution guidelines and agree to them.
  • I checked if the issue/feature exists in the latest version.

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 mauriciocolli added feature request Issue is related to a feature in the app good first issue Easy/simple issues perfect for newcomers to get involved in the project labels Mar 18, 2020
@mauriciocolli
Copy link
Contributor

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 1ae54f6.

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
Copy link
Member

B0pol commented Mar 18, 2020

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

@mauriciocolli
Copy link
Contributor

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
Copy link
Contributor Author

woj-tek commented Mar 18, 2020

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

EDIT: correct ticket

@gkeegan
Copy link
Contributor

gkeegan commented Mar 19, 2020

@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
Copy link
Contributor Author

woj-tek commented Mar 19, 2020

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
Copy link
Contributor

gkeegan commented Mar 19, 2020

What? That doesn't mention timestamps at all.

@B0pol
Copy link
Member

B0pol commented Mar 19, 2020

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
Copy link
Contributor Author

woj-tek commented Mar 19, 2020

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Contributor

@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).

  • getOriginalUrl → The url that was used to open the stream/item.
  • getUrl → A cleaned url that the extractor gives us.

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
Copy link
Collaborator

opusforlife2 commented Mar 23, 2020

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
Copy link
Contributor

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

@opusforlife2
Copy link
Collaborator

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

@SydneyDrone
Copy link
Contributor

@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
Copy link
Collaborator

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

@SydneyDrone
Copy link
Contributor

@opusforlife2 OK, I'm working on it.

@SydneyDrone
Copy link
Contributor

SydneyDrone commented Apr 11, 2022

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
Copy link
Collaborator

@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!

@litetex litetex removed the good first issue Easy/simple issues perfect for newcomers to get involved in the project label Apr 24, 2022
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
7 participants