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

Use athena-negotiation to detect language through Accept-Language header #2324

Merged

Conversation

syeopite
Copy link
Member

@syeopite syeopite commented Aug 12, 2021

Closes #2074

Follow up to #2299 (comment)

Please give my bounty of #2074 to @afrmtbl when merging

@TheFrenchGhosty
Copy link
Member

I don't really understand the difference between athena and just the "normal" guess of the browser language... Isn't the "normal guess" supposed to detect the browser language? What does this do "more"?

@unixfox
Copy link
Member

unixfox commented Aug 12, 2021

I don't really understand the difference between athena and just the "normal" guess of the browser language... Isn't the "normal guess" supposed to detect the browser language? What does this do "more"?

There is no "normal guess", it's just the server that have some code on the server side that is detecting the proper browser language.

#2299 is trying to detect the proper browser language without introducing any external library/dependency, but it requires a lot of codes.
#2324 is trying to detect the proper browser language with an external library/dependency (athena), and the code is lighter.

@syeopite
Copy link
Member Author

syeopite commented Aug 12, 2021

The implementation in #2299 does an exact match. So, if we have a header like this:

en,fr-CH;q=0.2,zh-Hans-CN;q=0.9,de;q=0.1

And decide to use that against the languages supported by Invidious

We'll get de as that's the only locale that matches exactly. However, in the header both fr-CH and zh-Hans-CN have a higher precedence than de. Thus zh-CN (highest precedence) should actually be matched. Here's what a proper implementation should look like:

  • Attempt to match zh-Hans-CN (highest precedence)
  • Attempt to match zh-CN
  • If that fails we'll attempt to match zh-*
  • If that also fails we'll attempt to match zh.
  • Decent down precedence list to find a match with the above steps.

This is the behavior seen in athena

@TheFrenchGhosty
Copy link
Member

Perfect. Let's go with this then.

@syeopite syeopite force-pushed the accept-language-via-athena-negotiation branch from f2e2dfd to f2005aa Compare August 12, 2021 22:01
@syeopite syeopite force-pushed the accept-language-via-athena-negotiation branch from f2005aa to 3202e4b Compare August 12, 2021 23:56
@isaackd
Copy link
Contributor

isaackd commented Aug 13, 2021

@syeopite The reason I checked the header again on signup is for users that signup without any previously saved preferences.

The following code creates a new user:

user, sid = create_user(sid, email, password)

but uses the default user preferences:
preferences: Preferences.new(CONFIG.default_user_preferences.to_tuple),

the preferences for the new user are then updated, but only if they have a saved PREFS cookie:

if env.request.cookies["PREFS"]?
preferences = env.get("preferences").as(Preferences)
PG_DB.exec("UPDATE users SET preferences = $1 WHERE email = $2", preferences.to_json, user.email)
cookie = env.request.cookies["PREFS"]
cookie.expires = Time.utc(1990, 1, 1)
env.response.cookies << cookie
end

Without checking the header again during signup, the user will be created with the default locale of en.

@syeopite
Copy link
Member Author

Yep, I just realized that... I was actually writing a reply (discarded now) to my post above when you posted a comment here.

Either way, I'll go ahead and add back the extra parsing there.

@syeopite syeopite merged commit fceb809 into iv-org:master Aug 24, 2021
@syeopite syeopite deleted the accept-language-via-athena-negotiation branch August 24, 2021 19:59
@TheFrenchGhosty
Copy link
Member

@afrmtbl to receive your reward either send your BTC address in a comment (preferred), or contact us directly, with a proof that this is indeed you.

@isaackd
Copy link
Contributor

isaackd commented Aug 24, 2021

@afrmtbl to receive your reward either send your BTC address in a comment (preferred), or contact us directly, with a proof that this is indeed you.

The address is bc1q2ymxdl6fuwvudwg2qgclnjdj8ftaqkdkyqdgh0. thanks!

@TheFrenchGhosty
Copy link
Member

A 20$ bounty has been rewarded to @afrmtbl for this PR.

Transaction (48034f558cc7f2074ef1812f1320f50b8b48c4b7383a7680576b54e9866ef6a2):

Amount sent: 0.00041820 BTC (~17.05€ or ~20$)

Mining fees (5 sat/byte): 0.0000071 BTC (~0.29€)

@TheFrenchGhosty TheFrenchGhosty added the bounty:paid The bounty has been paid label Aug 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bounty:paid The bounty has been paid bounty
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Enhancement] Autodetect browser language
5 participants