zawadz88 / MaterialPopupMenu

Shows Material popup menus grouped in sections & more
Apache License 2.0
646 stars 57 forks source link

Height calculation works incorrectly when items have dynamic size #49

Closed Tunous closed 5 years ago

Tunous commented 5 years ago

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 ```patch 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 @@ + + + app:srcCompat="@mipmap/ic_launcher" /> ```
zawadz88 commented 5 years ago

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 commented 5 years ago

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 ```patch 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 @@ + + + app:srcCompat="@mipmap/ic_launcher" /> ```
Tunous commented 5 years ago

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.