Merge 21.0 (21.0.0) code freeze into `trunk` #19472

mokagio · 2022-10-17T02:40:35Z

Update internal Pods to stable version
New version header in RELEASE-NOTE.txt
Localizable.strings updated
release_notes.txt updated with notes from RELEASE-NOTE.txt for current version
Version update in .xcconfig

Notice the Sentry update, which happened because it is a Tracks dependency. For the first time ever, running `bundle exec pod update ...` printed this kind of warning: ``` [!] `<PBXResourcesBuildPhase UUID=`FABB1FAA2602FC2C00C8785C`>` attempted to initialize an object with an unknown UUID. `FEC3B81A26C2915A00A395C7` for attribute: `files`. This can be the result of a merge and the unknown UUID is being discarded. ```

Running `bundle exec pod update ...` in the previous commit, b2b297c979465c14814ddaed5c5b636e7f08160d, printed a warning: ``` [!] `<PBXResourcesBuildPhase UUID=`FABB1FAA2602FC2C00C8785C`>` attempted to initialize an object with an unknown UUID. `FEC3B81A26C2915A00A395C7` for attribute: `files`. This can be the result of a merge and the unknown UUID is being discarded. ``` CocoaPods suggests there might have been a merge conflict not resolved properly in the project file, which is something bound to happen from time to time in a project with as many contributors as ours—and because of the inconvenient way Xcode generates and manages the file. Whenever I run into that kind of issue on my end, I add a new file then remove it. That procedure is enough to make Xcode "refresh" the project file, which usually gets rid of any dead reference, like the one CocoaPods highlighted. And that's exactly what this commit tracks. I also verified this changed by running the tests.

`bundle exec fastlane complete_code_freeze` failed with the following error, thrown as part of the `generate_strings_file_for_glotpress` lane: ``` [13:20:21]: genstrings: error: bad entry in file WordPress/Classes/ViewRelated/Stats/Insights/ViewsVisitors/ViewsVisitorsLineChartCell.swift (line = 58): Argument is not a literal string. [13:20:21]: fastlane finished with errors [!] genstrings: error: bad entry in file WordPress/Classes/ViewRelated/Stats/Insights/ViewsVisitors/ViewsVisitorsLineChartCell.swift (line = 58): Argument is not a literal string. ```

peril-wordpress-mobile · 2022-10-17T02:40:43Z

	Warnings
⚠️	The AppStoreStrings.po file must be updated any time changes are made to release notes

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

Generated by 🚫 dangerJS

mokagio · 2022-10-17T02:47:16Z

WordPress/WordPress.xcodeproj/project.pbxproj

@@ -18418,7 +18416,6 @@
 				17039226282E6D2F00F602E9 /* ViewsVisitorsLineChartCell.xib in Resources */,
 				FABB28472603067C00C8785C /* Launch Screen.storyboard in Resources */,
 				F465978F28E65F8A00D5F49A /* [email protected] in Resources */,
-				FEC3B81A26C2915A00A395C7 /* SingleButtonTableViewCell.xib in Resources */,


The changes in this file clean up what was most likely an incorrect merge on the project file.

See 3fc0ac0.

mokagio · 2022-10-17T03:02:12Z

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

-        let stringFormatValue = differencePercent != 0 ? "%@%@ (%@%%)" : "%@%@"
-        let stringFormat = NSLocalizedString(stringFormatValue, comment: "Difference label for Insights Overview stat, indicating change from previous period. Ex: +99.9K(5%)")
-        return String.localizedStringWithFormat(
-            stringFormat,
-            difference < 0 ? "" : "+",
-            difference.abbreviatedString(),
-            differencePercent.abbreviatedString()
-        )
+        // We want to show something like "+1.2K (5%)" if we have a percentage difference and "1.2K" if we don't.
+        // Because localized strings need to be strings literal, we cannot embed any conditional logic in the `localizedString...` call.
+        // We therefore need to generate different string literals base on the state.
+        let differenceSign = difference < 0 ? "" : "+"
+        if differencePercent != 0 {
+            let stringFormat = NSLocalizedString(
+                "insights.visitorsLineChartCell.differenceLabelWithPercentage",
+                value: "%@%@ (%@%%)",
+                comment: "Difference label for Insights Overview stat, indicating change from previous period, including percentage value. Example: +99.9K (5%)"
+            )
+            return String.localizedStringWithFormat(
+                stringFormat,
+                differenceSign,
+                difference.abbreviatedString(),
+                differencePercent.abbreviatedString()
+            )
+        } else {
+            let stringFormat = NSLocalizedString(
+                "insights.visitorsLineChartCell.differenceLabelWithoutPercentage",
+                value: "%@%@",
+                comment: "Difference label for Insights Overview stat, indicating change from previous period. Example: +99.9K"
+            )
+            return String.localizedStringWithFormat(
+                stringFormat,
+                differenceSign,
+                difference.abbreviatedString()
+            )
+        }


