fix: Correctly punctuate a status content description (#808)

Previous code blindly inserted commas and semi-colons as separators
between the components of a content description. If some of those
components were null you could have a content description that looked
like "... , , , ..." or similar, and the repeated reading of "comma" by
screen readers was jarring and reduced accessibility.

Fix this by inserting punctuation only where necessary, building up the
string piece by piece instead of using a string resource with hardcoded
punctuation.

Fixes #791.

app/lint-baseline.xml

@@ -716,6 +716,17 @@
             column="51"/>
     </issue>
 
+    <issue
+        id="Typos"
+        message="Repeated word &quot;til&quot; in message: possible typo"
+        errorLine1="    &lt;string name=&quot;action_add_to_tab&quot;>Legg til til fane&lt;/string>"
+        errorLine2="                                          ^">
+        <location
+            file="src/main/res/values-nb-rNO/strings.xml"
+            line="649"
+            column="43"/>
+    </issue>
+
     <issue
         id="ImpliedQuantity"
         message="The quantity `&apos;one&apos;` matches more than one specific number in this locale (0, 1), but the message did not include a formatting argument (such as `%d`). This is usually an internationalization error. See full issue explanation for more."
@@ -1185,7 +1196,7 @@
         errorLine2="                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~">
         <location
             file="src/main/res/values/donottranslate.xml"
-            line="278"
+            line="273"
             column="19"/>
     </issue>
 
@@ -1196,7 +1207,7 @@
         errorLine2="                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~">
         <location
             file="src/main/res/values/donottranslate.xml"
-            line="283"
+            line="278"
             column="19"/>
     </issue>
 

app/src/main/java/app/pachli/adapter/StatusBaseViewHolder.kt

@@ -740,7 +740,7 @@ abstract class StatusBaseViewHolder<T : IStatusViewData> protected constructor(i
             )
             setRebloggingEnabled(actionable.rebloggingAllowed(), actionable.visibility)
             setSpoilerAndContent(viewData, statusDisplayOptions, listener)
-            setDescriptionForStatus(viewData, statusDisplayOptions)
+            setContentDescriptionForStatus(viewData, statusDisplayOptions)
 
             // Workaround for RecyclerView 1.0.0 / androidx.core 1.0.0
             // RecyclerView tries to set AccessibilityDelegateCompat to null
@@ -759,41 +759,86 @@ abstract class StatusBaseViewHolder<T : IStatusViewData> protected constructor(i
         }
     }
 
-    private fun setDescriptionForStatus(
+    /** Creates and sets the content description for the status. */
-        viewData: T,
+    private fun setContentDescriptionForStatus(viewData: T, statusDisplayOptions: StatusDisplayOptions) {
-        statusDisplayOptions: StatusDisplayOptions,
-    ) {
         val (_, _, account, _, _, _, _, createdAt, editedAt, _, reblogsCount, favouritesCount, _, reblogged, favourited, bookmarked, sensitive, _, visibility) = viewData.actionable
-        val description = context.getString(
+
-            R.string.description_status,
+        // Build the content description using a string builder instead of a string resource
-            account.displayName,
+        // as there are many places where optional ";" and "," are needed.
-            getContentWarningDescription(context, viewData),
+        //
-            if (TextUtils.isEmpty(viewData.spoilerText) || !sensitive || viewData.isExpanded) viewData.content else "",
+        // The full string will look like (content in parentheses is optional),
-            getCreatedAtDescription(createdAt, statusDisplayOptions),
+        //
-            editedAt?.let { context.getString(R.string.description_post_edited) } ?: "",
+        // account.name
-            getReblogDescription(context, viewData),
+        // (; contentWarning)
-            viewData.username,
+        // ; (content)
-            if (reblogged) context.getString(R.string.description_post_reblogged) else "",
+        // (, poll)
-            if (favourited) context.getString(R.string.description_post_favourited) else "",
+        // , relativeDate
-            if (bookmarked) context.getString(R.string.description_post_bookmarked) else "",
+        // (, editedAt)
-            getMediaDescription(context, viewData),
+        // (, reblogDescription)
-            visibility.description(context),
+        // , username
-            getFavsText(favouritesCount),
+        // (, "Reblogged")
-            getReblogsText(reblogsCount),
+        // (, "Favourited")
+        // (, "Bookmarked")
+        // (, "n Favorite")
+        // (, "n Boost")
+        val description = StringBuilder().apply {
+            append(account.name)
+
+            getContentWarningDescription(context, viewData)?.let { append("; ", it) }
+
+            append("; ")
+
+            // Content is optional, and hidden if there are spoilers or the status is
+            // marked sensitive, and it has not been expanded.
+            if (TextUtils.isEmpty(viewData.spoilerText) || !sensitive || viewData.isExpanded) {
+                append(viewData.content, ", ")
+            }
+
             viewData.actionable.poll?.let {
+                append(
                     pollView.getPollDescription(
                         from(it),
                         statusDisplayOptions,
                         numberFormat,
                         absoluteTimeFormatter,
+                    ),
+                    ", ",
                 )
-            } ?: "",
+            }
-        )
+
+            append(getCreatedAtDescription(createdAt, statusDisplayOptions))
+
+            editedAt?.let { append(", ", context.getString(R.string.description_post_edited)) }
+
+            getReblogDescription(context, viewData)?.let { append(", ", it) }
+
+            append(", ", viewData.username)
+
+            if (reblogged) {
+                append(", ", context.getString(R.string.description_post_reblogged))
+            }
+
+            if (favourited) {
+                append(", ", context.getString(R.string.description_post_favourited))
+            }
+            if (bookmarked) {
+                append(", ", context.getString(R.string.description_post_bookmarked))
+            }
+            getMediaDescription(context, viewData)?.let { append(", ", it) }
+
+            append("; ")
+
+            visibility.description(context)?.let { append(it) }
+            getFavouritesCountDescription(favouritesCount)?.let { append(", ", it) }
+            getReblogsCountDescription(reblogsCount)?.let { append(", ", it) }
+        }
+
         itemView.contentDescription = description
     }
 
