address more feedback, add customization to # of description lines

app/src/main/java/org/mozilla/fenix/compose/list/ListItem.kt

@@ -51,6 +51,7 @@ fun TextListItem(
     label: String,
     modifier: Modifier = Modifier,
     description: String? = null,
+    maxDescriptionLines: Int = 1,
     onClick: (() -> Unit)? = null,
     iconPainter: Painter? = null,
     iconDescription: String? = null,
@@ -60,6 +61,7 @@ fun TextListItem(
         label = label,
         modifier = modifier,
         description = description,
+        maxDescriptionLines,
         onClick = onClick,
         afterListAction = {
             iconPainter?.let {
@@ -181,6 +183,7 @@ private fun ListItem(
     label: String,
     modifier: Modifier = Modifier,
     description: String? = null,
+    maxDescriptionLines: Int = 1,
     onClick: (() -> Unit)? = null,
     beforeListAction: @Composable RowScope.() -> Unit = {},
     afterListAction: @Composable RowScope.() -> Unit = {},
@@ -211,7 +214,7 @@ private fun ListItem(
                 SecondaryText(
                     text = description,
                     fontSize = 14.sp,
-                    maxLines = 1,
+                    maxLines = maxDescriptionLines,
                 )
             }
         }

app/src/main/java/org/mozilla/fenix/settings/address/ext/Address.kt

@@ -8,38 +8,20 @@ import androidx.annotation.VisibleForTesting
 import mozilla.components.concept.storage.Address
 
 /**
- * Generate a label from a given [Address]. A well-formed [Address] will have
- * a full name in the label, otherwise it will find the highest priority [Address] property
- * to use, as based on the desktop code found in FormAutofillUtils::getAddressLabel
- * https://searchfox.org/mozilla-central/rev/87ecd21d3ca517f8d90e49b32bf042a754ed8f18/toolkit/components/formautofill/FormAutofillUtils.jsm#323
+ * Generate a label item text for an [Address]. The combination of names is based on desktop code
+ * found here:
+ * https://searchfox.org/mozilla-central/rev/d989c65584ded72c2de85cb40bede7ac2f176387/toolkit/components/formautofill/FormAutofillNameUtils.jsm#400
  */
-fun Address.toAddressLabel(): String = getFullName().ifEmpty { iterateFields("") }
-
-/**
- * Generate a description from a given [Address]. A well-formed address will have a street address
- * as a description. An address with missing data will use the next available property after [label]
- * based on the priorities found in the desktop code in FormAutofillUtils::getAddressLabel
- * https://searchfox.org/mozilla-central/rev/87ecd21d3ca517f8d90e49b32bf042a754ed8f18/toolkit/components/formautofill/FormAutofillUtils.jsm#323
- *
- * @param label: The label for an [Address]. See [toAddressLabel].
- */
-fun Address.toAddressDescription(label: String): String = iterateFields(label)
-
-@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
-internal fun Address.getFullName(): String = listOf(givenName, additionalName, familyName)
+fun Address.toAddressLabel(): String = listOf(givenName, additionalName, familyName)
     .filter { it.isNotEmpty() }
     .joinToString(" ")
 
 /**
- * This will iterate through the available data in an [Address] based on the priority defined
- * in the desktop code found in FormAutofillUtils::getAddressLabel.
- * https://searchfox.org/mozilla-central/rev/87ecd21d3ca517f8d90e49b32bf042a754ed8f18/toolkit/components/formautofill/FormAutofillUtils.jsm#323
- * If [previous] is not empty, this function will find the next well-formed property after
- * [previous].
- *
- * @param [previous] The last [Address] property that was used for another text item.
+ * Generate a description item text for an [Address]. The element ordering is based on the
+ * priorities defined by the desktop code found here:
+ * https://searchfox.org/mozilla-central/rev/d989c65584ded72c2de85cb40bede7ac2f176387/toolkit/components/formautofill/FormAutofillUtils.jsm#323
  */
-private fun Address.iterateFields(previous: String): String = listOf(
+fun Address.toAddressDescription(): String = listOf(
     streetAddress.toOneLineAddress(),
     addressLevel3,
     addressLevel2,
@@ -49,7 +31,7 @@ private fun Address.iterateFields(previous: String): String = listOf(
     postalCode,
     tel,
     email
-).filter { it.isNotEmpty() }.firstOrNull { it != previous } ?: ""
+).filter { it.isNotEmpty() }.joinToString(", ")
 
 @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
 internal fun String.toOneLineAddress(): String =

app/src/main/java/org/mozilla/fenix/settings/address/view/AddressList.kt

@@ -39,11 +39,11 @@ fun AddressList(
 ) {
     LazyColumn {
         items(addresses) { address ->
-            val label = address.toAddressLabel()
             TextListItem(
                 label = address.toAddressLabel(),
                 modifier = Modifier.padding(start = 56.dp),
-                description = address.toAddressDescription(label),
+                description = address.toAddressDescription(),
+                maxDescriptionLines = 2,
                 onClick = { onAddressClick(address) },
             )
         }

app/src/test/java/org/mozilla/fenix/settings/address/ext/AddressTest.kt

@@ -10,69 +10,71 @@ import org.junit.Test
 
 class AddressTest {
     @Test
-    fun `WHEN all fields are populated THEN name and street address are used`() {
+    fun `WHEN all names are populated THEN label includes all names`() {
         val addr = generateAddress()
 
         val label = addr.toAddressLabel()
-        val description = addr.toAddressDescription(label)
 
         assertEquals("${addr.givenName} ${addr.additionalName} ${addr.familyName}", label)
-        assertEquals(addr.streetAddress, description)
     }
 
     @Test
-    fun `WHEN middle name is missing THEN given and family are combined`() {
+    fun `WHEN middle name is missing THEN label is given and family combined`() {
         val addr = generateAddress(additionalName = "")
 
-        val name = addr.getFullName()
+        val label = addr.toAddressLabel()
 
-        assertEquals("${addr.givenName} ${addr.familyName}", name)
+        assertEquals("${addr.givenName} ${addr.familyName}", label)
     }
 
     @Test
-    fun `WHEN only family and middle name are available THEN middle and family are combined`() {
+    fun `WHEN only family and middle name are available THEN label is middle and family combined`() {
         val addr = generateAddress(givenName = "")
 
-        val name = addr.getFullName()
+        val label = addr.toAddressLabel()
 
-        assertEquals("${addr.additionalName} ${addr.familyName}", name)
+        assertEquals("${addr.additionalName} ${addr.familyName}", label)
     }
 
     @Test
-    fun `WHEN name is missing THEN street address and address level 3 are used`() {
-        val addr = generateAddress(givenName = "", additionalName = "", familyName = "")
+    fun `WHEN only family name is available THEN label is family name`() {
+        val addr = generateAddress(givenName = "", additionalName = "")
 
         val label = addr.toAddressLabel()
-        val description = addr.toAddressDescription(label)
 
-        assertEquals(addr.streetAddress, label)
-        assertEquals(addr.addressLevel3, description)
+        assertEquals(addr.familyName, label)
+    }
+
+    @Test
+    fun `WHEN all properties are present THEN all properties present in description`() {
+        val addr = generateAddress()
+
+        val description = addr.toAddressDescription()
+
+        val expected = "${addr.streetAddress}, ${addr.addressLevel3}, ${addr.addressLevel2}, " +
+            "${addr.organization}, ${addr.addressLevel1}, ${addr.country}, " +
+            "${addr.postalCode}, ${addr.tel}, ${addr.email}"
+
+        assertEquals(expected, description)
     }
 
     @Test
-    fun `WHEN everything but tel and email are missing THEN tel and email are used`() {
+    fun `WHEN any properties are missing THEN description includes only present`() {
         val addr = generateAddress(
-            givenName = "",
-            additionalName = "",
-            familyName = "",
-            organization = "",
-            streetAddress = "",
             addressLevel3 = "",
-            addressLevel2 = "",
-            addressLevel1 = "",
-            postalCode = "",
-            country = ""
+            organization = "",
+            email = "",
         )
 
-        val label = addr.toAddressLabel()
-        val description = addr.toAddressDescription(label)
+        val description = addr.toAddressDescription()
 
-        assertEquals(addr.tel, label)
-        assertEquals(addr.email, description)
+        val expected = "${addr.streetAddress}, ${addr.addressLevel2}, ${addr.addressLevel1}, " +
+                "${addr.country}, ${addr.postalCode}, ${addr.tel}"
+        assertEquals(expected, description)
     }
 
     @Test
-    fun `WHEN everything but email is missing THEN email is label and description is empty`() {
+    fun `WHEN everything is missing THEN description is empty`() {
         val addr = generateAddress(
             givenName = "",
             additionalName = "",
@@ -84,13 +86,12 @@ class AddressTest {
             addressLevel1 = "",
             postalCode = "",
             country = "",
-            tel = ""
+            tel = "",
+            email = ""
         )
 
-        val label = addr.toAddressLabel()
-        val description = addr.toAddressDescription(label)
+        val description = addr.toAddressDescription()
 
-        assertEquals(addr.email, label)
         assertEquals("", description)
     }