Tweak a localization to remove any assumption on "+" localization #19473

mokagio · 2022-10-17T11:11:25Z

See discussion at #19472 (comment).

I opened a dedicated PR for this instead of committing directly to the release branch because the change is big and I didn't want to let a mistake land directly in the release branch.

Notice daec387 which successfully updates the .strings via the generate_strings_file_for_glotpress lane. I made this as part of this PR to verify my changed was correct, as far as our tooling, or rather genstring, goes.

@staskus you have more context on this than I do. I think I implemented a 1-1 version of the end result that e3c3e41 would have produced. Still, why don't we show a minus sign, -, when the difference is less than 0?

See discussion at #19472 (comment)

AliSoftware · 2022-10-17T11:26:44Z

WordPress/Classes/ViewRelated/Stats/Insights/ViewsVisitors/ViewsVisitorsLineChartCell.swift

+        switch (difference > 0, differencePercent != 0) {
+        case (true, true): // E.g.: +1.2k (5%)
+            stringFormat = NSLocalizedString(
+                "insights.visitorsLineChartCell.differenceLabelPosiviteWithPercentage",


Suggested change

"insights.visitorsLineChartCell.differenceLabelPosiviteWithPercentage",

"insights.visitorsLineChartCell.differenceLabelPositiveWithPercentage",

AliSoftware · 2022-10-17T11:27:06Z

WordPress/Classes/ViewRelated/Stats/Insights/ViewsVisitors/ViewsVisitorsLineChartCell.swift

+            // We cannot assume every locale would translate an English string like "+1.2k" in the same way.
+            // So, even though we have only a "+" prefix, we ought to make this string localized.
+            stringFormat = NSLocalizedString(
+                "insights.visitorsLineChartCell.differenceLabelPosiviteWithoutPercentage",


Suggested change

"insights.visitorsLineChartCell.differenceLabelPosiviteWithoutPercentage",

"insights.visitorsLineChartCell.differenceLabelPositiveWithoutPercentage",

AliSoftware · 2022-10-17T11:27:45Z

WordPress/Classes/ViewRelated/Stats/Insights/ViewsVisitors/ViewsVisitorsLineChartCell.swift

                difference.abbreviatedString()
            )
+        case (false, true): // E.g. 1.2k (5%)
+            stringFormat = NSLocalizedString(
+                "insights.visitorsLineChartCell.differenceLabelNotPosiviteWithPercentage",


Suggested change

"insights.visitorsLineChartCell.differenceLabelNotPosiviteWithPercentage",

"insights.visitorsLineChartCell.differenceLabelNonPositiveWithPercentage",

Or (depending on @staskus 's reply to your answer in the PR description about why we don't use negative sign in the copy):

Suggested change

"insights.visitorsLineChartCell.differenceLabelNotPosiviteWithPercentage",

"insights.visitorsLineChartCell.differenceLabelNegativeWithPercentage",

staskus · 2022-10-17T11:32:00Z

@mokagio

Thank you for the fixes! I'll review it now.

Still, why don't we show a minus sign, -, when the difference is less than 0?

We do show negative signs. Negative numbers, both absolute and percentages appear with a "-" automatically when they're negative numbers. So it wasn't explicitly included in the localization.

However, to be explicit from the localization POV, we could pass abs(...) numbers to a formatting function and request a localization with a minus (or an equivalent localized) sign.

staskus · 2022-10-17T11:38:02Z

WordPress/Classes/ViewRelated/Stats/Insights/ViewsVisitors/ViewsVisitorsLineChartCell.swift

                difference.abbreviatedString()
            )
+        case (false, true): // E.g. 1.2k (5%)


So yes, if we want to be explicit about the localization negative case, then we should handle cases when difference == 0 and difference < 0 differently:

difference == 0 no sign

difference < 0 negative sign in a localization, pass abs(difference).abbreviatedString()

mokagio · 2022-10-17T11:45:11Z

However, to be explicit from the localization POV, we could pass abs(...) numbers to a formatting function and request a localization with a minus (or an equivalent localized) sign.

I think that would be the best course of action, consistent with the rationale applied for the + sign.

It's too late in my day. If no one gets to this before me, I'll look at this ASAP tomorrow.

AliSoftware · 2022-10-17T15:34:53Z

However, to be explicit from the localization POV, we could pass abs(...) numbers to a formatting function and request a localization with a minus (or an equivalent localized) sign.

Actually, I'm not sure if this would be a better approach compared to using a NSNumberFormatter and let it do the proper formatting depending on the user's locale for us automatically. In face, if we configure our NSNumberFormatter property to even include the + sign if the value is positive (I think the API has an option for it?), then we'd be back to just needing only 2 strings to localize (with or without the %), and let the whole formatting logic to the system instead of letting the polyglots assume responsibility for it.

Actually, while we're at it, we should even use an NumberFormatter for the percentage as well, so that it uses the right numberFormatter.percentSymbol automatically depending on the formatter's locale (assuming we use numberStyle = .percent on it)… and if we do so, we might not even need any special localization anymore?

For example, I think this should do it without even requiring manual (NSLocalizedString) localization at all, just letting it all to the system to handle all cases

    var differenceLabel: String {
        var string: String
        
        let formatter = NumberFormatter()
        formatter.positivePrefix = formatter.plusSign // Enforce + sign if value is positive
        string = formatter.string(from: difference as NSNumber) ?? ""
        
        if differencePercent != 0 {
            formatter.numberStyle = .percent
            formatter.positivePrefix = "" // Reset the + prefix for the percentage value. Or should we?
            string += formatter.string(from: differencePercent as NSNumber).map { " (\($0))" } ?? ""
        }
        return string
    }

[EDIT] Oh wait, doing so won't output 5.7k for difference = 5_700, but would output 5700 instead. So we would lose the benefit of abbreviatedString…

Ok, so if you want the k you might rely on MeasurementFormatter instead…

…and a dimensionless Unit subclass

@objc class VisitCount: Unit {
    override init(symbol: String) { super.init(symbol: symbol) }
    @available(*, unavailable)
    required init(coder: NSCoder) { fatalError("Unimplemented") }
    
    static func measurement(from count: Int) -> Measurement<VisitCount> {
        switch abs(count) {
        case 0..<1_000:
            return Measurement(value: Double(count), unit: VisitCount(symbol: ""))
        case ..<1_000_000:
            let symbol = NSLocalizedString("visit.count.prefix.thousand", value: "k")
            return Measurement(value: Double(count/1_000), unit: VisitCount(symbol: symbol)
        default:
            let symbol = NSLocalizedString("visit.count.prefix.millions", value: "M")
            return Measurement(value: Double(count/1_000_000), unit: VisitCount(symbol: symbol))
        }
    }
}

struct StatsSegmentedControlData {
    …
    let difference: Int
    let differencePercent: Int
    …
    
    var differenceLabel: String {
        var string: String

        let nf = NumberFormatter()
        nf.positivePrefix = nf.plusSign // Enforce + sign if value is positive
        
        let mf = MeasurementFormatter()
        mf.numberFormatter = nf
        let differenceMeasurement = VisitCount.measurement(from: difference)
        string = mf.string(from: differenceMeasurement)
        
        if differencePercent != 0 {
            nf.numberStyle = .percent
//            nf.positivePrefix = "" // Reset the + prefix for the percentage value. Or should we?
            string += nf.string(from: Double(differencePercent)/100.0 as NSNumber).map { " (\($0))" } ?? ""
        }
        return string
    }
}

But going with that solution might start to go a bit astray from the current implementation and kind of re-inventing the abbreviatedString() utility… so that might bring us a bit too much changes for a PR targeting the release/ branch, and we might want to keep that for a future PR for next version instead.

If our abbreviatedString()'s implementation is already localized, then maybe just using NumberFormatter().plusSign as a string prefix if value>0, and using a NumberFormatter for the percent part when it's needed, can be enough after all, at least for this sprint, and thus not require additional localization of all the cases?

staskus · 2022-10-17T18:23:00Z

@AliSoftware

Actually, I'm not sure if this would be a better approach compared to using a NSNumberFormatter and let it do the proper formatting depending on the user's locale for us automatically.

Thanks for putting your thought into it 👍

I agree, handling all these additional cases manually seems like a stretch at the moment, especially given that a lot of the localization logic is already built in.

We could:

When percentages exist, keep the original NSLocalizedString(...) since it was already localized before the release, and improve it with formatter.plusSign
When percentages don't exist, simple return formatter.plusSign + difference.abbreviatedString()

Working code example

    var differenceLabel: String {
        // We want to show something like "+1.2K (+5%)" if we have a percentage difference and "1.2K" if we don't.
        // Negative cases automatically appear with a negative sign "-1.2K (-5%)" by using abbreviatedString()
        let formatter = NumberFormatter()
        formatter.locale = .current
        let plusSign = difference < 0 ? "" : "\(formatter.plusSign ?? "")"
        
        if differencePercent != 0 {
            let stringFormat = NSLocalizedString("%@%@ (%@%%)", comment: "Difference label for Insights Overview stat, indicating change from previous period. Ex: +99.9K(5%)")
            return String.localizedStringWithFormat(stringFormat,
                    plusSign,
                    difference.abbreviatedString(),
                    differencePercent.abbreviatedString())
        } else {
            return "\(plusSign)\(difference.abbreviatedString())"
        }
    }

AliSoftware · 2022-10-17T18:31:24Z

Sounds like a good compromise.

Only change I'd make to your code example is to use positional placeholders for the NSLocalizedString — i.e. %1$@%2$@ (%3$@%%) instead of %@%@ (%@%%) — and provide a more detailed translator comment to explain what each of the %1$@, %2%@ and %3$@` placeholders will be replaced with 🙂

[See also internal ref pbMoDN-1Df-p2 for more documentation/tips about our suggested practices about localization and positional placeholder]

peril-wordpress-mobile · 2022-10-18T01:57:15Z

	Messages
📖	This PR has the 'Releases' label: some checks will be skipped.

Generated by 🚫 dangerJS

mokagio · 2022-10-18T02:06:28Z

WordPress/Classes/ViewRelated/Stats/Insights/ViewsVisitors/ViewsVisitorsLineChartCell.swift

            let stringFormat = NSLocalizedString(
                "insights.visitorsLineChartCell.differenceLabelWithoutPercentage",
-                value: "%@%@",
-                comment: "Difference label for Insights Overview stat, indicating change from previous period. Example: +99.9K"
+                value: "%1$@%2$@",
+                comment: "Text for the Insights Overview stat difference label. Shows the change from the previous period. E.g.: +12.3K. %1$@ is the placeholder for the change sign ('-', '+', or none). %2$@ is the placeholder for the change numerical value."
            )
            return String.localizedStringWithFormat(
                stringFormat,
-                differenceSign,
+                plusSign,
                difference.abbreviatedString()
            )


I based this off @staskus suggestion but I decided to use a localized string in the case without the percentage as well, so that translators can apply the appropriate RTL.

I'm not actually sure whether that's necessary, though.

Maybe we should lean into NumberFormatter more for this? There's definitely rooms for improvement and internal refactors. The new tests added in this diff will help us with that in the future.

Thanks for the fix. 👍 I think the solution makes sense. We improved it by using numbered positions, localized plus signs, and most importantly, having localized strings static and not dynamic.

wpmobilebot · 2022-10-18T02:25:06Z

You can test the changes in Jetpack from this Pull Request by:

Clicking here or scanning the QR code below to access App Center
Then installing the build number pr19473-ede3b8e on your iPhone

If you need access to App Center, please ask a maintainer to add you.

wpmobilebot · 2022-10-18T05:59:36Z

You can test the changes in WordPress from this Pull Request by:

Clicking here or scanning the QR code below to access App Center
Then installing the build number pr19473-ede3b8e on your iPhone

If you need access to App Center, please ask a maintainer to add you.

staskus

Thank you for the work and additional tests 🙏

LGTM, I reviewed the code and tested the solution.

mokagio · 2022-10-18T07:02:04Z

Dismissed Olivier's review because I did my best to address the existing concerns, have received another approval, and want to get the code freeze merged into trunk (for which this PR is a required step) ASAP.

Proceeding to merge now.

If we missed anything, we can always issue a new round of localizations.

Dismissed Olivier's review because I did my best to address the existing concerns, have received another approval, and want to get the code freeze merged into trunk (for which this PR is a required step) ASAP.

mokagio added 2 commits October 17, 2022 22:02

Tweak a localization to remove any assumption on "+" localization

7e7210d

See discussion at #19472 (comment)

Update strings for localization

daec387

mokagio requested review from staskus and a team October 17, 2022 11:17

mokagio added Localization Releases labels Oct 17, 2022

mokagio added this to the 21.0 ❄️ milestone Oct 17, 2022

mokagio marked this pull request as ready for review October 17, 2022 11:19

AliSoftware previously requested changes Oct 17, 2022

View reviewed changes

AliSoftware mentioned this pull request Oct 17, 2022

Merge 21.0 (21.0.0) code freeze into trunk #19472

Merged

5 tasks

staskus reviewed Oct 17, 2022

View reviewed changes

Refine differenceLabel implementation and add tests

cd7b131

Update strings for localization

ede3b8e

mokagio commented Oct 18, 2022

View reviewed changes

mokagio requested review from staskus and AliSoftware October 18, 2022 02:06

staskus approved these changes Oct 18, 2022

View reviewed changes

mokagio merged commit 611dd09 into release/21.0 Oct 18, 2022

mokagio deleted the mokagio/21.0-localization-tweak branch October 18, 2022 07:03

mokagio mentioned this pull request Nov 2, 2022

Run genstrings in CI to validate LocalizedString usages #19553

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tweak a localization to remove any assumption on "+" localization #19473

Tweak a localization to remove any assumption on "+" localization #19473

mokagio commented Oct 17, 2022 •

edited

Loading

AliSoftware Oct 17, 2022

AliSoftware Oct 17, 2022

AliSoftware Oct 17, 2022

staskus commented Oct 17, 2022 •

edited

Loading

staskus Oct 17, 2022

mokagio commented Oct 17, 2022

AliSoftware commented Oct 17, 2022 •

edited

Loading

staskus commented Oct 17, 2022 •

edited

Loading

AliSoftware commented Oct 17, 2022 •

edited

Loading

peril-wordpress-mobile bot commented Oct 18, 2022

mokagio Oct 18, 2022 •

edited

Loading

staskus Oct 18, 2022

wpmobilebot commented Oct 18, 2022

wpmobilebot commented Oct 18, 2022

staskus left a comment

mokagio commented Oct 18, 2022 •

edited

Loading

	"insights.visitorsLineChartCell.differenceLabelPosiviteWithPercentage",
	"insights.visitorsLineChartCell.differenceLabelPositiveWithPercentage",

Tweak a localization to remove any assumption on "+" localization #19473

Tweak a localization to remove any assumption on "+" localization #19473

Conversation

mokagio commented Oct 17, 2022 • edited Loading

AliSoftware Oct 17, 2022

Choose a reason for hiding this comment

AliSoftware Oct 17, 2022

Choose a reason for hiding this comment

AliSoftware Oct 17, 2022

Choose a reason for hiding this comment

staskus commented Oct 17, 2022 • edited Loading

staskus Oct 17, 2022

Choose a reason for hiding this comment

mokagio commented Oct 17, 2022

AliSoftware commented Oct 17, 2022 • edited Loading

staskus commented Oct 17, 2022 • edited Loading

AliSoftware commented Oct 17, 2022 • edited Loading

peril-wordpress-mobile bot commented Oct 18, 2022

mokagio Oct 18, 2022 • edited Loading

Choose a reason for hiding this comment

staskus Oct 18, 2022

Choose a reason for hiding this comment

wpmobilebot commented Oct 18, 2022

wpmobilebot commented Oct 18, 2022

staskus left a comment

Choose a reason for hiding this comment

mokagio commented Oct 18, 2022 • edited Loading

mokagio commented Oct 17, 2022 •

edited

Loading

staskus commented Oct 17, 2022 •

edited

Loading

AliSoftware commented Oct 17, 2022 •

edited

Loading

staskus commented Oct 17, 2022 •

edited

Loading

AliSoftware commented Oct 17, 2022 •

edited

Loading

mokagio Oct 18, 2022 •

edited

Loading

mokagio commented Oct 18, 2022 •

edited

Loading