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

android automatic track management #23

Merged
merged 4 commits into from
Nov 11, 2021

Conversation

davidliu
Copy link
Contributor

No description provided.

@@ -1,5 +1,8 @@
package io.livekit.android.room.track

import io.livekit.android.events.BroadcastEventBus
import io.livekit.android.events.EventListenable

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this import used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like this one can be cleaned up, good eye.

Copy link

@boks1971 boks1971 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! A couple of small comments/questions.

val lastSize = size()
lastCoordinates = layoutCoordinates

if (lastVisible != isVisible() || lastSize != size()) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need some Kotlin education :-) or a new pair of glasses :-)

A couple of lines above these variables (lastVisible and lastSize) are set to return values from the functions checked against here. Under what conditions will those be not equal?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the underlying calculations of isVisible and size depend on the lastCoordinates, which gets updated in line 50. They'll be different if the new coordinates are different from the old coordinates.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah got it. Thank you

@davidliu davidliu merged commit 279fe4b into master Nov 11, 2021
@davidliu davidliu deleted the dl/lk-164-android-automatic-track-management branch November 11, 2021 10:17
Copy link
Member

@davidzhao davidzhao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great work!


override fun onGlobalLayout() {
handler.removeCallbacksAndMessages(null)
handler.postDelayed({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will this be debouncing the calls? or would it simply queue them up? we'd want a debounced behavior

also, when elements become visible, we should notify immediately to minimize delay of having no content showing. the delay should only be applied to when things are moving to invisible.

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

Successfully merging this pull request may close these issues.

3 participants