From 5b3677aa83ffb8074eeb711e9147cfc4130a93fa Mon Sep 17 00:00:00 2001 From: Mugurell Date: Mon, 12 Apr 2021 23:36:00 +0300 Subject: [PATCH] Cleanup - Don't count item padding in ExpandableLayout ExpandableLayout needs to iterate through all items until the one set as the limit of the collapsible menu and accumulate the distance. It adds the items height and margins but also erroneously the padding. Stop counting the padding if height is counted. Height contains any vertical padding set. --- .../browser/menu/view/ExpandableLayout.kt | 6 ++---- .../browser/menu/view/ExpandableLayoutTest.kt | 16 ++++++++-------- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/components/browser/menu/src/main/java/mozilla/components/browser/menu/view/ExpandableLayout.kt b/components/browser/menu/src/main/java/mozilla/components/browser/menu/view/ExpandableLayout.kt index 6fdcbfb3ec5..2cc8ca8fbd5 100644 --- a/components/browser/menu/src/main/java/mozilla/components/browser/menu/view/ExpandableLayout.kt +++ b/components/browser/menu/src/main/java/mozilla/components/browser/menu/view/ExpandableLayout.kt @@ -309,6 +309,7 @@ internal class ExpandableLayout private constructor(context: Context) : FrameLay // That distance will be the collapsed height of the ViewGroup used when this will be first shown on the screen. // Users will be able to afterwards expand the ViewGroup to the full height. @VisibleForTesting + @Suppress("ReturnCount") internal fun calculateCollapsedHeight(): Int { val listView = (wrappedView.getChildAt(0) as ViewGroup) // Simple sanity check @@ -328,17 +329,14 @@ internal class ExpandableLayout private constructor(context: Context) : FrameLay result += listView.paddingTop result += listView.paddingBottom - run loop@ { + run loop@{ listView.children.forEachIndexed { index, view -> if (index < lastVisibleItemIndexWhenCollapsed) { result += view.marginTop result += view.marginBottom - result += view.paddingTop - result += view.paddingBottom result += view.measuredHeight } else if (index == lastVisibleItemIndexWhenCollapsed) { result += view.marginTop - result += view.paddingTop // Edgecase: if the same item is the sticky footer and the lastVisibleItemIndexWhenCollapsed // the menu will be collapsed to this item but shown with full height. diff --git a/components/browser/menu/src/test/java/mozilla/components/browser/menu/view/ExpandableLayoutTest.kt b/components/browser/menu/src/test/java/mozilla/components/browser/menu/view/ExpandableLayoutTest.kt index c7f4d64069c..b534b6fe728 100644 --- a/components/browser/menu/src/test/java/mozilla/components/browser/menu/view/ExpandableLayoutTest.kt +++ b/components/browser/menu/src/test/java/mozilla/components/browser/menu/view/ExpandableLayoutTest.kt @@ -589,8 +589,8 @@ class ExpandableLayoutTest { var expected = 0 expected += viewHeightForEachProperty * 4 // marginTop + marginBottom + paddingTop + paddingBottom expected += listHeightForEachProperty * 4 // marginTop + marginBottom + paddingTop + paddingBottom - expected += itemHeightForEachProperty * 5 // height + marginTop + marginBottom + paddingTop + paddingBottom for the top item shown in entirety - expected += itemHeightForEachProperty * 2 // marginTop + paddingTop for the special view + expected += itemHeightForEachProperty * 3 // height + marginTop + marginBottom for the top item shown in entirety + expected += itemHeightForEachProperty // marginTop for the special view expected += itemHeightForEachProperty / 2 // as per the specs, show only half of the special view assertEquals(expected, result) } @@ -617,10 +617,10 @@ class ExpandableLayoutTest { var expected = 0 expected += viewHeightForEachProperty * 4 // marginTop + marginBottom + paddingTop + paddingBottom expected += listHeightForEachProperty * 4 // marginTop + marginBottom + paddingTop + paddingBottom - expected += itemHeightForEachProperty * 5 // height + marginTop + marginBottom + paddingTop + paddingBottom for the top item shown in entirety - expected += itemHeightForEachProperty * 2 // marginTop + paddingTop for the special view + expected += itemHeightForEachProperty * 3 // height + marginTop + marginBottom for the top item shown in entirety + expected += itemHeightForEachProperty // marginTop for the special view expected += itemHeightForEachProperty / 2 // as per the specs, show only half of the special view - expected += itemHeightForEachProperty // height of the sticky item + expected += itemHeightForEachProperty // height of the sticky item assertEquals(expected, result) } @@ -646,9 +646,9 @@ class ExpandableLayoutTest { var expected = 0 expected += viewHeightForEachProperty * 4 // marginTop + marginBottom + paddingTop + paddingBottom expected += listHeightForEachProperty * 4 // marginTop + marginBottom + paddingTop + paddingBottom - expected += itemHeightForEachProperty * 5 // height + marginTop + marginBottom + paddingTop + paddingBottom for the top item shown in entirety - expected += itemHeightForEachProperty * 2 // marginTop + paddingTop for the special view - expected += itemHeightForEachProperty // height of the sticky item + expected += itemHeightForEachProperty * 3 // height + marginTop + marginBottom for the top item shown in entirety + expected += itemHeightForEachProperty // marginTop for the special view + expected += itemHeightForEachProperty // height of the sticky item assertEquals(expected, result) } }