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

[Core] Guild scoped I18n #3896

Merged
merged 36 commits into from Oct 26, 2020
Merged

Conversation

Kowlin
Copy link
Member

@Kowlin Kowlin commented Jun 1, 2020

Never again!

Type

  • Bugfix
  • Enhancement
  • New feature

Description of the changes

Add Guild i18n support.

TODO:
Clean up code
Add Guild scoped Babel Numeric formatter support.

Fixes #1970

Never again!
@Jackenmen Jackenmen added Category: Core - i18n This is related to the i18n core API or locale files. Type: Enhancement Something meant to enhance existing Red features. Type: Feature New feature or request. labels Jun 1, 2020
@Kowlin Kowlin changed the title Guild I18n [Core] Guild scoped I18n Jun 1, 2020
@Jackenmen Jackenmen linked an issue Jun 1, 2020 that may be closed by this pull request
redbot/core/core_commands.py Outdated Show resolved Hide resolved
@Kowlin Kowlin linked an issue Jun 1, 2020 that may be closed by this pull request
3 tasks
@Kowlin Kowlin marked this pull request as ready for review June 2, 2020 00:07
@Kowlin Kowlin requested a review from tekulvw as a code owner June 2, 2020 00:07
@Kowlin
Copy link
Member Author

Kowlin commented Jun 11, 2020

Guild scoped i18n currently has an issue whereby modlog cases cannot be translated, this is not the fault of i18n, Modlog doesn't account for i18n existing at all, this PR should be good to merge and modlog handling can be done at a latter date. If it proofs that people are interested in,

@Kowlin Kowlin added this to the 3.3.10 milestone Jun 18, 2020
@mikeshardmind
Copy link
Contributor

This appears to be working as intended, but I'm going to take a closer look a little later at the memory use and how it scales based on concerns the last time this came up (toby's old PR)

@mikeshardmind
Copy link
Contributor

We may want a command for the bot owner to designate which locales are available, defaulting to all and release notes about this. Resource use grows as needed but will be higher.

@Kowlin
Copy link
Member Author

Kowlin commented Jun 21, 2020

Its a fair suggestion, but based on the calculations I did, I came around a total memory usage of 1.4 MB from French.

accounting for the fact that its ~460KB at 70% completion, adding another 30% on-top of that estimate leads it to being ~600KB of memory, doubling that to account for the overhead from Python.

Meaning that its very roughly ~1.5MB per language loaded.

@mikeshardmind
Copy link
Contributor

I was seeing about 15mb increase from french alone when accounting for everything from contextvars as well, i wonder if there may be more to that growth than the i18n itself though, give me a few more to look into this.

@mikeshardmind
Copy link
Contributor

Prior concern inflated by a tooling issue and a tool incorrectly assesing the lifecycle of contextvar held references.

Copy link
Contributor

@mikeshardmind mikeshardmind left a comment

Choose a reason for hiding this comment

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

There needs to be an exposed method for cog creators to set the contextvar to the appropriate locale from an event. Right now, set_contextual_locale works, but there's no supported way for cog creators to get the appropriate locale to set. (such as by asking by guild or guild id) This isn't necessary for command invokes, but would be needed for any messages sent by events to be handlable by cog creators.

@aikaterna
Copy link
Member

aikaterna commented Jun 22, 2020

While people are directed to Red’s Crowdin page to see what translations are available in [p]set globallocale or [p]set locale, we don’t seem to have a list of the language-country code pairs listed anywhere. For ease of use I would suggest having them listed somewhere, whether that is a command (might be a bit overboard imo), more help text in those commands (might be too crowded/long), or a link to somewhere where we can list those. If something like that is already available in the bot elsewhere or listed on Crowdin I am not aware of it.

Edit: [p]listlocales was a thing at one point... I don't know where it went, checking the bot now.
2nd Edit: Please add listlocales back to the bot. Was removed in #3676

It might also be helpful to have a display of the current guild and global locales in the informational display under [p]set, as the regional format override is displayed there already. Ideally I would like to see both global and local information for locale and regional format exposed in that info display, but of course this is just a suggestion for ux and I’d like to see what other people think, or if there is another way to display that for users.

@Jackenmen
Copy link
Member

Jackenmen commented Jun 22, 2020

This is a little bit problematic - locale can be set to any valid language code, not just those that we offer translations for. For one, 3rd-party cog creators might provide translations for languages we don't provide translations for, but also the regional formatting functionality allows you to set the formatting of any region. That's why we started to link to Crowdin to see available translations for Red. But you are right that it could not be immediately obvious what to set it to.
It would be great if Crowdin could just show it right under the language in the grid but sadly it doesn't... I think the link would probably still be the best, but if there is some website that lists all language code in en-US-like format, we could possibly link to that.