While I was at it, I also added reverse-DNS keys to that we can avoid ambiguous translations. See #19028.

FYI @staskus, as per this commit.

Also, I noticed there is no unit test for this logic. It would be great to add some, even though they wouldn't have caught this issue in particular.

@mokagio

Thank you for noticing the issue and making the fixes! 🤝

Should I add additional unit tests and additional cases that @AliSoftware was mentioning as a part of this PR?

wpmobilebot · 2022-10-17T03:22:55Z

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 pr19472-611dd09 on your iPhone

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

wpmobilebot · 2022-10-17T03:23:05Z

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 pr19472-611dd09 on your iPhone

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

AliSoftware

Pods updated to stable version
- ⚠️ See inline comment about >=
New header in RELEASE-NOTES.txt
JP and WP release_notes.txt updated with draft notes
JP release notes audited for differences between WP and JP
- Changes from 19383, 19378 and 19414 only kept in JP release notes and removed from WP 👍
WordPress/Resources/en.lproj/Localizable.strings updated with latest strings
Version bump in .xcconfig files

Still requested changes because I think we shouldn't use >= in the Podfile and keep ~> instead, but also because I think the strings for the Insights needs to be split even further, in 4 strings instead of 2. And if we do so, better do it before sending those strings to polyglots for translation.

Podfile

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

AliSoftware · 2022-10-17T07:21:52Z

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

-            differencePercent.abbreviatedString()
-        )
+        // We want to show something like "+1.2K (5%)" if we have a percentage difference and "1.2K" if we don't.
+        // Because localized strings need to be strings literal, we cannot embed any conditional logic in the `localizedString...` call.


👍 Nice catch!

