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.

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

Height calculation works incorrectly when items have dynamic size #49

Closed
Tunous opened this issue Apr 15, 2019 · 3 comments · Fixed by #51
Closed

Height calculation works incorrectly when items have dynamic size #49

Tunous opened this issue Apr 15, 2019 · 3 comments · Fixed by #51
Assignees
Labels

Comments

@Tunous
Copy link
Contributor

Tunous commented Apr 15, 2019

I don't have ideas how to fix that currently but calculation of height for the popup appears to work incorrectly when items can have dynamic height.

This can be easily reproduced with item that has long text applied in the viewBoundCallback block like I've done with below code:

Sample
diff --git a/sample/src/main/java/com/github/zawadz88/materialpopupmenu/sample/LightActivity.kt b/sample/src/main/java/com/github/zawadz88/materialpopupmenu/sample/LightActivity.kt
index c1823d606dbf7d2d1d2a4cbff025bce15ff756d2..e6e5652772bc62f1e91584cb7321141d0d0a86d6 100644
--- a/sample/src/main/java/com/github/zawadz88/materialpopupmenu/sample/LightActivity.kt
+++ b/sample/src/main/java/com/github/zawadz88/materialpopupmenu/sample/LightActivity.kt
@@ -321,6 +321,10 @@ class LightActivity : AppCompatActivity() {
                 }
                 customItem {
                     layoutResId = R.layout.view_custom_item_large
+                    viewBoundCallback = { view ->
+                        val textView: TextView = view.findViewById(R.id.customItemTextView)
+                        textView.text = "Some long text that is applied later to see if height calculation indeed is incorrectly calculated due to this binding."
+                    }
                 }
             }
         }
diff --git a/sample/src/main/res/layout/view_custom_item_large.xml b/sample/src/main/res/layout/view_custom_item_large.xml
index 7223c7b5b7d2087b3dc02972de35a5686403ce01..4486a545636d298fad9ecf39ce87f1ede481eb34 100644
--- a/sample/src/main/res/layout/view_custom_item_large.xml
+++ b/sample/src/main/res/layout/view_custom_item_large.xml
@@ -1,24 +1,30 @@
 <?xml version="1.0" encoding="utf-8"?>
 <LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
-    xmlns:tools="http://schemas.android.com/tools"
     xmlns:app="http://schemas.android.com/apk/res-auto"
+    xmlns:tools="http://schemas.android.com/tools"
     style="@style/Widget.MPM.Item"
     android:layout_width="match_parent"
-    android:layout_height="96dp"
+    android:layout_height="wrap_content"
     android:clickable="true"
     android:focusable="true"
     android:gravity="center"
-    android:orientation="horizontal"
-    android:paddingEnd="@dimen/mpm_popup_menu_item_padding_horizontal"
+    android:orientation="vertical"
+    android:paddingStart="@dimen/mpm_popup_menu_item_padding_horizontal"
     android:paddingLeft="@dimen/mpm_popup_menu_item_padding_horizontal"
+    android:paddingEnd="@dimen/mpm_popup_menu_item_padding_horizontal"
     android:paddingRight="@dimen/mpm_popup_menu_item_padding_horizontal"
-    android:paddingStart="@dimen/mpm_popup_menu_item_padding_horizontal"
-    tools:ignore="UseCompoundDrawables"
     tools:theme="@style/Widget.MPM.Menu">
 
+    <TextView
+        android:id="@+id/customItemTextView"
+        android:layout_width="match_parent"
+        android:layout_height="wrap_content"
+        android:gravity="center"
+        tools:text="Sample text" />
+
     <androidx.appcompat.widget.AppCompatImageView
         android:layout_width="wrap_content"
         android:layout_height="wrap_content"
-        app:srcCompat="@mipmap/ic_launcher"/>
+        app:srcCompat="@mipmap/ic_launcher" />
 
 </LinearLayout>
@zawadz88 zawadz88 added the bug label Apr 15, 2019
@zawadz88
Copy link
Owner

The might be some issues in MaterialRecyclerViewPopupWindow#measureHeightOfChildrenCompat. In theory it does invoke bindViewHolder, but I've never really tested it with e.g. wrap_content.
Most of that stuff was copied from DropDownListView (pre-AndroidX version). I see there were some changed in AndroidX so maybe applying them might fix the issue.
I'll have a look.

@zawadz88 zawadz88 self-assigned this Apr 15, 2019
@zawadz88
Copy link
Owner

I think I have a fix although I'll need to do more testing tomorrow (hopefully) to make sure this does not break something else.
This is how it looks like after the fix:

Screenshot_1555352947