-    protected fun getFavsText(count: Int): CharSequence {
+    /** @return "n Favourite", for use in a content description. */
-        if (count <= 0) return ""
+    protected fun getFavouritesCountDescription(count: Int): CharSequence? {
+        if (count <= 0) return null
 
         val countString = numberFormat.format(count.toLong())
         return HtmlCompat.fromHtml(
@@ -802,8 +847,9 @@ abstract class StatusBaseViewHolder<T : IStatusViewData> protected constructor(i
         )
     }
 
-    protected fun getReblogsText(count: Int): CharSequence {
+    /** @return "n Boost", for use in a content description. */
-        if (count <= 0) return ""
+    protected fun getReblogsCountDescription(count: Int): CharSequence? {
+        if (count <= 0) return null
 
         val countString = numberFormat.format(count.toLong())
         return HtmlCompat.fromHtml(
@@ -826,7 +872,10 @@ abstract class StatusBaseViewHolder<T : IStatusViewData> protected constructor(i
         cardView ?: return
 
         val (_, _, _, _, _, _, _, _, _, _, _, _, _, _, _, _, sensitive, _, _, attachments, _, _, _, _, _, poll, card) = viewData.actionable
-        if (cardViewMode !== CardViewMode.NONE && attachments.isEmpty() && poll == null && card != null &&
+        if (cardViewMode !== CardViewMode.NONE &&
+            attachments.isEmpty() &&
+            poll == null &&
+            card != null &&
             !TextUtils.isEmpty(card.url) &&
             (!sensitive || expanded) &&
             (!viewData.isCollapsible || !viewData.isCollapsed)
@@ -883,34 +932,32 @@ abstract class StatusBaseViewHolder<T : IStatusViewData> protected constructor(i
             return attachments.all { it.isPreviewable() }
         }
 
-        private fun getReblogDescription(context: Context, status: IStatusViewData): CharSequence {
+        /** @return "{account.username} boosted", for use in a content description. */
+        private fun getReblogDescription(context: Context, status: IStatusViewData): String? {
             return status.rebloggingStatus?.let {
                 context.getString(R.string.post_boosted_format, it.account.username)
-            } ?: ""
+            }
         }
 
-        private fun getMediaDescription(context: Context, status: IStatusViewData): CharSequence {
+        /** @return "Media: {0-n descriptions}", for use in a content description. */
-            if (status.actionable.attachments.isEmpty()) return ""
+        private fun getMediaDescription(context: Context, status: IStatusViewData): String? {
+            if (status.actionable.attachments.isEmpty()) return null
 
-            val mediaDescriptions =
+            val missingDescription = context.getString(R.string.description_post_media_no_description_placeholder)
-                status.actionable.attachments.fold(StringBuilder()) { builder: StringBuilder, (_, _, _, _, _, description): Attachment ->
+
-                    if (description == null) {
+            val mediaDescriptions = status.actionable.attachments.map {
-                        val placeholder =
+                it.description ?: missingDescription
-                            context.getString(R.string.description_post_media_no_description_placeholder)
-                        return@fold builder.append(placeholder)
-                    } else {
-                        builder.append("; ")
-                        return@fold builder.append(description)
-                    }
-                }
-            return context.getString(R.string.description_post_media, mediaDescriptions)
             }
 
-        private fun getContentWarningDescription(context: Context, status: IStatusViewData): CharSequence {
+            return context.getString(R.string.description_post_media, mediaDescriptions.joinToString(", "))
+        }
+
+        /** @return "Content warning: {spoilerText}", for use in a content description. */
+        private fun getContentWarningDescription(context: Context, status: IStatusViewData): String? {
             return if (!TextUtils.isEmpty(status.spoilerText)) {
                 context.getString(R.string.description_post_cw, status.spoilerText)
             } else {
-                ""
+                null
             }
         }
     }

app/src/main/java/app/pachli/adapter/StatusDetailedViewHolder.kt

@@ -38,7 +38,7 @@ class StatusDetailedViewHolder(
         visibilityIcon?.also {
             val alignment = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) DynamicDrawableSpan.ALIGN_CENTER else DynamicDrawableSpan.ALIGN_BASELINE
             val visibilityIconSpan = ImageSpan(it, alignment)
-            sb.setSpan(visibilityIconSpan, 0, visibilityString.length, Spanned.SPAN_EXCLUSIVE_EXCLUSIVE)
+            sb.setSpan(visibilityIconSpan, 0, visibilityString?.length ?: 0, Spanned.SPAN_EXCLUSIVE_EXCLUSIVE)
         }
         val metadataJoiner = context.getString(R.string.metadata_joiner)
         sb.append(" ")
@@ -82,13 +82,13 @@ class StatusDetailedViewHolder(
         listener: StatusActionListener<StatusViewData>,
     ) {
         if (reblogCount > 0) {
-            binding.statusReblogs.text = getReblogsText(reblogCount)
+            binding.statusReblogs.text = getReblogsCountDescription(reblogCount)
             binding.statusReblogs.show()
         } else {
             binding.statusReblogs.hide()
         }
         if (favCount > 0) {
-            binding.statusFavourites.text = getFavsText(favCount)
+            binding.statusFavourites.text = getFavouritesCountDescription(favCount)
             binding.statusFavourites.show()
         } else {
             binding.statusFavourites.hide()

app/src/main/java/app/pachli/util/StatusExtensions.kt

@@ -30,17 +30,17 @@ import app.pachli.core.network.model.Status
 // store resources (yet).
 
 /**
- * @return A description for this visibility, or "" if it's null or [Status.Visibility.UNKNOWN].
+ * @return A description for this visibility, or null if it's null or [Status.Visibility.UNKNOWN].
  */
-fun Status.Visibility?.description(context: Context): CharSequence {
+fun Status.Visibility?.description(context: Context): CharSequence? {
-    this ?: return ""
+    this ?: return null
 
     val resource: Int = when (this) {
         Status.Visibility.PUBLIC -> R.string.description_visibility_public
         Status.Visibility.UNLISTED -> R.string.description_visibility_unlisted
         Status.Visibility.PRIVATE -> R.string.description_visibility_private
         Status.Visibility.DIRECT -> R.string.description_visibility_direct
-        Status.Visibility.UNKNOWN -> return ""
+        Status.Visibility.UNKNOWN -> return null
     }
     return context.getString(resource)
 }

app/src/main/res/values/donottranslate.xml

@@ -192,11 +192,6 @@
         <item>never</item>
     </string-array>
 
-    <string name="description_status" translatable="false">
-        <!-- Display name, cw?, content?, poll? relative date, edited?, reposted by?, reposted?, favorited?, bookmarked?, username, media?; visibility, fav number?, reblog number?-->
-        %1$s; %2$s; %3$s, %15$s %4$s, %5$s, %6$s; %7$s, %8$s, %9$s, %10$s, %11$s; %12$s, %13$s, %14$s
-    </string>
-
     <string-array name="poll_duration_names">
         <item>@string/duration_5_min</item>
         <item>@string/duration_30_min</item>

core/network/src/main/kotlin/app/pachli/core/network/model/TimelineAccount.kt

@@ -40,6 +40,10 @@ data class TimelineAccount(
     val emojis: List<Emoji>? = emptyList(),
 ) {
 
+    /**
+     * The account's [displayName], falling back to [localUsername] if
+     * [displayName] is null or empty.
+     */
     @Suppress("DEPRECATION")
     val name: String
         get() = if (displayName.isNullOrEmpty()) {