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.

Sign up for GitHub

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

Update more in-app Stats texts to always use western arabic numerals #17217

Merged
merged 9 commits into from
Oct 12, 2022
Merged

Update more in-app Stats texts to always use western arabic numerals #17217

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0c5dfc9
Extract CharSequence.enforceWesternArabicNumerals
ovitrif Sep 27, 2022
b65822f
Enforce Western Arabic numerals in Views and Visitors Stats chart on …
ovitrif Sep 27, 2022
af6b25c
Enforce Western Arabic numerals in Stats text blocks
ovitrif Sep 27, 2022
6f6a27e
Enforce LTR layout direction for Stats change text view
ovitrif Sep 27, 2022
3a004f8
Enforce LTR layout direction for Stats change text view
ovitrif Sep 27, 2022
8bf25e3
Merge remote-tracking branch 'origin/fix/20.8-western-arabic-numerals…
ovitrif Sep 27, 2022
fc743ff
Remove unused imports from MaterialTextViewWithNumerals
ovitrif Sep 27, 2022
b5589f1
Merge branch 'trunk' into fix/20.8-western-arabic-numerals
ovitrif Oct 11, 2022
32cf991
Update release notes for 17217
ovitrif Oct 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion ...press/android/ui/stats/refresh/lists/sections/insights/usecases/ViewsAndVisitorsMapper.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import org.wordpress.android.ui.stats.refresh.utils.MILLION
import org.wordpress.android.ui.stats.refresh.utils.StatsDateFormatter
import org.wordpress.android.ui.stats.refresh.utils.StatsUtils
import org.wordpress.android.ui.utils.ListItemInteraction
import org.wordpress.android.util.extensions.enforceWesternArabicNumerals
import org.wordpress.android.viewmodel.ResourceProvider
import javax.inject.Inject

Expand Down Expand Up @@ -117,7 +118,7 @@ class ViewsAndVisitorsMapper
val value = it.getValue(selectedType)
val date = statsDateFormatter.parseStatsDate(statsGranularity, it.period)
Line(
statsDateFormatter.printDayWithoutYear(date),
statsDateFormatter.printDayWithoutYear(date).enforceWesternArabicNumerals() as String,
it.period,
value.toInt()
)
Expand Down
26 changes: 26 additions & 0 deletions WordPress/src/main/java/org/wordpress/android/util/extensions/StringExtensions.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
package org.wordpress.android.util.extensions

import android.annotation.SuppressLint
import android.text.SpannableString
import android.text.Spanned
import android.text.TextUtils
import java.util.Locale

/**
Expand All @@ -17,3 +20,26 @@ import java.util.Locale
fun String.capitalizeWithLocaleWithoutLint(locale: Locale): String {
return this.capitalize(locale)
}

/**
* Converts digits to Western Arabic -- Workaround for an issue in Android that shows Eastern Arabic numerals.
* There is an issue in Google's bug tracker for this: [SO Answer](https://stackoverflow.com/a/34612487/4129245).
* The returned String uses the default Locale.
* @return a String with numerals in Western Arabic format persisting spans if available.
*/
fun CharSequence.enforceWesternArabicNumerals(): CharSequence {
ovitrif marked this conversation as resolved.
Show resolved Hide resolved
val textWithArabicNumerals = this
// Replace Eastern Arabic numerals
.replace(Regex("[٠-٩]")) { match -> (match.value.single() - '٠').toString() }
// Replace Persian/Urdu numerals
.replace(Regex("[۰-۹]")) { match -> (match.value.single() - '۰').toString() }

// Restore spans if text is an instance of Spanned
if (this is Spanned) {
val spannableText = SpannableString(textWithArabicNumerals)
TextUtils.copySpansFrom(this, 0, this.length, null, spannableText, 0)
return spannableText
}
ovitrif marked this conversation as resolved.
Show resolved Hide resolved

return textWithArabicNumerals
}
18 changes: 1 addition & 17 deletions WordPress/src/main/java/org/wordpress/android/widgets/MaterialTextViewWithNumerals.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import android.text.Spanned
import android.text.TextUtils
import android.util.AttributeSet
import com.google.android.material.textview.MaterialTextView
import org.wordpress.android.util.extensions.enforceWesternArabicNumerals

/**
* MaterialTextView which enforces Western Arabic numerals.
Expand All @@ -18,21 +19,4 @@ class MaterialTextViewWithNumerals @JvmOverloads constructor(
override fun setText(text: CharSequence?, type: BufferType?) {
super.setText(text?.enforceWesternArabicNumerals(), type)
}

private fun CharSequence.enforceWesternArabicNumerals(): CharSequence {
val textWithArabicNumerals = this
// Replace Eastern Arabic numerals
.replace(Regex("[٠-٩]")) { match -> (match.value.single() - '٠').toString() }
// Replace Persian/Urdu numerals
.replace(Regex("[۰-۹]")) { match -> (match.value.single() - '۰').toString() }

val spannableText = SpannableString(textWithArabicNumerals)

// Restore spans if text is an instance of Spanned
if (this is Spanned) {
TextUtils.copySpansFrom(this, 0, this.length, null, spannableText, 0)
}

return spannableText
}
}
2 changes: 1 addition & 1 deletion WordPress/src/main/res/layout/stats_block_text_item.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
android:paddingStart="@dimen/margin_extra_large"
android:paddingEnd="@dimen/margin_extra_large">

<com.google.android.material.textview.MaterialTextView
<org.wordpress.android.widgets.MaterialTextViewWithNumerals
android:id="@+id/text"
style="@style/StatsBlockText"
android:layout_width="wrap_content"
Expand Down
6 changes: 5 additions & 1 deletion WordPress/src/main/res/layout/stats_block_value_item.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
android:paddingStart="@dimen/margin_extra_large"
android:paddingTop="@dimen/margin_extra_large"
android:paddingEnd="@dimen/margin_extra_large"
xmlns:tools="http://schemas.android.com/tools"
android:paddingBottom="8dp">

<org.wordpress.android.widgets.MaterialTextViewWithNumerals
Expand Down Expand Up @@ -38,5 +39,8 @@
android:layout_gravity="bottom"
android:textColor="?attr/wpColorSuccess"
android:textSize="16sp"
android:visibility="gone" />
android:visibility="gone"
tools:text="-7 (-100%)"
android:layoutDirection="ltr"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a request for a change. Just curious why we set the layout direction here 🤔

Copy link
Contributor Author

@ovitrif ovitrif Oct 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for raising the question @antonis 🙇

I noticed this view was placing the number sign inverted when using a RTL language, see screenshot:

non_ltr

This isn't the case in other places because we always have a prefix / suffix or the number is part of a longer string.

Upon further investigation it appears this happens only when the text of the view contains a number and any other characters except letters.

This seems to be the behavior of text views in RTL languages, though not consistent after we've introduced the changes to use western arabic numerals.

A few other explorations without setting android:layoutDirection="ltr":

Prefix word Suffix word

Conclusion: In order to maintain a consistent position of the number sign with the other places, this change was indeed needed 🙂.

At the time I was also using Wikipedia to check the topic and my understanding was that the negative sign is positioned to the right only for eastern arabic numerals.

tools:visibility="visible"/>
</LinearLayout>