Improve camera tile add/remove event handling #3954
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Following reports by @dshokouhi in chats and on Discord of camera tiles resetting unexpectedly or not being removed from the app, I tested adding/removing camera tiles again.
For camera tiles resetting, this can be caused by
onTileAddEvent
oronTileRemoveEvent
. This PR updatesonTileAddEvent
to not overwrite tile data if it already exists, hopefully fixing this (remove+add is annoying, the app would have to add soft delete).For camera tiles sticking around, during testing I noticed that the system was often destroying the service almost immediately after the add/remove events were sent. As a result, code started in the service's coroutine was cancelled before completing, and a camera tile wasn't added to the database or not removed from the database. I could semi-reliably get it to destroy the service within several milliseconds of adding/removing.
![Logcat example showing 10ms between event and destroy](https://private-user-images.githubusercontent.com/8148535/276742463-082363ab-1b7d-4a9a-8899-db624ecaa563.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzkwOTE2ODQsIm5iZiI6MTczOTA5MTM4NCwicGF0aCI6Ii84MTQ4NTM1LzI3Njc0MjQ2My0wODIzNjNhYi0xYjdkLTRhOWEtODg5OS1kYjYyNGVjYWE1NjMucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI1MDIwOSUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNTAyMDlUMDg1NjI0WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9NjMyODQ0M2Y1ZTU4ODgzYTNiNGUyNTQ0YzhiNzI4MWFlMTA1YWI0MmM3MWQ1NDllNjIyMjg4MzZjMDY0N2I5MiZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QifQ.1HcjOl59lqU4rLnMYhsKy1sCDt9I9Q9MsCAy062l3VU)
To prevent this, making sure the database work is completed before completing the callback function seems to be the only reliable way (
runBlocking
). This PR changes it for the camera tiles, during testing it still finishes in <100ms and the app isn't visible at the time (system plays animation) so blocking isn't an issue.Screenshots
n/a
Link to pull request in Documentation repository
n/a
Any other notes
If accepted I intend to update other tile services to do the same later (there haven't been any complaints yet, but they use other storage which may behave differently on cancellation).