Code review comment for lp:~nick-dedekind/indicator-sound/account-services-update-heuristics

Revision history for this message
Michael Terry (mterry) wrote :

> Not sure I understand. If we do this, then we will never
> send updates to AS since we need to call
> sync_volume_to_accountsservice.

I know. But look at my version of the method. It calls start_local_volume_timer() which will both kick off an update to AS and start a new timer. And it's better to have one code path for "things that should happen when we're updating AS" -- for example, you only added a call to stop_account_service_volume_timer() in one of those two places, but not the other.

> If we call accountservice_volume_changed_timeout immediately,
> then the local value will be updated to the asynchronous changes
> given by the AS property updates (which is what is causing the issue).
>
> 1) user set volume = 0.1
> 2) send async AS volume update -> 0.1
> 3) user set volume = 0.2
> 4) send async AS volume update -> 0.2
> 5) async property update from AS from step 2 -> set internal volume = 0.1
> 6) async property update from AS from step 4 -> set internal volume = 0.2
>
> So we get 0.1 -> 0.2 -> 0.1 -> 0.2

With the current branch, the gap between 4-5 is handled (because we don't update local immediately on first AS notification) but at the cost of not responding instantly to notifications we didn't cause (having to wait that one second).

What about doing these two things?
 A) Add back the "if (_local_volume_timer == 0)" guard, but inside start_account_service_volume_timer() instead of accountservice_volume_changed_timeout(). That is, if we have an active local timer, totally ignore the AS notification. This will prevent us from taking AS notifications that resulted from our own actions (as long as AS gets back to us within a second).
 B) If we get an AS notification (and no active local timer exists per above), and we don't have an active AS timer, update local volume (and start an AS timer like usual). This way, we can respond immediately to external notifications, but local notifications likely won't trigger us because of step A above.

If the system is swamped such that the gap between 4-5 is longer than a second (i.e. the local timer ends so we don't notice that there was a recent local volume change), this will have problems. But we already sort of had such problems because if the gap between 5-6 is longer than a second (or two with your original logic), we'll end up applying 0.1 as the volume for a bit. If the system is that swamped, we have no way to figure out which notifications are from us anyway. All we have is time proximity.

« Back to merge proposal