https://kotlinlang.org logo
Join Slack
Powered by
anyone here at Droidcon NY? :slightly_smiling_face...
# confetti
j
anyone here at Droidcon NY? 🙂
b
I'm attending Ash Davis talk about tests.
j
Have you tried Confetti there....hopefully working ok
b
Until now I was not getting more conferences to switch to. The last one was android makers. Now I've tried again and droidcon is listed. Trying now.
j
I wonder if there's some issue with conference screen getting updated....or we not to change something re. how that update gets triggered
b
Not sure. Tried yesterday, logged out and back in, but the conf list wasn't updated.
Trying now I noticed that somehow at some point bookmarking a talk does not work.
XRecorder_14092023_142150.mp4
Close and reopen the app all talks appear bookmarked
j
I saw same thing once and was fine then after restarting.... @mbonnin I wonder if this is somehow caused by it being first request when instance isn't running (with it showing cached data before that)
though thought your earlier changes kept the instances running for now
m
Most likely the optimistic updates getting rolled back indeed.
I need to remove that code
Optimistic updates do not make a lot of sense. We need the local data to be the source of truth
b
I couldn't identify a way to reproduce it, but it happens very often.
When happens, I can't see the bookmark being bookmarked, but when navigating to talk details the bookmark appears marked. However, navigating back to the talks list and back to the talk details, the bookmark now is unmarked.
Here we can see that the talked that appears marked in the list screen, is unmarked in the detail screen.
y
Optimistic updates do not make a lot of sense. We need the local data to be the source of truth
Not sure I agree when you have multiple devices 🙂
m
Sorry I meant local data and retries in the background to sync to the server
➕ 1
In the current state it's quite confusing to have the bookmark be checked only to be reverted a few seconds later because a network call fails
Or we could make the server more reliable of course but that doesn't account for network connectivity issues. Being able to set bookmarks on unreliable networks is nice
j
Just taking look at this. Had we seen any particular pattern for how/when it occurred?
I'm on what should be good network and see it still occasionally.
hmm, suspecting now it's something to do with app coming back in to foreground again after a certain period and trying to set bookmark then.....bookmark does seem to be set but bookmark icon in session list isn't updated
The update is reflected in bookmarks view.....and think that might be related to it using 
collectAsStateWithLifecycle
@Arkadii Ivanov the updated bookmarks info should be coming back from 
SessionsComponent
(through following we have in 
SessionsRoute
).....do you know if that should be lifecycle aware?
Copy code
val uiState by component.uiState.subscribeAsState()
a
From my point of view, 
collectAsStateWithLifecycle
only makes sense when the app is minimized to background. When the screen is in the back stack, its Composable is disposed anyway. Additionally, it only makes sense on hot Flows/Observables. Decompose 
Value
is cold, so using ``collectAsStateWithLifecycle`` on it won't make any difference.
j
btw can reproduce this quite easily now.....just put in background for about 5 minutes, then bring back to foreground and try and set bookmark in session list
a
This looks unrelated to 
collectAsStateWithLifecycle
. I think this needs to be debugged with logs 🙂
j
Yeah, does seem like that's unrelated. I see for example 
repository.watchBookmarks
not being triggered in failing case (in 
SessionsComponent
)
a
Perhaps, you can speedup testing by minimzing the app and then running 
adb shell am kill <pkg>
, then reopen the app.
As far as I understand the issue happens after the process death in background
j
I think in this case it's not that the app gets killed
but I'll test in case
a
Wondering, what else could be related, since you have to wait for ~5 minutes in background. This looks exactly like the system kills the app.
j
actually, very confused now.....doesn't seem like app needs to go in to background for this to occur
a
Are there any specific steps to reproduce?
j
still seeing that putting in background and then after a few minutes bringing back to foreground and trying to bookmark or unbookmark
that seems to be pretty reliable way to reproduce
I'm logging 
onCompletion
for 
repository.watchBookmarks
and see that that being invoked.....perhaps a factor
a
So killing the app via ADB doesn't help with debugging?
j
yeah, don't seem to see issue if I do that
I put in to background, short while later I see following in logs
Copy code
11-12 17:58:30.262 28746 28994 I FA      : Application backgrounded at: timestamp_millis: 1699811908235
11-12 17:58:33.214 28746 28746 I System.out: JFOR, onCompletion
bring to foreground and then see issue
looks like maybe flow is being cancelled
yeah, updated logging
Copy code
11-12 18:04:10.756 29376 29376 I System.out: JFOR, onCompletion, it = kotlinx.coroutines.flow.internal.ChildCancelledException: Child of the scoped flow was cancelled
a
Where do you put that log?
j
Copy code
repository.watchBookmarks(conference, user?.uid, user, bookmarksData)
    .map {
        println("JFOR, watchBookmarks, it = ${it.data}, user = ${user?.uid}")
        responseData.copy(bookmarksResponse = it)
    }
    .onStart {
        println("JFOR, onStart")
        emit(responseData)
    }
    .onCompletion {
        println("JFOR, onCompletion, it = $it")
    },
in 
SessionsComponent
In the case where the issue happened without going in to background I think 
onCompletion
was somehow triggered that time as well......but it seems to be triggered consistently if app goes in to background
think maybe related to the 
stateIn
here....
Copy code
val uiState: StateFlow<SessionsUiState> =
        combineUiState()
            .combine(searchQuery) { uiState, search ->
                filterSessions(uiState, search)
            }.stateIn(coroutineScope, SharingStarted.WhileSubscribed(5000), SessionsUiState.Loading)
so more like 5s then the few minutes I mentioned before 🙂
a
I'm trying to wrap my head around
j
if I change 
WhileSubscribed
to use say 20s then that's how long after going to background that I see 
onCompletion
logged..
a
Yep, already discovered that
Let me think
I will submit a fix
👍 1
j
nice!
Not sure history of why 
Channel
was used there but seems like 
StateFlow
is cleaner approach all right
❤️ 1
I've pushed new version to play store (in internal test) with this fix....be great if folks could do quick sanity test and I'll promote to production soon hopefully then
❤️ 1
a
I have tried the following steps, works just fine. 1. Open session list 2. Minimize the app for 10 seconds 3. Activate the app and press one of the bookmark buttons Result: bookmark toggled
🎯 2
👍 3
y
That's a nice fix, works better, simpler and more idiomatic.
❤️ 1
➕ 1
m
Wow, good catch and awesome debugging 👏
❤️ 1
I'm pretty sure I'm guilty of the initial 
Channel
stuff back in the day. I somewhat assumed this was called only once, sorry
y
Possibly 3 more to fix?
image.png
👀 1
s
Was the issue perhaps that the vaue was dropped when we were in the background and nobody was listening in on the channel? If we use a channel with an unlimited buffer (
Channel(Channel.UNLIMITED)
), wouldn’t it do the same as the MutableStateFlow here? With the difference that it’d also not drop when more than 1 events comes in (which probably wouldn’t matter here anyway). And that 
StateFlow
just feels a bit out of place here no? Since we don’t really model state here, it’s just that we want the values to not be dropped. But I could be wrong, wydt? 👀
j
btw I'm still seeing some issues here with bookmarks getting out of sync
particularly between session list and bookmark view....not consistent and haven't seen particular pattern yet.....will take closer look at logs when I get a chance
👍 1
m
If we use a channel with an unlimited buffer (
Channel(Channel.UNLIMITED)
), wouldn’t it do the same as the MutableStateFlow here?
I think the main difference is that 
channel.consumeAsFlow()
can be collected only once while you can collect a MutableStateFlow several times.
s
Yeah but you can also do 
channel.receiveAsFlow()
which does not “consume” it, which again I think gives the same result
👍 1
a
When the session list screen is first started, we call refresh and the UI subscribes to the channel. When refreshing finishes, it sends the result to the channel, then flatMapLatest is called, which subscribes to bookmarks repository. When we minimize the app, the UI unsubscribes. Then when we restore the app, UI resubscribes, but nobody calls refresh - and so the channel is not emitting anything.
We could trigger refresh on every onStart, but it looks more like a workaround rather than a fix. Caching the result after the latest refresh (in MutableStateFlow) looks better to me.
j
ok, the issue I'm seeing now also seems to be related to 
repository.watchBookmarks
being cancelled but this is without app going in to background.....think it's somehow triggered by going between session list and bookmarks list
I see 
bookmarksComponent
has dependency on 
sessionsComponent
but not sure if that could be a factor
a
Perhaps we should fix other similar cases with Channel
j
I'm not sure if that's a factor in this case..... don't think any channels being used now for this scenario....but could be wrong
Curious if anyone else is seeing this issue as well?
a
Seems to work fine for me.
Bookmarked a session, opened bookmarks screen,v waited for 10 seconds, unbookmarked, opened session list screen again - bookmark removed.
j
I haven't found reliable pattern to reproduce yet
so, 
bookmarks
in 
BookmarksComponent
has dependency on 
SessionsComponent.uiState
....could there be any issue if 
SessionsComponent
has different lifecycle? Just thinking aloud right now 🙂
a
All looks good to me
j
hmm, I guess 
BookmarksComponent
creates it's own copy of 
SessionsComponent
had in my head that it was version tied to session list UI
a
BookmarksComponent
creates an instance of 
SessionsSimpleComponent
.
SessionsSimpleComponent
was extracted from 
SessionsComponent
so it can be reused.
I'm AFK until evening. I could check later if there will be some steps to reproduce.
👍 1
j
probably not related to this issue but just curious if anyone remembers why we have
Copy code
fun watchBookmarks(
        conference: String,
        uid: String?,
        tokenProvider: TokenProvider?,
        initialData: GetBookmarksQuery.Data?,
    ): Flow<ApolloResponse<GetBookmarksQuery.Data>> = flow {
        apolloClientCache.getClient(conference, uid).query(GetBookmarksQuery())
            .tokenProvider(tokenProvider)
            .watch(initialData)

        emitAll(values)
    }
instead of just
Copy code
fun watchBookmarks(
        conference: String,
        uid: String?,
        tokenProvider: TokenProvider?,
        initialData: GetBookmarksQuery.Data?,
    ): Flow<ApolloResponse<GetBookmarksQuery.Data>> = 
        apolloClientCache.getClient(conference, uid).query(GetBookmarksQuery())
            .tokenProvider(tokenProvider)
            .watch(initialData)
m
Probably historical? There might have been some processing in between at some point that was removed without simplifying the expression?
👍 1
j
again probably unrelated but wondering whether using 
bookmarkedSessionsQuery
(added I think for Wear client) might simplify logic we have for android client (for bookmarks view) as well (using a version of it with a 
watch
)
Looking at the issue I mentioned again.....seeing it pretty frequently but not specific steps yet. It seems related to use of 
repository.watchBookmarks
in both places (session list and bookmark list) given both use session component......with one being cancelled as you switch to the other. And then for whatever reason it not being triggered when you make some change in one and switch to the other.
a
Here are the steps to reproduce. 1. Open the app on session list screen and have some sessions bookmarked. 2. Force stop the app and reopen it on session list screen 3. Open bookmarks, open the session list again 4. Wait for 5+ seconds 5. Unbookmark a session 6. Open the bookmarks screen The screen is not updated, i.e. the session is still bookmarked
j
fwiw I don't seem to have to force stop to see issue
a
Maybe
But now it should be easy to track down the issue.
j
@mbonnin what's purpose of passing 
initialData
in to 
watch
here?
Copy code
fun watchBookmarks(
        conference: String,
        uid: String?,
        tokenProvider: TokenProvider?,
        initialData: GetBookmarksQuery.Data?,
    ): Flow<ApolloResponse<GetBookmarksQuery.Data>> = flow {
        val values = apolloClientCache.getClient(conference, uid).query(GetBookmarksQuery())
            .tokenProvider(tokenProvider)
            .watch(initialData)

        emitAll(values)
    }
maybe clutching at straws but wondering if that could be a factor as we switch back and forth between 2 views and call this again each time
a
Oh, I think I know what the actual cause. It looks like 
watchBookmarks
doesn't emit the current set after subscription. So when you switch to the tab that was opened before (e.g. open bookmarks, change something there, then go back to session list), it emits the previously cached state (which I fixed yesterday), and then subscribes to 
watchBookmarks
and just waits for updates (which may not come).
✅ 1
🎯 1
Let me upload a PR
👍 1
m
@mbonnin what's purpose of passing 
initialData
in to 
watch
here?
The initial set of keys to watch is retrieved from 
initialData
. You can also pass null but the watcher will query the cache for every cache change until the query succeeds.
j
looks good! new version published to play store (internal test)
❤️ 2
That's in production now
🔥 2
5 Views
Open in Slack
PreviousNext
Powered by