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

[BUG] format_number and thousands separators #4911

Open
edent opened this issue Dec 1, 2022 · 5 comments
Open

[BUG] format_number and thousands separators #4911

edent opened this issue Dec 1, 2022 · 5 comments

Comments

@edent
Copy link

edent commented Dec 1, 2022

Describe the bug

Large numbers are formatted in an unexpected way. For example 1,024 is rendered as 1 024 - there is a space rather than a comma.

To Reproduce
Steps to reproduce the behavior:

  1. Add lots of feeds
  2. Go on holiday for a few weeks
  3. Look at the page title and unread count
  4. See error

Expected behavior

Different locales have different digit grouping. In the UK, I'd expect to see the , used to separate out the decimals.

Screenshots

Screenshot showing a written number with a space instead of a thousands separator.

Environment information (please complete the following information):

  • FreshRSS version: 1.20.1
  • PHP version: 8.1
  • Installation type: Softalicious

Additional context

The current code is:

function format_number($n, $precision = 0) {
	// number_format does not seem to be Unicode-compatible
	return str_replace(' ', ' ',	// Thin non-breaking space
		number_format($n, $precision, '.', ' ')
	);
}

I think the number_format should be:

number_format($n, $precision, '.', ',')

However, you might want to include decimal and thousands separators in the i18n files?

@Frenzie
Copy link
Member

Frenzie commented Dec 1, 2022

However, you might want to include decimal and thousands separators in the i18n files?

Please avoid that at all costs.

@edent
Copy link
Author

edent commented Dec 1, 2022

Haha! Fair enough! A better way is probably

setlocale(LC_NUMERIC, "en_GB.UTF-8");
$separator = localeconv()["thousands_sep"];

That depends on if you can set the locale based on the user interface language.

@Frenzie
Copy link
Member

Frenzie commented Dec 1, 2022

That's actually still what I want to avoid at all costs. A random sample of horrific user experiences includes:

  • The Nvidia driver and various other programs take the location for showing a UI language instead of… the language. Nvidia may have fixed this by now.
  • The language being used as a decision for how to display dates, numbers, etc. (E.g., many Americans living in Europe want US English + sane dates like dd-mm-yyyy or yyyy-mm-dd instead of mm-dd-yyyy, Euro for currency, etc.)

So "in the i18n files" is fine provided it means something like a completely separate file parsed by a completely different setting.

@Alkarex
Copy link
Member

Alkarex commented Mar 21, 2023

I have been slow there, but here is a comment at last @Frenzie : we have a i18n/en, and a i18n/en-US for instance. In those languages, aren't number formatting rules defined by precisely the language and not by something else?
If I am not mistaken, for ISO, our current space is OK for formatting numbers, while , and . must be handled with care as the meaning vary depending on language/culture.

@Frenzie
Copy link
Member

Frenzie commented Mar 22, 2023

While it's true there's a certain correlation between what's recommended by style guides/standards and languages, in all of Windows, Mac and Linux/BSD it works something like this.

image

That's because anything else provides an atrocious user experience. Whether you want colour or color or couleur and whether you want the first day of the week to be Monday or the time to be 24h or the date format to be dd-mm-yyyy are all completely separate things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants