Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Player Model] The getter for WorkManager considers the app context #2123

Merged
merged 1 commit into from
Jun 13, 2024

Conversation

nan-li
Copy link
Contributor

@nan-li nan-li commented Jun 12, 2024

Description

One Line Summary

Allow custom work manager to be initialized on demand, which resolves Hilt dependency manager conflict.

Details

  • With helpful comments from the community and revisiting WorkManager source code.
  • We were mimicking the isInitialized() API that checks for a WorkManager instance via WorkManagerImpl.getInstance() and initializing it with a blank Configuration when the getter returned null.
  • However, we should use WorkManagerImpl.getInstance(Context) instead as this getter will allow for on-demand initialization with the app's custom WorkManager.
  • Because Hilt dependency manager or other app configurations such as using lateinit may not have the WorkManager instance initialized at the time we check.

Motivation

Scope

Work Manager initialization.

Testing

Unit testing

  • None due to limited ability to reproduce original crashes
  • Except to run a bare unit test that experiences the original "not found" crash when given a testing context.
  • Then using the OSWorkManagerHelper's getter instead does not crash and returns a new Work Manager instance.
@Test
public void testTest() {
    // Crashes
    WorkManager.getInstance(ApplicationProvider.getApplicationContext());
    
    // Works
    OSWorkManagerHelper.getInstance(ApplicationProvider.getApplicationContext());
}

Manual testing

Limited ability to test manually due to unable to reproduce original crash

  • Tested normal app flow in an emulator
  • Receiving notifications in background and swiped away state
  • Opening app, etc.
  • Receive receipt worker runs and succeeds

Tested a client-supplied sample Hilt project and appears to resolve the problems.

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
  • I have reviewed this PR myself, ensuring it meets each checklist item

This change is Reviewable

@nan-li nan-li changed the title [v4] The getter for WorkManager considers the app context [] The getter for WorkManager considers the app context Jun 12, 2024
@nan-li nan-li changed the title [] The getter for WorkManager considers the app context [Player Model] The getter for WorkManager considers the app context Jun 12, 2024
@nan-li nan-li requested review from jkasten2 and emawby June 12, 2024 20:37
* We were getting the WorkManager instance via `WorkManagerImpl.getInstance()` and initializing it with a blank Configuration when the getter returned null.
* However, we should use `WorkManagerImpl.getInstance(Context)` instead as this getter will allow for on-demand initialization with the app's custom WorkManager.
@nan-li nan-li force-pushed the player_model_work_manager_uses_context branch from 3e2f3ee to baf9213 Compare June 13, 2024 17:59
@nan-li nan-li merged commit f54094a into player-model-main Jun 13, 2024
1 of 2 checks passed
@nan-li nan-li deleted the player_model_work_manager_uses_context branch June 13, 2024 21:25
@nan-li nan-li mentioned this pull request Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants