[Slider] label is gone with slider

[manabu-nakamura]

closes #4319
I'm worried about performance:

  private boolean isVisible() {
    if (getVisibility() != VISIBLE) {
      return false;
    }
    for (ViewParent viewParent = getParent(); viewParent instanceof View; viewParent = viewParent.getParent()) {
      if (((View) viewParent).getVisibility() != VISIBLE) {
        return false;
      }
    }
    return true;
  }

[dsn5ft]

Thanks @manabu-nakamura, can you explain what you mean by your comment about performance? What is that code snippet from?

[manabu-nakamura]

The isVisible() method is an alternative to the isVisible field (and the onVisibilityAggregated() method). These are what I wrote. The isVisible field (and the onVisibilityAggregated() method) is more lightweight than the isVisible() method, I think.

[dsn5ft]

@manabu-nakamura just wondering about the default value being true here. if the Slider visibility is set to gone or invisible initially (e.g. in the xml layout), will onVisibilityAggregated() be called with false immediately after the view is inflated / created?

if not then maybe in the constructor we could set isVisible = getVisibility() == View.VISIBLE?

[dsn5ft]

I just checked locally and onVisibilityAggregated doesn't seem to get called when the Slider itself has android:visibility="invisible" or android:visibility="gone" in the XML layout.

So the isVisible boolean would be incorrect in those cases, which makes me think we should not initialize isVisible to true and instead do isVisible = getVisibility() == View.VISIBLE in the constructor.

Although that doesn't cover the case where the Slider is visible but the parent view is gone. because in that case getVisibility() == View.VISIBLE would return true in the Slider constructor and onVisibilityAggregated doesn't get called after the initial inflation to correct it.

[manabu-nakamura]

Thank you very much. Following pubiqq's advice, I replaced the isVisible field with the isShown() method. I tried displaying labels in the SliderScrollContainerDemoFragment demo. There seems to be no problem.
https://github.com/material-components/material-components-android/blob/master/catalog/java/io/material/catalog/slider/SliderScrollContainerDemoFragment.java

slider.setLabelBehavior(LabelFormatter.LABEL_VISIBLE);

[pubiqq]

It seems I was invited here by mistake, nevertheless I have something to say:

• You cannot use View#onVisibilityAggregated(boolean) as it's only available from API 24.
• You don't need to implement the visibility check yourself, use View#isShown().

[manabu-nakamura]

Thank you for your great advice!

[dsn5ft]

@manabu-nakamura @pubiqq View#isShown basically does the same thing as the code snippet in the PR description:

• https://cs.android.com/android/platform/superproject/main/+/main:frameworks/base/core/java/android/view/View.java;l=12361-12373;drc=843a30bb3461775a25ae2cf62762c75c50e3b5f7

so I think the performance concern is still relevant here - since isSliderVisibleOnScreen() / updateLabels() is called on every scroll + global layout change:

material-components-android/lib/java/com/google/android/material/slider/BaseSlider.java

Lines 353 to 358 in 8eae09d

	@NonNull
	private final ViewTreeObserver.OnScrollChangedListener onScrollChangedListener =
	this::updateLabels;
	@NonNull
	private final ViewTreeObserver.OnGlobalLayoutListener onGlobalLayoutListener =
	this::updateLabels;

meaning it would walk up the full view hierarchy on every scroll + global layout change.

[pubiqq]

The performance loss is negligible (few viewFlags & VISIBILITY_MASK != VISIBLE checks), I don't see any reason to worry about that.

[manabu-nakamura]

I don't think there seem to be any performance issues, but I tried rewriting the program.

[dsn5ft]

@manabu-nakamura did you test this? I don't think isShown() will really work here since when the view class / object is initially constructed, it is not added to the hierarchy yet. I think the field should be left uninitialized and then you can do isVisible = isShown(); in onAttachedToWindow()?

[dsn5ft]

actually I'm thinking we can do isVisible = isShown() in the constructor after the super() call (that way the super class has at least processed the initial setVisibility()), and then also isVisible = isShown() in onAttachedToWindow() to account for the parent hierarchy.

[manabu-nakamura]

I was able to run Test8 without any problems in the following cases:

• BottomAppBar
  • android:visibility="visible"
  • android:visibility="invisible"
  • android:visibility="gone"

https://docs.oracle.com/javase/specs/jls/se23/html/jls-12.html#jls-12.5

[manabu-nakamura]

I modified the initialization of isVisible.

[dsn5ft]

BottomAppBar? The case I'm referring to is when the parent of the Slider is invisible/gone, isShown() won't work correctly in the constructor because the parent hierarchy is not assigned yet:

https://stackoverflow.com/questions/7099249/using-view-getparent-in-views-constructor

I'll add it to onAttachedToWindow() when importing the PR.

[manabu-nakamura]

Thank you very much. I see.

I was able to run Test8 without any problems in the following cases:

  <com.google.android.material.bottomappbar.BottomAppBar
    android:id="@+id/appBar"
    android:layout_width="match_parent"
    android:layout_height="wrap_content"
    android:layout_gravity="bottom"
    android:fitsSystemWindows="true"
    android:visibility="visible">				<=
    <com.google.android.material.slider.Slider
      android:id="@+id/slider"
      android:layout_width="match_parent"
      android:layout_height="wrap_content"
      app:labelBehavior="visible" />
  </com.google.android.material.bottomappbar.BottomAppBar>

  <com.google.android.material.bottomappbar.BottomAppBar
    android:id="@+id/appBar"
    android:layout_width="match_parent"
    android:layout_height="wrap_content"
    android:layout_gravity="bottom"
    android:fitsSystemWindows="true"
    android:visibility="invisible">				<=
    <com.google.android.material.slider.Slider
      android:id="@+id/slider"
      android:layout_width="match_parent"
      android:layout_height="wrap_content"
      app:labelBehavior="visible" />
  </com.google.android.material.bottomappbar.BottomAppBar>

  <com.google.android.material.bottomappbar.BottomAppBar
    android:id="@+id/appBar"
    android:layout_width="match_parent"
    android:layout_height="wrap_content"
    android:layout_gravity="bottom"
    android:fitsSystemWindows="true"
    android:visibility="gone">					<=
    <com.google.android.material.slider.Slider
      android:id="@+id/slider"
      android:layout_width="match_parent"
      android:layout_height="wrap_content"
      app:labelBehavior="visible" />
  </com.google.android.material.bottomappbar.BottomAppBar>