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

Improve the String::humanize_size() method #31993

Merged
merged 1 commit into from
Sep 7, 2019

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Sep 5, 2019

  • Use "B" insted of "Bytes" to be more compact
  • Use suffixes that denote a binary prefix
  • Make suffixes localizable

This removes the need for the custom EditorNetworkProfiler:_format_bandwidth() method, see discussion in #31870. We could also consider exposing this method to scripts, so that projects that need to display file sizes can use it directly.

cc @Faless

- Use "B" insted of "Bytes" to be more compact
- Use suffixes that denote a binary prefix
- Make suffixes localizable

This removes the need for the custom
`EditorNetworkProfiler:_format_bandwidth()` method.
Copy link
Collaborator

@Faless Faless 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.
Any particular reason for using TTR and RTR? Are those meant to be localized? How?

@Calinou
Copy link
Member Author

Calinou commented Sep 7, 2019

@Faless Some languages use different unit suffixes. For instance, in French, we use "o", "Kio", "Mio", "Gio", …

If I'm not mistaken, the .po generation process will look at both TTR() and RTR() strings (for use in the editor). Running projects could translate the value by translating strings like " KiB" (note the leading space), but the function isn't exposed to scripting yet.

As for the %s/s translation, I presume there are languages in which the word for "second" doesn't start with a "s" 🙂

@Faless Faless merged commit e9f49a6 into godotengine:master Sep 7, 2019
@Faless
Copy link
Collaborator

Faless commented Sep 7, 2019 via email

@akien-mga akien-mga added this to the 3.2 milestone Sep 23, 2019
@@ -3296,7 +3296,7 @@ String String::humanize_size(size_t p_size) {
int digits = prefix_idx > 0 ? _humanize_digits(p_size / _div) : 0;
double divisor = prefix_idx > 0 ? _div : 1;

return String::num(p_size / divisor).pad_decimals(digits) + prefix[prefix_idx];
return String::num(p_size / divisor).pad_decimals(digits) + RTR(prefix[prefix_idx]);
Copy link
Member

Choose a reason for hiding this comment

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

That's bogus, RTR() and TTR() strings are extracted, not compiled, and there's no string to translate in prefix[prefix_idx]. It's on the original strings themselves that you should use RTR (and the leading space should likely be left out).

@Calinou Calinou deleted the improve-string-humanize-size branch January 27, 2020 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants