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.
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.
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( ) volume_ timer = 0; next_local_ volume) { next_local_ volume = false; local_volume_ timer() ;
{
_local_
if (_send_
_send_
start_
}
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. volume_ timer == 0) {
110 + if (_local_
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.