Various Play insight fixes - #1367
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a set of defensive checks and parsing adjustments intended to reduce crashes and improve diagnostics around “lost” activity/DB states and Google Play insights, primarily by hardening null handling and numeric parsing.
Changes:
- Replace several direct
Double.parseDouble(...)calls withSafeParse.parseDouble(...)and expandSafeParse.parseDoublebehavior. - Add guards/logging around potentially-null runtime components (tracker, DB, Activity contexts, dialogs/spinners).
- Adjust workout step insertion and notification update behavior to be more resilient during runtime.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| app/src/main/org/runnerup/workout/WorkoutBuilder.java | Uses SafeParse for autolap parsing; adjusts step insertion logic for countdown/rest steps. |
| app/src/main/org/runnerup/workout/Step.java | Guards s.tracker.resume() against null and logs a warning. |
| app/src/main/org/runnerup/workout/RepeatStep.java | Adds bounds-check in getTime() to avoid out-of-range crashes. |
| app/src/main/org/runnerup/view/StartFragment.java | Avoids requireActivity() crash when requesting permissions from a detached fragment. |
| app/src/main/org/runnerup/view/ManualActivity.java | Uses SafeParse.parseDouble for manual distance entry. |
| app/src/main/org/runnerup/view/AudioCueSettingsFragment.java | Adds scheme-name sanitization and extra null-safety when opening prefs. |
| app/src/main/org/runnerup/util/SafeParse.java | Changes parseDouble behavior (including comma normalization). |
| app/src/main/org/runnerup/tracker/Tracker.java | Adds null checks/logging for DB usage and writer usage. |
| app/src/main/org/runnerup/notification/ForegroundNotificationDisplayStrategy.java | Avoids repeated startForeground by tracking foreground state and using NotificationManager.notify for updates. |
| app/src/main/org/runnerup/export/SyncManager.java | Adds null checks around auth activity start and spinner title updates. |
| app/src/main/org/runnerup/export/RunKeeperSynchronizer.java | Uses SafeParse for autolap preference parsing. |
| app/src/main/org/runnerup/export/format/RunKeeper.java | Uses SafeParse for distance parsing from JSON. |
| app/src/main/org/runnerup/db/PathSimplifier.java | Uses SafeParse for tolerance preference parsing with fallback defaults. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| String scheme = editText.getText().toString(); | ||
| if (!scheme.contentEquals("")) { | ||
| createNewAudioScheme(scheme); | ||
| updateSortOrder(scheme); | ||
| switchTo(scheme); | ||
| if (scheme.isEmpty() | ||
| || scheme.contains("/") | ||
| || scheme.contains("\\") | ||
| || scheme.contains("..")) { | ||
| return; | ||
| } |
As speech is asynch, a fragment may be detached when it occurs.
1342b1f to
3d3b31d
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (4)
app/src/main/org/runnerup/view/AudioCueSettingsFragment.java:73
- settingsName is now sanitized before being used as the scheme identifier, but other code in this fragment still treats the scheme name as the raw DB value (e.g. spinner selection uses adapter.find(settingsName) and DB operations use AUDIO_SCHEMES.NAME). For scheme names containing spaces/special chars, the sanitized settingsName will no longer match DB/adapter values, leading to wrong spinner selection and inability to reliably detect/avoid redundant switchTo() replacements.
This issue also appears on line 231 of the same file.
settingsName = sanitizeSettingsName(requireArguments().getString("name"));
if (settingsName != null) {
PreferenceManager prefMgr = getPreferenceManager();
prefMgr.setSharedPreferencesName(settingsName + SUFFIX);
app/src/main/org/runnerup/view/AudioCueSettingsFragment.java:233
- deleteAudioScheme() passes settingsName (sanitized) into deleteAudioSchemeImpl(), but the DB row is stored under the raw scheme name (createNewAudioScheme inserts the unsanitized value). This causes the DB delete to fail for schemes whose name is changed by sanitization (e.g. spaces -> underscores).
+ sanitizeSettingsName(name)
+ SUFFIX
+ ".xml");
app/src/main/org/runnerup/tracker/Tracker.java:606
- setNextLocationType() now returns early when mDBWriter is null, but it also skips updating mLocationType. That can leave the in-memory location type stale and can affect subsequent state-machine decisions/logging that rely on mLocationType.
if (mDBWriter == null) {
android.util.Log.w(
"Tracker", "setNextLocationType: mDBWriter is null (newType=" + newType + ")");
return;
}
app/src/main/org/runnerup/notification/ForegroundNotificationDisplayStrategy.java:43
- In the non-foreground branch, service.getSystemService(Context.NOTIFICATION_SERVICE) can return null; notificationManager.notify(...) would then throw an NPE. This would make notification updates crash instead of being a no-op/fallback.
} else {
android.app.NotificationManager notificationManager =
(android.app.NotificationManager) service.getSystemService(Context.NOTIFICATION_SERVICE);
notificationManager.notify(notificationId, notification);
}
| private void saveActivity(Double manualDistance) { | ||
| if (mDB == null) { | ||
| android.util.Log.e("Tracker", "saveActivity called but mDB is null"); | ||
| return; | ||
| } |
As speech is asynch, a fragment may be detached when it occurs.
3d3b31d to
a4dfe99
Compare
notify as normal in foreground.
If onDestroy gets called before saveActivity(), the workout cannot be saved. Avoid crash.
allow comm vs point If no previous default, assume 0 is OK defaults
No dialog to request permissions if the activity is gone
likely no steps for this workout
As speech is asynch, a fragment may be detached when it occurs.
Illegal characters causes exceptions
a4dfe99 to
5d261bb
Compare
5d261bb to
2e6b7ae
Compare
As speech is asynch, a fragment may be detached when it occurs.
Various Play insight fixes, mostly fixed with Gemini
I expect a few of them are not really addressing the core issue with activity or db "lost", but they could give better insight at next occurrence.
Could test only a few of them, before and after the fixes, most are not reproducible at all.