Fix issue #4213: trim the last char of the URL if it is a closing parenthesis

[kirisakow]

Fix #4213 issue
trim the last char of the URL if it is a closing parenthesis

• I carefully read the contribution guidelines and agree to them.
• I have tested the API against NewPipe.
• I agree to create a pull request for NewPipe as soon as possible to make it compatible with the changed API.

[wb9688]

YouTube trims any additional characters from v. This implementation is incorrect.

[kirisakow]

Dear @wb9688, perhaps I was not clear enough. What I am trying to fix here is NewPipe's being unable to handle YouTube links which, inadvertedly, include a closing parenthesis at the end.

To test it empirically, I invite you to open this very page in your favorite web browser for Android, and try to open with NewPipe each one of the two links below: the first shall work, whereas the second shall not (with "Unsupported URL" for the message).

1. https://www.youtube.com/watch?v=mBuoZY7kAlg
2. https://www.youtube.com/watch?v=mBuoZY7kAlg)

And that's what I am trying to fix here by trimming the last character from the video's ID if that character is a closing parenthesis.

Thank you for your attention. Please help me implement that fix.

[wb9688]

Try opening e.g. https://www.youtube.com/watch?v=mBuoZY7kAlgwhatever. Also note how with e.g. https://www.youtube.com/watch?list=PLEqQd15F3NFGFUYJhz8KvGNz-QDhJVynT&v=OGS7c0-CmRswhatever it will strip the other parameters.

[kirisakow]

Dear friend, please watch me reproduce the issue with my Android device in Firefox for Android web browser:

https://youtu.be/bZP0jtp9WrQ

[wb9688]

Again, YouTube strips any additional characters (and if there are additional characters, it will strip other parameters), while NewPipe considers the URL simply invalid, however this PR naively strips the last character if it's ), which is also incorrect as it should exactly follow YouTube's behavior.

[kirisakow]

All right, as you wish

[opusforlife2]

@kirisakow He's talking about implementing the generic Youtube-like behaviour so it handles all cases of improper URLs, not just the parenthesis one. Did you close the PR because you don't wish to take that on?

[kirisakow]

Dear @opusforlife2, @wb9688,

I closed because l wasn't told what needs to be done — only that my solution was incorrect.

If you are open to tell me what you think needs to be done — I'll do it.

In the mean time, I don't want to spend my time guessing what you people expect me to do.

[Stypox]

Let me try to provide a clearer explanation, sorry for the confusion ;-)
Your implementation is not optimal since:

• it is applied to all services, but some services could have a closing parenthesis as part of the id, so in that case the link would be made invalid. Therefore it should only be applied to YouTube (see YoutubeStreamLinkHandlerFactory, YoutubePlaylistLinkHandlerFactory and the similar ones)
• it does not behave the same way as the official youtube client. What wb9688 suggested was to not only remove parenthesis, but also all spare characters at the end of the url, while you are at it. That's what the official yt app and official browser version do: every character after the id is just ignored. To make this compliant you could trim the id to only the first N characters (where N is the length of the id, =11 for yt videos)

Thank you for your contribution and sorry for the confusion ;-)

[Stypox]

Oh, forgot to add that you should keep this in mind while implementing: when the official browser version gets a video [didn't check other features] url with an id parameter v= longer than 11 chars, all of the following url parameters are ignored

[kirisakow]

I applied the suggested solution to the YoutubeStreamLinkHandlerFactory class alone (for now). Before I move on, please tell me if you guys are happy with it. @Stypox @wb9688 @opusforlife2 @gzsombor

(As for the numerous reverts, they had to be done to clean all the side effects induced by the automatic formatter — which I eventually disabled)

[opusforlife2]

I'm not a dev so I can't comment on that aspect, but thank you for continuing to work on this. 🥂

[Stypox]

This looks better, thank you ;-)
I only pointed out a few style issues, but the overall approach is good. Add final to all variables as you can, including function arguments.

[kirisakow]

@Stypox Done

[Stypox]

Please add one or two tests for urls with this longer-than-usual id (in YoutubeStreamLinkHandlerFactoryTest should do)
Thank you!

[TobiGr]

Thanks

[wb9688]

Issues I pointed out still aren't fixed…

[Stypox]

Even though you this does not handle urls exactly like youtube, like @wb9688 said, I prefer this behaviour. The reason that urls sometimes have additional characters at the end is not something done by youtube, but is something done by accident when writing e.g. messages. So if NewPipe can understand that a &list= url comes from a playlist instead of falling back to only using &v= like YouTube does it's only a plus. Therefore I'm merging this.

[Stypox]

I rebased on dev and run the tests again, everything works fine. Thank you!
@ TobiGr I can't merge due to other failing tests, could you do that? nevermind, I can