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

> Functionality wise, it seems to work fine. Much snappier, and thank you for
> cleaning up after my mistake.
>
> Code comments:
>
> - I think I'd prefer if local_volume_changed_timeout() looked more like the
> following, so we don't duplicate logic about what to do when we change AS
> volume. But no biggie either way. Just a comment.
>
> bool local_volume_changed_timeout()
> {
> _local_volume_timer = 0;
> if (_send_next_local_volume) {
> _send_next_local_volume = false;
> start_local_volume_timer();
> }
> return false; // G_SOURCE_REMOVE
> }

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. It's different to AS property update timeout as we want to send the first update immediately.
What the current code does is:
1) if the timeout hasn't been started, send an AS update immediately and start the timeout (for case 2).
2) if the timeout is already started, only send an AS update at the end of the timeout.

>
> 96 + _accountservice_volume_timer = Timeout.add_seconds (2,
> accountservice_volume_changed_timeout);
>
> Two seconds is a long time. What if I change volume in greeter and swipe away
> immediately. I'll see old volume.

The update will be delayed for 2 seconds. Could make it smaller.

>
> Also, why don't we immediately call accountservice_volume_changed_timeout()
> and then set a timeout for 2 seconds later to check again (like we do for
> local volume -- change first, then set a timer for updates)? Surely if we get
> an AS notification and we haven't changed local volume recently, no reason to
> wait 2 seconds.

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 2 -> set internal volume = 0.2

So we get 0.1 -> 0.2 -> 0.1 -> 0.2

>
> 109 + // if the local value hasn't changed.
> 110 + if (_local_volume_timer == 0) {
>
> What if local value has changed and the one second timer has expired while our
> two second AS timer was going? Or even if they were both on one second
> timers, we don't know which order glib will go through that second's
> callbacks. Maybe have start_local_volume_timer() call
> stop_account_service_volume_timer()? That way we can drop the check on
> _local_volume_timer in the AS callback completely and trust that the callback
> just won't happen if local volume is changed.

Done

>
> 115 + return true; // G_SOURCE_CONTINUE
>
> That seems risky. If we get an update for AS, we wait two seconds. In the
> meantime local volume is changed. When we get the AS callback, we see that
> and wait another two seconds. At the end of which we apply the AS volume from
> four seconds ago... Right? If I'm reading that correctly, seems bad. Another
> reason to call stop_account_service_volume_timer() when local volume is
> updated.

We only apply the AS volume from the latest received property update (_account_service_volume = volume),
not the value when we started the timer.
But removed as result of calling stop_account_service_volume_timer

>
> I mean, not that a user is going to be going back and forth changing volumes
> in two different places. But still.