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

Revision history for this message
Nick Dedekind (nick-dedekind) 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?

Looks good. I'm happy with the changes.
I've pushed your changes to this branch; So I'll review your revisions if you review mine ;)

« Back to merge proposal