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

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

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
}

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.

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.

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.

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.

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

review: Needs Fixing

« Back to merge proposal