-
Notifications
You must be signed in to change notification settings - Fork 46
Debug logger #1667
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
Debug logger #1667
Conversation
| (it + DebugLogData( | ||
| message = message, | ||
| group = group, | ||
| date = date, | ||
| formattedDate = dateUtils.getLocaleFormattedTime(date.toEpochMilli()) | ||
| )).sortedByDescending { log -> log.date } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can use some optimization here, given we can potentially use this log a lot. A few things to consider:
- We might not want to create two new lists everytime you log an item
- We might want to limit the size of the memory log
I think it's probably better to use a mutable structure here. So you'd have to have setup like:
val logs = Deque<DebugLogData>() // The mutable data structure to store logs
val logNotification = MutableSharedFlow()
fun addLog(item: DebugLogData) {
synchronized(logs) {
logs.addLast(item)
while (logs.size > 100) {
logs.popFirst()
}
}
logNotification.tryEmit(Unit)
}
Then, on the UI side, you'll then listen to the logNotification, and you'll need to copy the logs to a list with a lock when displaying on the UI. This way we won't have a very high overhead when appending logs, and we will have a slower log viewer screen but that's ok it's a debug only facility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yep it definitely needs to be limited. I wanted to add that in and it slipped my mind as I implemented it.
|
I wonder if it's better to just tap into our existing |
| val logSnapshots: Flow<List<DebugLogData>> = | ||
| logChanges.onStart { emit(Unit) }.map { currentSnapshot() } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to do this:
| val logSnapshots: Flow<List<DebugLogData>> = | |
| logChanges.onStart { emit(Unit) }.map { currentSnapshot() } | |
| val logSnapshots: Flow<List<DebugLogData>> get() = | |
| logChanges.onStart { emit(Unit) }.map { currentSnapshot() } |
As the flow shouldn't be reused
| message = text, | ||
| group = group, | ||
| date = now, | ||
| formattedDate = dateUtils.getLocaleFormattedTime(now.toEpochMilli()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can just store the date, and let the UI do the formatting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SessionHero01 That's how I initially had it setup but our DateUtil class needs to be injected so can't be in a composable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't we pass DateUtils around in compose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could but is there an advantage? The VM can have the responsibility of formatting and manage the exact data the UI will need. Or do you think it's better done in Compose?
I think it might be cleaner to have that formatting in the VM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be in the VM - I just find it wasteful to have it computed but no one is using it here in the logger.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah right, you mean calculating it on every log while it might never be displayed. Ok I'll see if there's a clean way to pass it to the composable
| logChanges.tryEmit(Unit) | ||
|
|
||
| // Toast decision is independent from capture. | ||
| if (getGroupToastPreference(group)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getting a preference is not free (it can involve I/O), i wonder if we should cache it somewhere
| viewModel: DebugMenuViewModel, | ||
| onBack: () -> Unit, | ||
| ){ | ||
| val logs by viewModel.debugLogs.collectAsStateWithLifecycle(initialValue = emptyList()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this might have to become:
| val logs by viewModel.debugLogs.collectAsStateWithLifecycle(initialValue = emptyList()) | |
| val logs by remember { viewModel.debugLogs }.collectAsStateWithLifecycle(initialValue = emptyList()) |
Adds a new debug logger functionality.
It allows certain logs to be grouped into known categories, which can be set to pop up as toasts to be visible in situ in the app as it happens.
The new feature also keeps an in-memory cache of these logs which can be seen in a new screen.
The logs can be filtered, copied, and individually copied per log with a long press