android icon indicating copy to clipboard operation
android copied to clipboard

Remove usage of `runBlocking` from `SettingsActivity`

Open loganrosen opened this issue 4 months ago • 4 comments

Summary

As part of #5688, removing usage of runBlocking from SettingsActivity. This involved changing setAppActive into a suspend function and then making sure it's invoked from a coroutine.

Checklist

  • [ ] New or updated tests have been added to cover the changes following the testing guidelines.
  • [x] The code follows the project's code style and best_practices.
  • [x] The changes have been thoroughly tested, and edge cases have been considered.
  • [x] Changes are backward compatible whenever feasible. Any breaking changes are documented in the changelog for users and/or in the code for developers depending on the relevance.

loganrosen avatar Oct 12 '25 01:10 loganrosen

After thinking about it a bit more: by setting the active state in the lifecycleScope, we risk that the app is not set as inactive before the activity/fragment/app is quit, as the scope may be cancelled. That should never happen as it defeats the purpose of an app lock.

(This may also apply in other places, not only here)

jpelgrom avatar Oct 14 '25 19:10 jpelgrom

After thinking about it a bit more: by setting the active state in the lifecycleScope, we risk that the app is not set as inactive before the activity/fragment/app is quit, as the scope may be cancelled. That should never happen as it defeats the purpose of an app lock.

(This may also apply in other places, not only here)

@jpelgrom Do we need to use an application-level scope here?

loganrosen avatar Oct 16 '25 02:10 loganrosen

@jpelgrom Do we need to use an application-level scope here?

I missed that this PR was waiting for a response from me, sorry.

I'm tempted to say yes, but would like another opinion. Maybe I'm making this too big, and it would increase the scope of the PR. @TimoPtr what do you think?

jpelgrom avatar Oct 27 '25 12:10 jpelgrom

@jpelgrom Do we need to use an application-level scope here?

I missed that this PR was waiting for a response from me, sorry.

I'm tempted to say yes, but would like another opinion. Maybe I'm making this too big, and it would increase the scope of the PR. @TimoPtr what do you think?

In general I'm not a big fan of application scope. It could be in a viewModel.

Now that I think a bit more about it, we could still have race condition if for some reason the localStorage is slow. We might need to make the IntegrationRepositoryImpl.appActive thread safe.

It could be made in a first PR before this one. Also I would like to have tests for such changes.

TimoPtr avatar Oct 27 '25 13:10 TimoPtr