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

> > 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.

Hm. did you miss a line? did you mean:

bool local_volume_changed_timeout()
{
 _local_volume_timer = 0;
 if (_send_next_local_volume) {
  _send_next_local_volume = false;
  sync_volume_to_accountsservice.begin (_volume); <--- need to send AS update for local changes.
  start_local_volume_timer();
 }
 return false; // G_SOURCE_REMOVE
}

If this is the case, then it's exactly the same as my code, only that mine is more efficient by not cancelling and re-adding a timer.
I only added stop_account_service_volume_timer to start_local_volume_timer. only one place

> > 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.

C) If we get an AS update which wasn't sourced by the local change while the local timer is still active (but after the local change has already been applied to the AS) the update will be lost and the local value will no longer match. I maybe be wrong though...
The way it is currently will guarantee that the local value always matches the AS value and visa vera (although up to 1 second behind).

>
> 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.

If you're convinced about your method, could you make the changes, test and and pastebin the changelog? I'm finiding it a bit hard to visualize logic.