refactor: extract shared tap-action helper and skip no-op alarm updates#26
Merged
Conversation
- Extract duplicate getLaunchIntentForPackage + fallback logic from both widgets into resolveTapAction() in WidgetTapAction.kt; each widget still reads its own DataStore key so per-widget targets are unchanged - Guard AlarmWidgetReceiver DataStore write + updateAll() behind a change-detection check to avoid redundant writes on repeated broadcasts
Reviewer's GuideRefactors duplicate widget tap-target resolution into a shared helper and adds a change-detection guard to avoid redundant alarm widget updates when the alarm text is unchanged. Sequence diagram for alarm broadcast handling with change-detection guardsequenceDiagram
actor System
participant AlarmManager
participant AlarmWidgetReceiver
participant DataStore
participant AlarmWidget
System ->> AlarmManager: trigger alarm broadcast
AlarmManager ->> AlarmWidgetReceiver: onReceive(intent)
activate AlarmWidgetReceiver
AlarmWidgetReceiver ->> AlarmWidgetReceiver: compute alarmText from intent or default
AlarmWidgetReceiver ->> DataStore: data.first()
DataStore -->> AlarmWidgetReceiver: currentAlarmText
AlarmWidgetReceiver ->> AlarmWidgetReceiver: compare alarmText != currentAlarmText
alt text changed
AlarmWidgetReceiver ->> DataStore: edit set ALARM_TEXT = alarmText
AlarmWidgetReceiver ->> AlarmWidget: updateAll(context)
else text unchanged
AlarmWidgetReceiver ->> AlarmWidgetReceiver: skip DataStore write and updateAll
end
deactivate AlarmWidgetReceiver
Sequence diagram for shared widget tap action resolutionsequenceDiagram
actor User
participant Widget as Widget
participant WidgetTapAction as resolveTapAction
participant PackageManager
participant MainActivity
Widget ->> WidgetTapAction: resolveTapAction(context, tapPackage)
alt tapPackage not null or empty
WidgetTapAction ->> PackageManager: getLaunchIntentForPackage(tapPackage)
PackageManager -->> WidgetTapAction: launchIntent
alt launchIntent has component
WidgetTapAction -->> Widget: Action to start component
else no component
WidgetTapAction -->> Widget: Action to start MainActivity
end
else no tapPackage
WidgetTapAction -->> Widget: Action to start MainActivity
end
User ->> Widget: tap widget
Widget ->> Widget: execute tapAction
Class diagram for shared widget tap action and alarm update guardclassDiagram
class AlarmWidget {
+provideGlance(context)
}
class WeatherWidget {
+provideGlance(context)
}
class AlarmWidgetReceiver {
+onReceive(context, intent)
+updateAlarmText(context, intent)
}
class WidgetTapActionKt {
+resolveTapAction(context, tapPackage)
}
class WeatherDataStore {
<<object>>
+ALARM_WIDGET_TAP_PACKAGE
+WIDGET_TAP_PACKAGE
+ALARM_TEXT
}
class DataStore {
+data
+edit(transform)
}
AlarmWidget --> WeatherDataStore : reads
WeatherWidget --> WeatherDataStore : reads
AlarmWidget --> WidgetTapActionKt : uses
WeatherWidget --> WidgetTapActionKt : uses
AlarmWidgetReceiver --> WeatherDataStore : reads_writes
AlarmWidgetReceiver --> DataStore : uses
AlarmWidgetReceiver --> AlarmWidget : updateAll
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
AlarmWidgetReceiver.updateAlarmText, you’re doing a separatedataStore.data.first()read beforeedit; consider collapsing this to a singleeditcall that checks the existing value inside the transaction and only triggersupdateAllwhen it actually changes to avoid an extra I/O pass. - You might make
resolveTapActiona bit more flexible by parameterizing the fallback activity (e.g., via aKClass<out Activity>defaulting toMainActivity), which keeps the helper reusable if other widgets later need a different default.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `AlarmWidgetReceiver.updateAlarmText`, you’re doing a separate `dataStore.data.first()` read before `edit`; consider collapsing this to a single `edit` call that checks the existing value inside the transaction and only triggers `updateAll` when it actually changes to avoid an extra I/O pass.
- You might make `resolveTapAction` a bit more flexible by parameterizing the fallback activity (e.g., via a `KClass<out Activity>` defaulting to `MainActivity`), which keeps the helper reusable if other widgets later need a different default.
## Individual Comments
### Comment 1
<location path="app/src/main/java/com/wassupluke/widgets/widget/AlarmWidgetReceiver.kt" line_range="54" />
<code_context>
}
- context.dataStore.edit { it[WeatherDataStore.ALARM_TEXT] = alarmText }
- AlarmWidget().updateAll(context)
+ val current = context.dataStore.data.first()[WeatherDataStore.ALARM_TEXT]
+ if (alarmText != current) {
+ context.dataStore.edit { it[WeatherDataStore.ALARM_TEXT] = alarmText }
+ AlarmWidget().updateAll(context)
+ }
} finally {
</code_context>
<issue_to_address>
**suggestion:** Consider folding the read-then-write into a single `edit` transaction to avoid extra I/O and eliminate subtle race windows.
The current pattern does a `dataStore.data.first()` followed by a conditional `edit`, which adds a second transaction and a race window where another writer could change the value between your read and write. You can keep the "only update when changed" optimization and make the comparison/write atomic by doing the read and conditional write inside a single `edit` and tracking whether it changed:
```kotlin
var changed = false
context.dataStore.edit { prefs ->
val current = prefs[WeatherDataStore.ALARM_TEXT]
if (alarmText != current) {
prefs[WeatherDataStore.ALARM_TEXT] = alarmText
changed = true
}
}
if (changed) {
AlarmWidget().updateAll(context)
}
```
Suggested implementation:
```
}
var alarmTextChanged = false
context.dataStore.edit { prefs ->
val current = prefs[WeatherDataStore.ALARM_TEXT]
if (alarmText != current) {
prefs[WeatherDataStore.ALARM_TEXT] = alarmText
alarmTextChanged = true
}
}
if (alarmTextChanged) {
AlarmWidget().updateAll(context)
}
} finally {
```
If `kotlinx.coroutines.flow.first` is no longer used elsewhere in `AlarmWidgetReceiver.kt` after this change, you should remove the unused import:
- `import kotlinx.coroutines.flow.first`
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
| context.dataStore.edit { it[WeatherDataStore.ALARM_TEXT] = alarmText } | ||
| AlarmWidget().updateAll(context) | ||
| val current = context.dataStore.data.first()[WeatherDataStore.ALARM_TEXT] |
There was a problem hiding this comment.
suggestion: Consider folding the read-then-write into a single edit transaction to avoid extra I/O and eliminate subtle race windows.
The current pattern does a dataStore.data.first() followed by a conditional edit, which adds a second transaction and a race window where another writer could change the value between your read and write. You can keep the "only update when changed" optimization and make the comparison/write atomic by doing the read and conditional write inside a single edit and tracking whether it changed:
var changed = false
context.dataStore.edit { prefs ->
val current = prefs[WeatherDataStore.ALARM_TEXT]
if (alarmText != current) {
prefs[WeatherDataStore.ALARM_TEXT] = alarmText
changed = true
}
}
if (changed) {
AlarmWidget().updateAll(context)
}Suggested implementation:
}
var alarmTextChanged = false
context.dataStore.edit { prefs ->
val current = prefs[WeatherDataStore.ALARM_TEXT]
if (alarmText != current) {
prefs[WeatherDataStore.ALARM_TEXT] = alarmText
alarmTextChanged = true
}
}
if (alarmTextChanged) {
AlarmWidget().updateAll(context)
}
} finally {
If kotlinx.coroutines.flow.first is no longer used elsewhere in AlarmWidgetReceiver.kt after this change, you should remove the unused import:
import kotlinx.coroutines.flow.first
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
getLaunchIntentForPackage+ fallback-to-MainActivitylogic from bothWeatherWidgetandAlarmWidgetinto a singleresolveTapAction()helper inWidgetTapAction.kt. Each widget still reads from its own DataStore key (WIDGET_TAP_PACKAGE/ALARM_WIDGET_TAP_PACKAGE), so per-widget tap targets are independent and unchanged.AlarmWidgetReceiver.updateAlarmText()so the DataStore write andupdateAll()call are skipped when the alarm text hasn't changed, avoiding redundant recompositions on repeated broadcasts.Test plan
./gradlew :app:compileDebugKotlinSummary by Sourcery
Refactor widget tap handling into a shared helper and avoid unnecessary alarm widget updates when the alarm text is unchanged.
Enhancements: