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.

Sign up for GitHub

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

Jump to bottom

Search: Add support for Youtube URLs #4146

Merged
merged 5 commits into from
Aug 24, 2024
Merged

Search: Add support for Youtube URLs #4146

merged 5 commits into from
Aug 24, 2024

Conversation

SamantazFox
Copy link
Member

Closes #3300

@SamantazFox SamantazFox marked this pull request as ready for review October 5, 2023 21:14
@SamantazFox SamantazFox requested a review from a team as a code owner October 5, 2023 21:14
@SamantazFox SamantazFox requested review from unixfox and syeopite and removed request for a team October 5, 2023 21:14
syeopite
syeopite reviewed Oct 22, 2023
View reviewed changes
src/invidious/yt_backend/url_sanitizer.cr Outdated Show resolved Hide resolved
src/invidious/yt_backend/url_sanitizer.cr Show resolved Hide resolved
src/invidious/yt_backend/url_sanitizer.cr Outdated Show resolved Hide resolved
@syeopite
Copy link
Member

Would it be possible to make this configurable (either in this PR or maybe after #2377)? Since this does prevent some search queries such as youtube.com from going through and instead just being redirected.

@SamantazFox
Copy link
Member Author

Searching youtube.com is skipped for this exact reason:
https://github.com/iv-org/invidious/pull/4146/files#diff-19bdcb6c58e32fcfbe37cf190733371fc06272002c38b847e114e9705b3ce649R149-R152

However, searching for youtube.com/ (note the trailing /) will trigger URL redirection

@syeopite
Copy link
Member

Oops I missed that.

But my point still stands. In general this PR blocks certain search queries from going through to YouTube due to the URL redirection functionality.

youtube.com
youtube.com/
youtube.com/watch?v=

etc all returns different search results on YouTube. And although this may be an edge case, it still does "block" certain search results from showing up on Invidious and that's something I'd like to avoid if possible.

@SamantazFox
Copy link
Member Author

Is that edge case worth the added complexity of another config option?
IIRC Piped has such a URL search feature and no toggle.

@syeopite
Copy link
Member

I think its worth it. Blocking search terms from going through to YouTube is something Invidious should never do imo edgecase or not.

@syeopite
Copy link
Member

syeopite commented Nov 24, 2023 •
edited
Loading

This is one of the few times where I disagree on doing something automatically for the sake of convenience. To give a personal anecdote, I've accidentally encountered the automatically redirection feature on Libreddit more than once when just trying to search for a subreddit name. Its way less of an issue on YouTube but it does exist.

There are many popular videos titled with just the Never Gonna Give You Up link for example. This feature (without an toggle) would make it harder and perhaps impossible to locate those videos through the search function.

@SamantazFox
Copy link
Member Author

What do you think of a DDG "bang" style trigger, instead? In essence, the URL search would trigger only if the search field contains exactly one valid URL, without any extra search terms, and preceded by !.

@syeopite
Copy link
Member

Won't that end up being the exact same problem? The search results for a link prefixed with a exclamation mark and one that isn't is slightly different. It should occur less than with just a link but it doesn't really solve the problem.

If a configuration option is too complex what about a "smart search" button directly within the search bar? Here's an example mock up with a placeholder icon.
smart search

Enabled by default, since this is an edge case, it can be disabled by toggling the button prior to a search result. The only additional complexity then would be a single check for a query parameter in the search handler.

@SamantazFox
Copy link
Member Author

Well, the probability that a user searches for an exact, valid Youtube URL, with a leading exclamation mark (that seem to be ignored by youtube anyway?) without any other search terms, is very thin, if not to say null.

I like the idea of a smart search thing, but I don't know how to integrate that nicely with the rest (a search button, for sure, and maybe a toggle for suggestions). I'm worried it'll clutter the search bar...

@syeopite
Copy link
Member

True. I'd just rather not add an option that restricts certain search results in Invidious no matter how much of an edge case it is.

but I don't know how to integrate that nicely with the rest...

I can take care of that in another PR. Could you just add a smart search url parameter for now?

I don't believe it will clutter the search bar that much if its well designed. Google for instance has four buttons (five when its focused) in their search bar.

@syeopite
Copy link
Member

syeopite commented Dec 2, 2023

Actually now that I think about it, I'll be fine with a bang system as long as the ! (and the backslash) can be escaped.

@SamantazFox
Copy link
Member Author

@syeopite As you requested, I added some logic to bypass this feature.

By default, if the search query is a valid youtube URL, it redirects to that page.

If the query starts with an excalmation mark (!), then the search returns to regular "dumb" search. If you still want to search for !www.youtube.com/etc.., you have to type a second !, like so: !!www.youtube.com/etc...

Is that good for you?

@syeopite
Copy link
Member

Yeah that's fine by me.

syeopite
syeopite approved these changes Feb 18, 2024
View reviewed changes
src/invidious/search/query.cr Show resolved Hide resolved
src/invidious/search/query.cr Outdated
Comment on lines 63 to 65
if @raw_query.starts_with?('!')
@inhibit_ssf = true
@raw_query = @raw_query[1..]
Copy link
Member

Choose a reason for hiding this comment

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

Using ! as an inhibitor seems a bit odd to me, especially when it functions as an escape. What do you think of using a backslash instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm neutral on this

Copy link
Member

Choose a reason for hiding this comment

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

If it doesn't make parsing more difficult then I'm in favor of using a backslash instead of an exclamation mark. Since usually an ! is to use a special smart functionality rather than negate it

@SamantazFox SamantazFox added need-testing This feature needs to be deployed and tested to see if it's working, and doesn't break something ready and removed need-testing This feature needs to be deployed and tested to see if it's working, and doesn't break something labels Aug 17, 2024
@SamantazFox SamantazFox merged commit 3c6a662 into iv-org:master Aug 24, 2024
7 of 8 checks passed
@SamantazFox SamantazFox deleted the url-search branch August 24, 2024 17:51
syeopite added a commit to syeopite/invidious that referenced this pull request Aug 24, 2024
@syeopite
Ameba: Fix Naming/PredicateName
aa0179a 
Introduced in iv-org#4146
@syeopite syeopite mentioned this pull request Aug 24, 2024
Merged
syeopite added a commit that referenced this pull request Aug 24, 2024
@syeopite
Fix lint errors introduced in #4146 and #4295 (#4876)
ccecc6d 
* Ameba: Fix Naming/VariableNames

Introduced in #4295

* Ameba: Fix Naming/PredicateName

Introduced in #4146
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@syeopite syeopite syeopite approved these changes

@unixfox unixfox Awaiting requested review from unixfox unixfox is a code owner automatically assigned from iv-org/developers

Assignees
No one assigned
Labels
ready
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Enhancement] [Search] Open video pasting YouTube URL
2 participants
@SamantazFox @syeopite