Skip to content

Retain AchievementManager UI notifier instead of weakly referencing it#78

Closed
Copilot wants to merge 2 commits into
masterfrom
copilot/fix-review-comment-3380005450
Closed

Retain AchievementManager UI notifier instead of weakly referencing it#78
Copilot wants to merge 2 commits into
masterfrom
copilot/fix-review-comment-3380005450

Conversation

Copilot AI commented Jun 9, 2026

Copy link
Copy Markdown

The reviewed code stored AchievementManager's UiNotifier in a WeakReference, which allowed notifier callbacks to disappear unexpectedly. This change keeps the notifier alive until it is explicitly cleared, matching the intended callback lifecycle.

  • Notifier lifetime

    • Replace WeakReference<UiNotifier?> with a strong UiNotifier?
    • Update setUiNotifier() to store the notifier directly and clear it on null
  • Android adapter path

    • Update setCurrentActivity() to install the toast-backed notifier as a strong reference
    • Clear the notifier when the activity is cleared, preserving explicit lifecycle control
  • Regression coverage

    • Add a focused test that verifies:
      • the exact notifier instance is retained after setUiNotifier(notifier)
      • the notifier is removed after setUiNotifier(null)
private var uiNotifier: UiNotifier? = null

fun setUiNotifier(notifier: UiNotifier?) {
    this.uiNotifier = notifier
}

Copilot AI changed the title [WIP] Fix code as per review comment 3380005450 Retain AchievementManager UI notifier instead of weakly referencing it Jun 9, 2026
Copilot finished work on behalf of rubo77 June 9, 2026 21:24
Copilot AI requested a review from rubo77 June 9, 2026 21:24
@rubo77 rubo77 marked this pull request as ready for review June 10, 2026 05:32
Copilot AI review requested due to automatic review settings June 10, 2026 05:32
@rubo77 rubo77 closed this Jun 10, 2026
@rubo77 rubo77 deleted the copilot/fix-review-comment-3380005450 branch June 10, 2026 05:33

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes an unintended notifier lifecycle issue in AchievementManager where UI callbacks could disappear due to storing the UiNotifier in a WeakReference. It updates the manager to retain the notifier strongly until it is explicitly cleared, which better matches the expected callback lifecycle (including the Android toast-backed notifier path).

Changes:

  • Replace WeakReference<UiNotifier?> storage with a strong UiNotifier? field.
  • Update setCurrentActivity() to install/clear the toast-backed notifier via strong assignment (sets null when activity is cleared).
  • Add an Android instrumentation regression test verifying the notifier instance is retained and then cleared.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
app/src/main/java/roboyard/logic/achievements/AchievementManager.kt Switch notifier storage to a strong reference; adjust Android activity adapter setup and update-nudge dispatch accordingly.
app/src/androidTest/java/roboyard/eclabs/achievements/AchievementManagerTest.java Add regression test asserting setUiNotifier() retains the exact instance and clears it when set to null.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants