chore: demo app log listener cleanup#2650
Conversation
* Logs were being duplicated in logcat * remove extraneous logs
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — demo-app-only log cleanup; the inline note about duplicate logcat lines in the throwable overloads is a minor pre-existing nit, not blocking.
Extended reasoning...
Overview
This PR is scoped entirely to examples/demo/app/ — the OneSignal SDK demo app. It consolidates demo logging onto a single LogManager.TAG = "OneSignalDemo" constant, drops per-class TAG constants (OneSignalExample, OneSignalService, OneSignalRepository, OneSignalHMS, MainViewModel, TooltipHelper), removes redundant android.util.Log calls in MainViewModel, simplifies LogManager (drops (tag, …) overloads, removes legacy info/debug/warn/error aliases, introduces a UI-only appendToPanel() used by the SDK log listener to avoid double-logging), and updates callers to the simplified API. No production SDK code under OneSignal/ or OneSignalSDK/ is touched.
Security risks
None. There are no auth, crypto, permission, or input-handling changes; this is purely log-tag bookkeeping. The pre-existing demo OneSignalService still uses subscription IDs (not REST API keys) per its existing comments, and that path is unchanged here.
Level of scrutiny
Low. Demo/example code with no shipped-SDK surface, no public API change, and mechanical call-site updates. A human reviewer would essentially be skimming for typos.
Other factors
The bug hunting system flagged one pre-existing issue: LogManager.e(msg, throwable) / w(msg, throwable) still emit two logcat lines because they call Log.e/w(…, throwable) and then route through log(…) which writes again. The PR introduced appendToPanel() precisely to break this duplication on the SDK-listener path, so this is the exact category the PR targets; however, it predates this PR and only affects four demo call sites in OneSignalService.kt. The inline comment is sufficient — not blocking for approval given the demo-only blast radius.
| fun e(message: String, throwable: Throwable) { | ||
| Log.e(TAG, message, throwable) | ||
| log("$message: ${throwable.message}", LogLevel.ERROR) | ||
| } | ||
|
|
||
| fun w(tag: String, message: String, throwable: Throwable) { | ||
| Log.w(tag, message, throwable) | ||
| log(tag, "$message: ${throwable.message}", LogLevel.WARN) | ||
| fun w(message: String, throwable: Throwable) { | ||
| Log.w(TAG, message, throwable) | ||
| log("$message: ${throwable.message}", LogLevel.WARN) | ||
| } |
There was a problem hiding this comment.
🟣 The throwable overloads LogManager.e(message, throwable) and LogManager.w(message, throwable) at lines 75-83 still produce duplicate logcat lines: they call Log.e/Log.w(TAG, message, throwable) and then call log(...), which itself emits a second Log.e/Log.w(TAG, message). This is a pre-existing issue, but it's precisely the duplication category this PR targets — and appendToPanel() was introduced for exactly this case. Fix by replacing the log(...) call with appendToPanel(...) in both overloads. While here, the KDoc on log() at lines 26-28 is now stale on both counts: the tag parameter no longer exists, and the SDK log listener bypasses log() for appendToPanel().
Extended reasoning...
What the bug is
After this PR, LogManager.kt has two throwable overloads:
fun e(message: String, throwable: Throwable) {
Log.e(TAG, message, throwable) // logcat write #1 (with stack trace)
log("$message: ${throwable.message}", LogLevel.ERROR) // routes through log() → logcat write #2
}
fun w(message: String, throwable: Throwable) {
Log.w(TAG, message, throwable)
log("$message: ${throwable.message}", LogLevel.WARN)
}And log() itself does:
fun log(message: String, level: LogLevel) {
when (level) {
LogLevel.DEBUG -> Log.d(TAG, message)
LogLevel.INFO -> Log.i(TAG, message)
LogLevel.WARN -> Log.w(TAG, message)
LogLevel.ERROR -> Log.e(TAG, message)
}
appendToPanel(message, level)
}So every LogManager.e(msg, e) / LogManager.w(msg, w) call produces two logcat entries under the OneSignalDemo tag — one with the stack trace (good), and one with "$msg: ${throwable.message}" appended (the duplicate).
Step-by-step proof
Consider the call site OneSignalService.kt:101:
} catch (e: Exception) {
LogManager.e("Error sending notification", e)
...
}When e is, say, IOException("Connection refused"):
LogManager.e("Error sending notification", e)is invoked → matches the throwable overload at line 75.- Logcat line 1:
Log.e("OneSignalDemo", "Error sending notification", e)— writes "Error sending notification" plus the full stack trace. - Then
log("Error sending notification: Connection refused", LogLevel.ERROR)is called at line 77. - Inside
log(), theLogLevel.ERRORbranch fires: Logcat line 2:Log.e("OneSignalDemo", "Error sending notification: Connection refused"). - Then
appendToPanel(...)adds the message to the UI panel (correct).
Result in logcat:
E/OneSignalDemo: Error sending notification
java.io.IOException: Connection refused
at ...
at ...
E/OneSignalDemo: Error sending notification: Connection refused
Both lines are about the same error. The first has the full trace; the second is a redundant summary. This is exactly the "duplicate noisy logs" pattern the PR's description calls out as motivation.
Why existing code doesn't prevent it
The PR introduced appendToPanel() as the UI-only primitive precisely to break this duplication on the SDK-listener path (see MainApplication.kt:53, which the PR changed from LogManager.log(...) to LogManager.appendToPanel(...)). But the throwable overloads were not updated to follow the same pattern — they still route through log() for the UI-side append, which carries the unwanted second logcat write along with it.
Impact
Demo app only (no production impact). Affected callers all live in OneSignalService.kt:
- Line 101:
LogManager.e("Error sending notification", e) - Line 160:
LogManager.e("Error sending custom notification", e) - Line 209:
LogManager.e("Error parsing user response", e) - Line 218:
LogManager.e("Error fetching user", e)
Each one will emit two E/OneSignalDemo lines per exception. Noisy and exactly contradicts the PR's stated goal.
How to fix
Replace log(...) with appendToPanel(...) in both throwable overloads — logcat is already covered by the Log.e/Log.w call with the throwable above it:
fun e(message: String, throwable: Throwable) {
Log.e(TAG, message, throwable)
appendToPanel("$message: ${throwable.message}", LogLevel.ERROR)
}
fun w(message: String, throwable: Throwable) {
Log.w(TAG, message, throwable)
appendToPanel("$message: ${throwable.message}", LogLevel.WARN)
}While editing this file, also refresh the stale KDoc on log() at lines 26-28 ("Log with custom tag (used by SDK log listener)") — the tag parameter was removed in this same PR (TAG is hardcoded), and MainApplication.kt:53 was changed to call appendToPanel(...) directly, so the SDK listener no longer goes through log().
Severity
pre_existing — the dual-call pattern existed before this PR. Flagged because (a) the PR's explicit motivation is "Logs were being duplicated in logcat," (b) the PR added appendToPanel() as the exact primitive to fix this, and (c) the PR modified these very function signatures (dropping the tag parameter) so the fix is one keyword away.
fadi-george
left a comment
fadi-george
left a comment
There was a problem hiding this comment.
im working on cleaning the demo
📊 Diff Coverage Report✓ Coverage check passed (no source files changed) |
thanks, sounds good, the noisy logs were getting to me |
2 participants
Description
One Line Summary
Clean up duplicate noisy logs, and consolidate all demo logs into one tag so they are easy to find.
Details
Motivation
Scope
Only demo app
Testing
Unit testing
OPTIONAL - Explain unit tests added, if not clear in the code.
Manual testing
Run example app and see logs
Affected code checklist
Checklist
Overview
Testing
Final pass