mozilla-mobile/android-components

Sync engine status should be observable through `SyncStore`.

MatthewTighe opened this issue · 5 comments

MatthewTighe commented

To match the pattern of observability of Sync components through the SyncStore, we should updated the SyncEnginesStorage such that it publishes its updates to the Store. This will help decouple implementations from the specifics of requiring either a Context or a SyncEnginesStorage and will generally decrease reliance on needing to know the internals of various Sync components like the FxaAccountManager.

┆Issue is synchronized with this Jira Task

MatthewTighe commented

In mozilla-mobile/fenix#26150, the recent synced tab on the Fenix home screen will not respond to changes to the tabs Sync engine.

The simple solution is to just check the engine status as the feature is started. However, I think it could be beneficial for the future to add sync engine status to the SyncStore so that it can be easily observed. Also, I somehow managed to talk myself out of the simple solution and already did almost all of the work of a patch for it last week, so I selfishly thought it would be worth checking to see if we wanted to accept the patch anyway. I do think all the benefits in the OP are true, but I don't think it's a strictly necessary change. Maybe @jonalmeida can weigh in?

jonalmeida commented

This will help decouple implementations from the specifics of requiring either a Context or a SyncEnginesStorage and will generally decrease reliance on needing to know the internals of various Sync components like the FxaAccountManager.

I'm not sure I follow this part correctly. Are you saying that the SyncStore is coupled to SyncEnginesStorage?

To match the pattern of observability of Sync components through the SyncStore, we should updated the SyncEnginesStorage such that it publishes its updates to the Store.

I agree - this is a nice addition to have. What would the state object look like, I wonder? Something like:

data class SyncState(
  val engines: List<EngineStatus> = emptyList(),
) : State

data class EngineStatus(
  val engine: SyncEngine, // already exists in mozilla.components.service.fxa
  val enabled: Boolean, // same value as `status` in `SyncEnginesStorage.setStatus`
  // more properties over here in the future?
)

I'm also curious to know how you've design the integration to know the engine states from the existing account manager. It might be time to consider passing the store into the account manager to dispatch the status update actions at all the points where SyncEnginesStorage.setStatus is called.

MatthewTighe commented

This will help decouple implementations from the specifics of requiring either a Context or a SyncEnginesStorage and will generally decrease reliance on needing to know the internals of various Sync components like the FxaAccountManager.

I'm not sure I follow this part correctly. Are you saying that the SyncStore is coupled to SyncEnginesStorage?

Rather, that implementations of other Sync features are coupled to SyncEnginesStorage as there is no other way to inspect the status of the engines (other than retrieving it directly from shared preferences, which should also be discouraged IMO).

I agree - this is a nice addition to have. What would the state object look like, I wonder? Something like:

In my current implementation I had the list of enabled engines in the state directly, and removed them as they were disabled. I'm happy to update it with your suggestion if we would like to have the extra detail

I'm also curious to know how you've design the integration to know the engine states from the existing account manager. It might be time to consider passing the store into the account manager to dispatch the status update actions at all the points where SyncEnginesStorage.setStatus is called.

I did something similar and updated SyncEnginesStorage to require a SyncStore which it would then dispatch to. Since SyncEnginesStorage could previously be instantiated using just a Context, it is constructed in-place throughout A-C and Fenix so I am not sure that just updating the account manager would be enough. It may be an opportunity to take a deeper look there and centralize the logic around engines.

In fact, I'll just post what I have as a draft now and we can discuss there if you'd prefer.