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

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

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

No. Go back to my original function suggestion:

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
}

When start_local_volume_timer() is called, _local_volume_timer is 0. So a new AS sync will be kicked off already, without having to duplicate code. By setting _local_volume_timer to 0, we are setting the class state as if the user just started changing volume and all the normal code will be run that handles that.

> I only added stop_account_service_volume_timer to start_local_volume_timer. only one place

Right. But we want it called whether this is the first or third tick of the timer, eh? So in my version, it will be called each tick (because each tick goes back to start_local_volume_timer(). But in your version, only the first tick of the timer will stop the AS listener. (You could add a call to stop_account_service_volume_timer() in your version of local_volume_changed_timeout(), but why bother with the duplication?)

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

But in that specific case we can queue up a one second timer without immediately updating volume.

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

Look at lp:~mterry/indicator-sound/as-update-modifications

Does that solve both of our concerns?

« Back to merge proposal