Potential fix
diff --git a/material-popup-menu/src/main/java/androidx/appcompat/widget/MaterialRecyclerViewPopupWindow.kt b/material-popup-menu/src/main/java/androidx/appcompat/widget/MaterialRecyclerViewPopupWindow.kt
index e54f29b..37e651b 100644
--- a/material-popup-menu/src/main/java/androidx/appcompat/widget/MaterialRecyclerViewPopupWindow.kt
+++ b/material-popup-menu/src/main/java/androidx/appcompat/widget/MaterialRecyclerViewPopupWindow.kt
@@ -279,7 +279,7 @@ class MaterialRecyclerViewPopupWindow(
     private fun measureHeightOfChildrenCompat(maxHeight: Int): Int {
 
         val parent = FrameLayout(contextThemeWrapper)
-        val widthMeasureSpec = View.MeasureSpec.makeMeasureSpec(0, View.MeasureSpec.UNSPECIFIED)
+        val widthMeasureSpec = View.MeasureSpec.makeMeasureSpec(dropDownWidth, View.MeasureSpec.EXACTLY)
 
         // Include the padding of the list
         var returnedHeight = 0
@@ -310,6 +310,7 @@ class MaterialRecyclerViewPopupWindow(
                 View.MeasureSpec.makeMeasureSpec(0, View.MeasureSpec.UNSPECIFIED)
             }
             itemView.measure(widthMeasureSpec, heightMeasureSpec)
+            itemView.forceLayout()
 
             returnedHeight += itemView.measuredHeight
 
diff --git a/sample/src/main/java/com/github/zawadz88/materialpopupmenu/sample/LightActivity.kt b/sample/src/main/java/com/github/zawadz88/materialpopupmenu/sample/LightActivity.kt
index ce29895..bedbf87 100644
--- a/sample/src/main/java/com/github/zawadz88/materialpopupmenu/sample/LightActivity.kt
+++ b/sample/src/main/java/com/github/zawadz88/materialpopupmenu/sample/LightActivity.kt
@@ -13,6 +13,7 @@ import android.view.Menu
 import android.view.MenuItem
 import android.view.View
 import android.widget.CheckBox
+import android.widget.TextView
 import android.widget.Toast
 import butterknife.BindView
 import butterknife.ButterKnife
@@ -310,6 +311,10 @@ class LightActivity : AppCompatActivity() {
                 }
                 customItem {
                     layoutResId = R.layout.view_custom_item_large
+                    viewBoundCallback = { view ->
+                        val textView: TextView = view.findViewById(R.id.customItemTextView)
+                        textView.text = "Some long text that is applied later to see if height calculation indeed is incorrectly calculated due to this binding."
+                    }
                 }
             }
         }
diff --git a/sample/src/main/res/layout/view_custom_item_large.xml b/sample/src/main/res/layout/view_custom_item_large.xml
index 7223c7b..d4a0477 100644
--- a/sample/src/main/res/layout/view_custom_item_large.xml
+++ b/sample/src/main/res/layout/view_custom_item_large.xml
@@ -1,24 +1,31 @@
 <?xml version="1.0" encoding="utf-8"?>
 <LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
-    xmlns:tools="http://schemas.android.com/tools"
     xmlns:app="http://schemas.android.com/apk/res-auto"
+    xmlns:tools="http://schemas.android.com/tools"
     style="@style/Widget.MPM.Item"
     android:layout_width="match_parent"
-    android:layout_height="96dp"
+    android:layout_height="wrap_content"
     android:clickable="true"
     android:focusable="true"
     android:gravity="center"
-    android:orientation="horizontal"
-    android:paddingEnd="@dimen/mpm_popup_menu_item_padding_horizontal"
+    android:orientation="vertical"
+    android:paddingStart="@dimen/mpm_popup_menu_item_padding_horizontal"
     android:paddingLeft="@dimen/mpm_popup_menu_item_padding_horizontal"
+    android:paddingEnd="@dimen/mpm_popup_menu_item_padding_horizontal"
     android:paddingRight="@dimen/mpm_popup_menu_item_padding_horizontal"
-    android:paddingStart="@dimen/mpm_popup_menu_item_padding_horizontal"
     tools:ignore="UseCompoundDrawables"
     tools:theme="@style/Widget.MPM.Menu">
 
+    <TextView
+        android:id="@+id/customItemTextView"
+        android:layout_width="match_parent"
+        android:layout_height="wrap_content"
+        android:gravity="center"
+        tools:text="Sample text" />
+
     <androidx.appcompat.widget.AppCompatImageView
         android:layout_width="wrap_content"
         android:layout_height="wrap_content"
-        app:srcCompat="@mipmap/ic_launcher"/>
+        app:srcCompat="@mipmap/ic_launcher" />
 
 </LinearLayout>

zawadz88 pushed a commit that referenced this issue Apr 16, 2019
zawadz88 pushed a commit that referenced this issue Apr 16, 2019
@Tunous
Copy link
Contributor Author

Tunous commented Apr 16, 2019

Almost there. I just accidentally found that the height is also incorrect if you add vertical margin to custom items. It is calculated without taking it into account.

I can try to create a pull request for that tomorrow unless someone else will be quicker.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants