Hi team, I notice something weird in this source c...

# compose

[eneim]

Hi team, I notice something weird in this source code of Android: https://cs.android.com/androidx/platform/frameworks/support/+/androidx-main:compose/[…]pose/ui/platform/Wrapper.android.kt;l=62?q=Wrapper.android.kt (It may be more suitable to post in #C04TPPEQKEJ but I don't know who should this message is to. Perhaps @Ian Lake).

I wonder why it has to removeAllViews() when childCount is <= 0 in the else flow. In stead, should the removeAllViews() is called right before creating a new AndroidComposeView instance and in the same block:

// Suggestion:
val composeView = if (childCount > 0) { getChildAt(0) as? AndroidComposeView } else { null } ?: /* at this point we know the parent doesn't contain any AndroidComposeView, regardless of its childCount, so we have to ensure it is empty and then add a new AndroidComposeView to it */ run {
    removeAllViews()
    // Create a new AndroidComposeView and "also" add it to "this".
}

From the code, it seems that, if childCount > 0 but the first child is not an AndroidComposeView, the parent doesn't clear it before adding the newly created one.

Also, (AFAIK) even when childCount <= 0, calling removeAllViews() will cause a layout traversal so doing it seems to be unnecessary.

[shikasd]

removeAllViews is usually followed by addView which causes relayout anyways
It is added here in case someone tries to add a different type of view to the container (see as?, it might return null)

[eneim]

I see.

It is added here in case someone tries to add a different type of view to the container

Should it be added where childCount is not zero? If I understand it correctly, the current code does removeAllViews() when the parent is empty, which doesn't do anything meaningful.

[shikasd]

Maybe, I don't think that does anything really

[eneim]

On the other hand, if childCount > 0 and the first child is not an AndroidComposeView, the if/else block returns null (and so a new AndroidComposeView will be created and added) without any removeAllViews() call, which I think is a more critical situation.

[shikasd]

I think you are not reading this correctly, removeAllViews triggers based on the result of if/else

[shikasd]

Ah, actually I am not reading this correctly

[shikasd]

Nvm

[shikasd]

Yeah, idk really here, probably a bug

[eneim]

I can create a bug ticket if needed, but if you or someone else in the team could take care of it then it can save some time. (Unless keeping the reports in tickets are important for historical reason).

[shikasd]

I don't think it is really a bug, maybe documentation problem as it doesn't mention which cases it prevents (bugs in platform?)
But I vaguely remember questioning this part myself and being told that it is correct lol

[eneim]

I see. I think if you were told it is correct then it might be internal optimization for other system. But if we as the library user could break it by using other kind of public API, then it may be indeed a bug 😃 .

I will see if this observation can be exploited by client code (e.g., I manually add a buggy child View there and cause some issues, and see if the compiler or other internal system reports/handles/optimizes it, or if the client actually fails to work).

If that is the case I will create a ticket asking for help.