@mikeshardmind
Copy link
Contributor

The locales which we support are technically limited to validity under CLDR and recognized by babel

babel.localedata.locale_identifiers() [currently] generates a list of 758 of these, which would obviously be unruly for any realistic use by end-users.

We could create an interactive command which is hidden but pointed to which helps pick a locale based on an interactive prompt and use of babel.negotiate_locale (which compares requested locales with a set of "available" ones and tries to find the best fit, sacrificing region if no exact match) though this might create misleading circumstances when it results in recommending any of the languages we technically have Crowdin support for but under 10% translation.

I don't really have a good solution for this other than perhaps a webpage which we control or trust which checks the browser info to tell the user what their locale code actually is and how much if any of that locale is translated.

@Kowlin
Copy link
Member Author

Kowlin commented Jun 27, 2020

Delaying this until I find time to implement the requested features.

@Kowlin Kowlin removed this from the 3.3.10 milestone Jun 27, 2020
@Kowlin Kowlin removed the QA: Needed label Jun 27, 2020
@aikaterna
Copy link
Member

I tested this for close to a week with a few thousand users/few servers using it and no one was able to find a way to cause a traceback or unintended behaviour. Made sure to try different combos of the global regional format override and various languages as well, but only tested languages with a good amount of coverage (mostly FR, RU, and DE).

Copy link
Member

@Jackenmen Jackenmen left a comment

Choose a reason for hiding this comment

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

[p]set locale and [p]set regionalformat don't update the i18n cache.

Update: I meant [p]set globallocale and [p]set globalregionalformat, sorry about that 😬

Copy link
Member

@Jackenmen Jackenmen left a comment

Choose a reason for hiding this comment

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

To add, I've tested everything else and the aforementioned cache is the only issue BUT there are some core cogs that need to set contextual locale manually in events/tasks:

  • Audio's lavalink_event_handler() (I am about 76% certain that this is the only place in Audio that needs this, we can ask Draper to get this closer to 99% territory)
  • Filter's maybe_filter_name() and on_message()
  • Mod's on_message()
  • Report's on_message() and on_raw_reaction_add()
  • Streams's check_streams()

I can help out with this if you don't feel like doing it all, just hit me up ;)

Jackenmen
Jackenmen previously approved these changes Oct 15, 2020
Copy link
Member

@Jackenmen Jackenmen left a comment

Choose a reason for hiding this comment

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

Looks good to me, but I'll probably hold this unmerged until #3634 (unless that doesn't happen in 3.4.1) and then add i18n support for Mutes here and merge after testing.

@Jackenmen Jackenmen added Blocked By: Other PR Blocked by another PR. QA: Passed Used by few QA members. Has been approved by the assigned QA member(s). labels Oct 15, 2020
Co-authored-by: Draper <27962761+Drapersniper@users.noreply.github.com>
@Jackenmen Jackenmen removed the QA: Passed Used by few QA members. Has been approved by the assigned QA member(s). label Oct 22, 2020
# Conflicts:
#	redbot/cogs/audio/core/events/lavalink.py
@Jackenmen Jackenmen added Changelog Entry: Pending Changelog entry for this PR hasn't been added by repo maintainers yet. and removed Blocked By: Other PR Blocked by another PR. labels Oct 23, 2020
@Jackenmen
Copy link
Member

Jackenmen commented Oct 26, 2020

The changes I just pushed around Mutes cog need testing which I'll be doing later today.

Copy link
Member

@Jackenmen Jackenmen left a comment

Choose a reason for hiding this comment

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

LGTM

@Jackenmen Jackenmen merged commit 2413c6a into Cog-Creators:V3/develop Oct 26, 2020
@Jackenmen Jackenmen added Changelog Entry: Added Changelog entry for this PR has already been added to changelog PR. and removed Changelog Entry: Pending Changelog entry for this PR hasn't been added by repo maintainers yet. labels Oct 27, 2020
@TurnrDev
Copy link
Contributor

😍

@Kowlin Kowlin deleted the pr/guild-i18n branch February 12, 2021 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Core - i18n This is related to the i18n core API or locale files. Changelog Entry: Added Changelog entry for this PR has already been added to changelog PR. Type: Enhancement Something meant to enhance existing Red features. Type: Feature New feature or request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Per-server localising [V3 i18n] Guild-overides for localisations
8 participants