Not sure how that was missed in the original PR 😓, did gentrings warned you about it during code freeze, telling you that the parameters have to be static values? If so, maybe we should run genstrings on CI on each PR (even if we don't commit the generated Localizable.strings on those PR CI runs, but just to gather potential warnings generated by it)? 🤔

did gentrings warned you about it during code freeze, telling you that the parameters have to be static values?

Yes, phew. The error was:

[!] genstrings: error: bad entry in file WordPress/Classes/ViewRelated/Stats/Insights/ViewsVisitors/ViewsVisitorsLineChartCell.swift (line = 58): Argument is not a literal string.

If so, maybe we should run genstrings on CI on each PR (even if we don't commit the generated Localizable.strings on those PR CI runs, but just to gather potential warnings generated by it)? 🤔

Yes! I thought about that too but didn't flesh out how it could work. I'd like a dedicated step, so the failure message is clear, but I worry if it's a waste to spin up a macOS box etc. "just" for that.

but I worry if it's a waste to spin up a macOS box etc. "just" for that.

Fair point. Maybe we could integrate that in an existing step (e.g. the one already doing swiftlint)?

AliSoftware · 2022-10-17T07:31:15Z

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

+        // We want to show something like "+1.2K (5%)" if we have a percentage difference and "1.2K" if we don't.
+        // Because localized strings need to be strings literal, we cannot embed any conditional logic in the `localizedString...` call.
+        // We therefore need to generate different string literals base on the state.
+        let differenceSign = difference < 0 ? "" : "+"


Even that conditional about differenceSign is making assumptions about the fact that those two variants would always be translated the same with just the added + in all languages — while depending on how different locales decide to display those values according to locale rules and practices, that might not be true (e.g. either they might put the + sign elsewhere like after instead of before in some locales, or they might have some other character for those cases, just like Arabic has a different Unicode character for the % sign, aka U+066A, or something else…)

So personally I'd even suggest to provide 4 different strings and not just 2, to cover all cases while allowing polyglots to still provide all variants matching their locale rules.

(See also: pbMoDN-3tH-p2#the-tips)

Good point, we shouldn't be assuming particular symbols or their order in different languages.

Good point. On it...

Co-authored-by: Olivier Halligon <[email protected]>

See discussion at #19472 (comment)

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

mokagio requested a review from AliSoftware 10 minutes ago

I think we need to wait for #19473 to land first before approving and (auto-)merging this PR to trunk, as we don't want the current keys to be sent to GlotPress and to start being localized by poyglots… if we're going to change them via #19473 right after (which will require us to land it to trunk again anyway).

So I'll wait for that other PR to land before updating my review of this one; but for the record, apart from that pending fix, the rest LGTM 👍

AliSoftware

Approving now that the tweak to fix L10n issue has been merged.

mokagio · 2022-10-18T07:27:14Z

Thank you for following up @AliSoftware

mokagio added 10 commits October 17, 2022 12:11

Bump version number

c6328b5

Update draft release notes for 21.0.

ff147e8

Update draft release notes for 21.0.

cde05ad

Release Notes: add new section for next version (21.1)

5ac3202

Remove Jetpack-only entries from WordPress 21.0 raw release notes

d751a7a

Remove "Jetpack-only" prefix from Jetpack 21.0 release notes

6b3286d

Update strings for localization

6e9225f

mokagio self-assigned this Oct 17, 2022

mokagio added the Releases label Oct 17, 2022

mokagio commented Oct 17, 2022

View reviewed changes

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

mokagio requested a review from a team October 17, 2022 03:02

mokagio marked this pull request as ready for review October 17, 2022 03:02

mokagio enabled auto-merge October 17, 2022 03:03

AliSoftware requested changes Oct 17, 2022

View reviewed changes

mokagio and others added 3 commits October 17, 2022 20:04

Fix a typo in a comment

47c1500

Co-authored-by: Olivier Halligon <[email protected]>

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

7e7210d

See discussion at #19472 (comment)

Update strings for localization

daec387

mokagio mentioned this pull request Oct 17, 2022

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

Merged

mokagio requested a review from AliSoftware October 17, 2022 11:21

mokagio and others added 2 commits October 18, 2022 12:56

Refine differenceLabel implementation and add tests

cd7b131

Update strings for localization

ede3b8e

Improve Insight differenceLabel localization logic (#19473)

611dd09

AliSoftware approved these changes Oct 18, 2022

View reviewed changes

mokagio merged commit 5ed3316 into trunk Oct 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge 21.0 (21.0.0) code freeze into `trunk` #19472

Merge 21.0 (21.0.0) code freeze into `trunk` #19472

mokagio commented Oct 17, 2022

peril-wordpress-mobile bot commented Oct 17, 2022

mokagio Oct 17, 2022

mokagio Oct 17, 2022

staskus Oct 17, 2022

wpmobilebot commented Oct 17, 2022 •

edited

Loading

wpmobilebot commented Oct 17, 2022 •

edited

Loading

AliSoftware left a comment •

edited

Loading

AliSoftware Oct 17, 2022

mokagio Oct 17, 2022

AliSoftware Oct 17, 2022 •

edited

Loading

AliSoftware Oct 17, 2022

staskus Oct 17, 2022

mokagio Oct 17, 2022

AliSoftware commented Oct 17, 2022

AliSoftware left a comment

mokagio commented Oct 18, 2022

Merge 21.0 (21.0.0) code freeze into trunk #19472

Merge 21.0 (21.0.0) code freeze into trunk #19472

Conversation

mokagio commented Oct 17, 2022

peril-wordpress-mobile bot commented Oct 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wpmobilebot commented Oct 17, 2022 • edited Loading

wpmobilebot commented Oct 17, 2022 • edited Loading

AliSoftware left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AliSoftware Oct 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AliSoftware commented Oct 17, 2022

AliSoftware left a comment

Choose a reason for hiding this comment

mokagio commented Oct 18, 2022

Merge 21.0 (21.0.0) code freeze into `trunk` #19472

Merge 21.0 (21.0.0) code freeze into `trunk` #19472

wpmobilebot commented Oct 17, 2022 •

edited

Loading

wpmobilebot commented Oct 17, 2022 •

edited

Loading

AliSoftware left a comment •

edited

Loading

AliSoftware Oct 17, 2022 •

edited

Loading