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

Understanding Visibility Changes, Layout, and maxOf/minOf #117

Open
luis-cortes opened this issue Jan 22, 2021 · 3 comments
Open

Understanding Visibility Changes, Layout, and maxOf/minOf #117

luis-cortes opened this issue Jan 22, 2021 · 3 comments

Comments

@luis-cortes
Copy link

luis-cortes commented Jan 22, 2021

Overview:

I'm trying to achieve an effect similar to #116, but with the maxOf function. Unfortunately for me, I'm a little bit confused by Contour's behavior when I change the visibility of one of the optional views.

Steps:

We can easily reproduce the behavior that's confusing me using the sample app.

  • Add private val card3 = ExpandableBioCard1(context) to SampleView:
  • Add this to SampleView's init block
card3.layoutBy(
 x = matchParentX(marginLeft = 24.dip, marginRight = 24.dip),
 y = maxOf(topTo { card2.bottom() + 24.ydip }, topTo { card1.bottom() + 24.ydip })
)
  • Add isVisible = false to ExpandableBioCard2's init block (in my real app this can happen in the render method, but it has the same effect here)
  • Notice that there is additional space in between card3 and card1

(Possibly Misguided) Expectations:

I expected card2's visibility change to GONE to result in card2.bottom() evaluating to 0, causing maxOf to pick the second argument.
I expected this for two reasons:

  • The docs for View.GONE state that the "view is invisible, and it doesn't take any space for layout purposes"
  • In a LinearLayout that contains two views, if you set the visibility of the first view to GONE the second view automatically takes its place

Of course, this is not what's happening. maxOf is always picking topTo { card2.bottom() + 24.ydip } because while card2.height()=0, card2.bottom() is still > card1.bottom().

Current (Possibly Permanent) Workaround:

I can get the desired behavior by doing this:

card3.layoutBy(
  x = matchParentX(marginLeft = 24.dip, marginRight = 24.dip),
  y = topTo {
    when {
       card2.isVisible -> card2.bottom() + 24.ydip
       else -> card1.bottom() + 24.ydip
    }
  }
)

Questions:

  • Is this working as intended? Why does card2.bottom() not evaluate to 0?
  • Is checking the visibility in topTo the recommended approach for achieving this effect?

Thank you all for this amazing library! I won't ever write another layout in XML if I can help it 😄 .

@luis-cortes
Copy link
Author

@carranca I did some more digging around found the view set to GONE does not get laid out and is considered to have position and size 0 test you wrote.

I've played around with this test and was surprised by what Contour is doing

Failed Test 1

@Test
  fun `failed test 1`() {
    val view = View(activity)

    val layout = contourLayout(activity) {
      view.layoutBy(
          leftTo { parent.centerX() },
          topTo { parent.centerY() }
      )
    }

    // Delaying the visibility change until this point causes `view` to act 
    // like its been set to `INVISIBLE` and _not_ `GONE` 
    view.visibility = View.GONE
    layout.forceRelayout()
    // Fails because view.left and view.right are both 100
    assertThat(view.left).isEqualTo(0)
    assertThat(view.right).isEqualTo(0)
    // Fails because view.top and view.bottom are both 25
    assertThat(view.top).isEqualTo(0)
    assertThat(view.bottom).isEqualTo(0)
    assertThat(view.width).isEqualTo(0)
    assertThat(view.height).isEqualTo(0)
  }        

Failed Test 2

I had to make some extra changes to support getting the value from bottom(), but I'm pretty sure this matches the behavior that I mentioned in the first comment.

class MyContourHelper(context: Context): ContourLayout(context) {
  val view = View(context).apply {
    visibility = GONE
  }

  val viewBottom: Int
    get() = view.bottom().value

  init {
    view.layoutBy(
      leftTo { parent.centerX() },
      topTo { parent.centerY() }
    )
    layoutSizeOf(200, 50)
  }
}
  @Test
  fun `failed test 2`() {

    val layout = MyContourHelper(activity)
    val view = layout.view
    
    // Fails because view.bottom() evaluates to 25
    assertThat(layout.viewBottom).isEqualTo(0)
    assertThat(view.left).isEqualTo(0)
    assertThat(view.right).isEqualTo(0)
    assertThat(view.top).isEqualTo(0)
    assertThat(view.bottom).isEqualTo(0)
    assertThat(view.width).isEqualTo(0)
    assertThat(view.height).isEqualTo(0)
  }

I can imagine reasons for view.bottom not being equal to view.bottom(), but having the view behave as INVISIBLE when it's set to GONE is very confusing.

Am I missing something or getting something wrong here?

cc @aerb @saket

@carranca
Copy link
Collaborator

Hmmm yeah the behaviour doesn't look right. I'm not sure if I agree with all your expectations though (happy to discuss and/or be corrected though), but I do agree that the behaviour isn't right:

I do agree that if a view is GONE, its bottom() and right() should not be equal to its top() + height() and left() + width(), respectively. I would expect that bottom() == top() (i.e. height = 0) and left() == right() (i.e. width = 0).

In your Failed Test 1 however, you're asserting that right and bottom should be 0, and I don't think that's correct. In this specific scenario I'd expect right to equal left and top to equal bottom (considering its position is centered on the parent), since those are the distance from the parent's right and bottom edges (respectively).

Does that make sense?

@luis-cortes
Copy link
Author

@carranca Thanks for getting back to me.

Failed Test 1 is basically identical to view set to GONE does not get laid out and is considered to have position and size 0() except that I delayed the visibility change until after the initial layout.

The original test made the same assertions on the view so I'm confused about why setting it to GONE later makes the assertions invalid. I was under the impression that setting a view to GONE resulted in the same behavior regardless of when the visibility change happens.

Is this incorrect or should the original test case be updated to reflect this?

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

No branches or pull requests

2 participants