Skip to content

Conversation

@lukashert
Copy link

Adds Player#setEffectiveLocale(Locale) / Player#getEffectiveLocale() to set and get a player-specific locale, which is used for server-side translations.

Closes #10570

@lukashert lukashert requested a review from a team as a code owner April 16, 2025 17:43
@github-project-automation github-project-automation bot moved this to Awaiting review in Paper PR Queue Apr 16, 2025
@github-project-automation github-project-automation bot moved this from Awaiting review to Awaiting final testing in Paper PR Queue Apr 22, 2025
@github-project-automation github-project-automation bot moved this from Awaiting final testing to Changes required in Paper PR Queue Apr 25, 2025
@lukashert lukashert requested a review from lynxplay April 25, 2025 10:49
@lynxplay
Copy link
Contributor

I am semi uncertain on the naming "effective locale".
I know this mirrors velocity's API, however in the context of velocity, it being "effective" makes sense, given that all of velocity's messages are using the global translator to be resolved.

This differs drastically on backend servers like paper, where basically none of the messages sent are actually translated on the server side beyond plugin messages. Command feedback, server system messages, etc are all not translated on the server but instead resolved by the client itself.
Given I don't have an immediate better idea for the naming, nor am I sure this change in naming is 100% needed, I'll throw this at the rest of team/contributors.

@davidmayr
Copy link
Contributor

Given I don't have an immediate better idea for the naming, nor am I sure this change in naming is 100% needed, I'll throw this at the rest of team/contributors.

How about just mentioning what that locale actually does in the javadoc?

@lynxplay
Copy link
Contributor

Well, javadocs are a nice tool. But they are generally a second resort and do not save us from poorly named methods.
A user looking for a way to force a locale will still run into Player#setEffectiveLocale and just not read those docs until already hammering their head into a wall as to why it isn't working.

- public String language = "en_us";
+ public String language = null; // Paper - default to null
+ public java.util.Locale adventure$locale = java.util.Locale.US; // Paper
+ @Nullable
Copy link
Member

Choose a reason for hiding this comment

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

try put this in the same line because the comment only apply to the field and not this.

@davidmayr
Copy link
Contributor

Well, javadocs are a nice tool. But they are generally a second resort and do not save us from poorly named methods. A user looking for a way to force a locale will still run into Player#setEffectiveLocale and just not read those docs until already hammering their head into a wall as to why it isn't working.

I mean, I agree. But then again, there are no better name suggestions yet. And I think it makes sense to mention it regardless

@lynxplay
Copy link
Contributor

Yea certainly a good point to expand the javadocs here regardless 👍

@kezz
Copy link
Member

kezz commented Apr 25, 2025

Seeing as Velocity also has this and Adventure has a lot of requests of users wanting to override the locale used for server-side translating, I've gone ahead and opened a PR to implement this in Adventure directly, which also solves the issue this PR has about confusing developers. PaperMC/adventure#1221

@Brokkonaut
Copy link
Contributor

How about Player.get/setServerSideLocale() ?

@lynxplay
Copy link
Contributor

Yea, setServerSideLocale is a nice name, however the adventure PR might be nicer.

@Owen1212055
Copy link
Member

Seeing above, I am going to go ahead and close this PR.

Thank you so much for your contribution, but this will be much better supported natively by the adventure PR listed above.

@Owen1212055 Owen1212055 closed this May 1, 2025
@github-project-automation github-project-automation bot moved this from Changes required to Closed in Paper PR Queue May 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Closed

Development

Successfully merging this pull request may close these issues.

9 participants