Conversation
296a553 to
c92ef18
Compare
55202a8 to
5417516
Compare
# Conflicts: # app/src/main/java/org/blitzortung/android/alert/handler/AlertHandler.kt # app/src/main/java/org/blitzortung/android/app/WidgetProvider.kt
|
There was a problem hiding this comment.
Pull request overview
Adds an Android home-screen widget that renders the existing AlarmView in the background via WorkManager, alongside build/tooling upgrades to support the new setup.
Changes:
- Introduces
WidgetProvider+WidgetUpdateWorkerand wires them into the manifest / Dagger graph. - Adds widget resources (provider XML, layout, preview drawable) and small rendering tweaks for widget sizing.
- Updates Gradle/AGP + related build configuration and bumps app version.
Reviewed changes
Copilot reviewed 24 out of 25 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| gradle/wrapper/gradle-wrapper.properties | Updates Gradle wrapper distribution. |
| gradle.properties | Adjusts Android/AGP-related Gradle properties for the new toolchain. |
| build.gradle.kts | Updates top-level plugin versions and detekt/ktlint configuration. |
| app/build.gradle.kts | Updates app version, kapt configuration, and dependency versions. |
| app/src/main/AndroidManifest.xml | Enables the widget receiver in the manifest (receiver definition updated). |
| app/src/main/java/org/blitzortung/android/app/BOApplication.kt | Exposes the Dagger AppComponent instance for non-Activity entry points (worker/widget). |
| app/src/main/java/org/blitzortung/android/app/Main.kt | Moves StrikeColorHandler construction to DI. |
| app/src/main/java/org/blitzortung/android/app/WidgetProvider.kt | AppWidgetProvider now schedules immediate + periodic WorkManager updates. |
| app/src/main/java/org/blitzortung/android/app/WidgetUpdateWorker.kt | New worker that renders AlarmView to a bitmap and pushes RemoteViews updates. |
| app/src/main/java/org/blitzortung/android/alert/Warning.kt | Updates LocalActivity.toString() formatting and handles “no distance” case. |
| app/src/main/java/org/blitzortung/android/util/MeasurementSystem.kt | Adds unitSymbol used for widget/status text formatting. |
| app/src/test/java/org/blitzortung/android/alert/AlertResultTest.kt | Adds unit tests for LocalActivity.toString() output. |
| app/src/main/res/layout/widget.xml | Updates widget layout to include a status/update-time text line. |
| app/src/main/res/xml/widget_provider.xml | Updates widget provider metadata (preview, resize, update period). |
| app/src/main/res/drawable/widget_preview.xml | Adds a widget preview vector asset. |
| app/src/main/res/drawable/rounded_box.xml | Adjusts widget background styling (fill/corners/stroke). |
| app/src/main/res/values/strings.xml | Updates copyright year. |
| app/src/main/java/org/blitzortung/android/dagger/module/ActivityBindingModule.kt | Adds injector binding for WidgetProvider. |
| app/src/main/java/org/blitzortung/android/dagger/component/AppComponent.kt | Exposes dependencies needed by the widget worker via component methods. |
| app/src/main/java/org/blitzortung/android/map/overlay/color/StrikeColorHandler.kt | Makes StrikeColorHandler injectable (@Inject, @Singleton). |
| app/src/main/java/org/blitzortung/android/data/MainDataHandler.kt | Exposes dataProvider with a private setter. |
| app/src/main/java/org/blitzortung/android/app/view/support/CanvasProvider.kt | Adds dimension guard + adjusts CanvasWrapper properties. |
| app/src/main/java/org/blitzortung/android/app/view/alarm/LocalActivityRenderer.kt | Minor paint/text sizing adjustments used by the renderer. |
Comments suppressed due to low confidence (1)
app/src/main/java/org/blitzortung/android/app/view/support/CanvasProvider.kt:23
CanvasProvider.provide()caches the firstCanvasWrapperit creates and returns it for all subsequent calls, even if the requestedwidth/heightchange (e.g., after a view resize). This can lead to drawing into a bitmap with stale dimensions. Recreate the cachedCanvasWrapperwhenever the requested size differs from the existing one (and consider handling background color changes similarly if needed).
fun provide(backgroundColor: Int, width: Int, height: Int): CanvasWrapper? {
if (width <= 0 || height <= 0) {
return null
}
if (canvas == null) {
canvas = CanvasWrapper(width, height, backgroundColor)
}
return canvas
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 25 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 32 changed files in this pull request and generated 17 comments.
Comments suppressed due to low confidence (1)
app/src/main/java/org/blitzortung/android/app/view/support/CanvasProvider.kt:24
CanvasProvidercaches a singleCanvasWrapperbut never recreates it when the requestedwidth/heightchange (and it also ignores updatedbackgroundColor). This can render incorrectly after a resize (e.g., widget resize / view re-measure). Consider recreating the cachedCanvasWrapperwhen dimensions or background color differ from the cached instance.
fun provide(backgroundColor: Int, width: Int, height: Int): CanvasWrapper? {
if (width <= 0 || height <= 0) {
return null
}
if (canvas == null) {
canvas = CanvasWrapper(width, height, backgroundColor)
}
return canvas
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| val location = worker.testGetLastKnownLocation() | ||
|
|
||
| assertThat(location).isNotNull | ||
| assertThat(location!!.latitude).isEqualTo(51.0) | ||
| assertThat(location.longitude).isEqualTo(7.0) | ||
| } |
There was a problem hiding this comment.
assertThat(location).isNotNull is not invoking an assertion; in Kotlin this is just a method reference expression and won’t fail the test if location is null. Call the assertion method (or use a proper non-null assertion pattern) so the test actually verifies the behavior.
| fun workerClassIsOpen() { | ||
| val workerClass = WidgetUpdateWorker::class.java | ||
| val modifiers = workerClass.modifiers | ||
| val isPublic = java.lang.reflect.Modifier.isPublic(modifiers) | ||
| assertThat(isPublic).isTrue() | ||
| } |
There was a problem hiding this comment.
This test is named workerClassIsOpen but it only asserts the class is public. open in Kotlin corresponds to “not final”; if you want to test openness, assert !Modifier.isFinal(modifiers) (or rename the test to reflect what it checks).
| @@ -45,4 +45,41 @@ class AlertResultTest { | |||
| assertThat(uut.bearingName).isEqualTo("foo") | |||
| assertThat(uut.sectorWithClosestStrike).isNotNull | |||
There was a problem hiding this comment.
assertThat(uut.sectorWithClosestStrike).isNotNull does not execute an assertion (it’s a method reference expression in Kotlin), so this test won’t fail if the value is null. Invoke the assertion method (or use another explicit non-null assertion) so the test actually verifies the condition.
| assertThat(uut.sectorWithClosestStrike).isNotNull | |
| assertThat(uut.sectorWithClosestStrike).isNotNull() |
| <receiver | ||
| android:name=".WidgetProvider" | ||
| android:exported="true"> | ||
| <meta-data | ||
| android:name="android.appwidget.provider" | ||
| android:resource="@xml/widget_provider" /> | ||
| <intent-filter> | ||
| <action android:name="android.appwidget.action.APPWIDGET_UPDATE" /> | ||
| </intent-filter> | ||
| </receiver>--> | ||
| </receiver> |
There was a problem hiding this comment.
WidgetProvider is exported without restricting who can send APPWIDGET_UPDATE. Add android:permission="android.permission.BIND_APPWIDGET" on the receiver (common pattern for widgets) to prevent other apps from triggering your update logic via broadcasts.
| val isPublic = java.lang.reflect.Modifier.isPublic(modifiers) | ||
| assertThat(isPublic).isTrue() |
There was a problem hiding this comment.
This test is named widgetProviderIsOpen but it only asserts the class is public. If the intent is to ensure it’s non-final (open), assert !Modifier.isFinal(modifiers) (or rename the test).
| val isPublic = java.lang.reflect.Modifier.isPublic(modifiers) | |
| assertThat(isPublic).isTrue() | |
| val isOpen = !java.lang.reflect.Modifier.isFinal(modifiers) | |
| assertThat(isOpen).isTrue() |
|
|
||
| ### Coroutines Testing | ||
| - **kotlinx-coroutines-test 1.9.0** - Test coroutines with virtual time | ||
| - **Turbine 1.2.0** - Flow testing made easy |
There was a problem hiding this comment.
This testing guide lists library versions that don’t match the versions declared in app/build.gradle.kts (e.g., AssertJ/MockK/Robolectric/coroutines-test). Please update the documented versions (or remove the specific version numbers) to keep the guide accurate.
| - **Turbine 1.2.0** - Flow testing made easy | |
| - **Turbine** - Flow testing made easy |
| override fun onUpdate(context: Context, appWidgetManager: AppWidgetManager, appWidgetIds: IntArray) { | ||
| super.onUpdate(context, appWidgetManager, appWidgetIds) | ||
| Log.v(Main.LOG_TAG, "WidgetProvider.onUpdate() - re-rendering with new size") | ||
| scheduleImmediateUpdate(context) | ||
| scheduleNextUpdate(context) | ||
| } |
There was a problem hiding this comment.
Widget updates are scheduled but this provider never sets up the widget RemoteViews (e.g., click PendingIntent) in onUpdate/onEnabled. That means the widget can be non-interactive until WidgetUpdateWorker runs (and if the worker is deferred by constraints, clicks do nothing). Consider setting the click PendingIntent (and any static view state) here and using the worker only for rendering/updating the bitmap/text.
| // Get location mode from preferences | ||
| val preferences = PreferenceManager.getDefaultSharedPreferences(applicationContext) | ||
| val locationMode = preferences.get(PreferenceKey.LOCATION_MODE, LocationHandler.MANUAL_PROVIDER) | ||
| Log.v(Main.LOG_TAG, "WidgetUpdateWorker.doWork() location mode: $locationMode") |
There was a problem hiding this comment.
This uses android.preference.PreferenceManager, while the rest of the app (e.g., AppModule/Main) uses androidx.preference.PreferenceManager. Prefer androidx.preference.PreferenceManager.getDefaultSharedPreferences(...) here to avoid deprecated APIs and keep preference access consistent.
| val x = calculateLocalCoordinate(location.longitude, scale) | ||
| val y = calculateLocalCoordinate(location.latitude, scale) | ||
| val dataArea = DataArea(x, y, scale) | ||
|
|
||
| val parameters = Parameters( | ||
| region = 0, | ||
| gridSize = 5000, | ||
| interval = TimeInterval(duration = 60), | ||
| dataArea = dataArea | ||
| ) |
There was a problem hiding this comment.
Parameters is constructed with dataArea (local x/y/scale) but region = 0 (global). In JsonRpcData, GLOBAL_REGION ignores dataArea and calls the global endpoint, so the widget will not request local strike data around the location. Use region = LOCAL_REGION (from org.blitzortung.android.data.provider) when providing a DataArea, or route through the existing LocalData.updateParameters(...) logic to build consistent parameters.
| } catch (e: Exception) { | ||
| Log.e(Main.LOG_TAG, "WidgetUpdateWorker.doWork() failed", e) | ||
| return if (anyWidgetUpdated) { | ||
| Result.success() | ||
| } else { | ||
| // Retry for transient failures like network errors | ||
| Result.retry() | ||
| } | ||
| } |
There was a problem hiding this comment.
If doWork() throws before reaching the per-widget RemoteViews update (e.g., network error), the widget may be left showing the progress indicator set by WidgetClickReceiver, and you return Result.retry() without clearing it. Consider updating the widget in the exception path to hide the progress bar and show an error/last-updated timestamp, then decide whether to retry() or success() (to avoid an infinite progress state).
|




This is closing #45