Store audio device's active state in preference

chromeos/audio/cras_audio_handler.cc

Index: chromeos/audio/cras_audio_handler.cc
diff --git a/chromeos/audio/cras_audio_handler.cc b/chromeos/audio/cras_audio_handler.cc
index 8ee6009f35c69e6ee1ec3070716b5bf75654fd30..1238e36c137f44b6e2e2b216865d5ea2a2cb98e0 100644
--- a/chromeos/audio/cras_audio_handler.cc
+++ b/chromeos/audio/cras_audio_handler.cc
@@ -43,14 +43,6 @@ bool IsInNodeList(uint64_t node_id,
   return std::find(id_list.begin(), id_list.end(), node_id) != id_list.end();
 }
 
-bool IsNodeInTheList(uint64_t node_id, const AudioNodeList& node_list) {
-  for (size_t i = 0; i < node_list.size(); ++i) {
-    if (node_id == node_list[i].id)
-      return true;
-  }
-  return false;
-}
-
 }  // namespace
 
 CrasAudioHandler::AudioObserver::AudioObserver() {
@@ -395,6 +387,15 @@ void CrasAudioHandler::SetActiveOutputNode(uint64_t node_id, bool notify) {
       SetActiveOutputNode(node_id);
   if (notify)
     NotifyActiveNodeChanged(false);
+
+  // Save state for all output nodes.
+  for (AudioDeviceMap::iterator it = audio_devices_.begin();
+       it != audio_devices_.end(); ++it) {
+    if (it->second.is_input)
+      continue;
+    audio_pref_handler_->SetDeviceState(it->second,
+        it->second.active ? AUDIO_STATE_ACTIVE : AUDIO_STATE_INACTIVE);
+  }
 }
 
 void CrasAudioHandler::SetActiveInputNode(uint64_t node_id, bool notify) {
@@ -402,6 +403,15 @@ void CrasAudioHandler::SetActiveInputNode(uint64_t node_id, bool notify) {
       SetActiveInputNode(node_id);
   if (notify)
     NotifyActiveNodeChanged(true);
+
+  // Save state for all input nodes.
+  for (AudioDeviceMap::iterator it = audio_devices_.begin();
+       it != audio_devices_.end(); ++it) {
+    if (!it->second.is_input)
+      continue;
+    audio_pref_handler_->SetDeviceState(it->second,
+        it->second.active ? AUDIO_STATE_ACTIVE : AUDIO_STATE_INACTIVE);
+  }
 }
 
 void CrasAudioHandler::SetVolumeGainPercentForDevice(uint64_t device_id,
@@ -780,7 +790,7 @@ void CrasAudioHandler::SwitchToDevice(const AudioDevice& device, bool notify) {
 
 bool CrasAudioHandler::HasDeviceChange(const AudioNodeList& new_nodes,
                                        bool is_input,
-                                       AudioNodeList* new_discovered) {
+                                       AudioDevicePriorityQueue* new_discovered) {
   size_t num_old_devices = 0;
   size_t num_new_devices = 0;
   for (AudioDeviceMap::const_iterator it = audio_devices_.begin();
@@ -790,7 +800,8 @@ bool CrasAudioHandler::HasDeviceChange(const AudioNodeList& new_nodes,
   }
 
   bool new_or_changed_device = false;
-  new_discovered->clear();
+  while (!new_discovered->empty())
+    new_discovered->pop();
   for (AudioNodeList::const_iterator it = new_nodes.begin();
        it != new_nodes.end(); ++it) {
     if (is_input == it->is_input) {
@@ -799,7 +810,7 @@ bool CrasAudioHandler::HasDeviceChange(const AudioNodeList& new_nodes,
       AudioDevice device(*it);
       DeviceStatus status = CheckDeviceStatus(device);
       if (status == NEW_DEVICE)
-        new_discovered->push_back(*it);
+        new_discovered->push(device);
       if (status == NEW_DEVICE || status == CHANGED_DEVICE) {
         new_or_changed_device = true;
       }
@@ -845,8 +856,8 @@ void CrasAudioHandler::UpdateDevicesAndSwitchActive(
       ++old_output_device_size;
   }
 
-  AudioNodeList hotplug_output_nodes;
-  AudioNodeList hotplug_input_nodes;
+  AudioDevicePriorityQueue hotplug_output_nodes;
+  AudioDevicePriorityQueue hotplug_input_nodes;
   bool output_devices_changed =
       HasDeviceChange(nodes, false, &hotplug_output_nodes);
   bool input_devices_changed =
@@ -910,12 +921,27 @@ void CrasAudioHandler::UpdateDevicesAndSwitchActive(
       NotifyActiveNodeChanged(true);
     } else {
       // If user has hot plugged a new node, we should change to the active
-      // device to the new node if it has the highest priority; otherwise,
-      // we should keep the existing active node chosen by user.
+      // device to the new node if (1) it has the highest priority; or (2)
+      // the last state before it hot plugged was active; otherwise, we should
+      // keep the existing active node chosen by user.
       // For all other cases, we will choose the node with highest priority.

[jennyz, 2015/12/17 22:50:21]

How about change this as:
If there is no current active node, or, no new hot plugged node, select the
active node by their priorities.

[hychao, 2015/12/29 10:57:25]

Done.

-      if (!active_input_node_id_ || hotplug_input_nodes.empty() ||
-          IsNodeInTheList(input_devices_pq_.top().id, hotplug_input_nodes)) {
+      if (!active_input_node_id_ || hotplug_input_nodes.empty()) {
         SwitchToDevice(input_devices_pq_.top(), true);
+      } else {

[jennyz, 2015/12/17 22:50:21]

How about comment like this:
If user has hot plugged any input nodes, look at the one with highest
priorty(normally, there is only one hot plugged input node), 1) If it was a new
device with higher priority, switch the active to this device. 2) If it was
device plugged in before, and was NOT selected as the active one last time
before it was unplugged, then don't change the current active device. 3) If it
was plugged in before and was selected as the active one before it was unplugged
last time, switch the active to this device.

[hychao, 2015/12/29 10:57:25]

Thanks for the suggestion. I broke down and moved the comments of these device
states to each the switch case below, to make it more readable.

+        AudioDeviceState last_state =
+            audio_pref_handler_->GetDeviceState(hotplug_input_nodes.top());
+        switch (last_state) {
+        case AUDIO_STATE_ACTIVE:
+          SwitchToDevice(hotplug_input_nodes.top(), true);
+          break;
+        case AUDIO_STATE_NOT_AVAILABLE:

[jennyz, 2015/12/17 22:50:21]

This is the case if the device is new, not plugged in before, right? If true,
please add a comment.

[hychao, 2015/12/29 10:57:25]

Done.

+          if (input_devices_pq_.top().id == hotplug_input_nodes.top().id)
+            SwitchToDevice(hotplug_input_nodes.top(), true);
+          break;
+        case AUDIO_STATE_INACTIVE:

[jennyz, 2015/12/17 22:50:21]

No need to add this line, it should fall into default case. If you would like to
add this line for drawing attention for this case, please add a comment.

[hychao, 2015/12/29 10:57:25]

Thanks for reminding. I think its useful to keep this line, so I can put the
comment right next to AUDIO_STATE_INACTIVE case.

+        default:
+          break;
+        }
       }
     }
   }
@@ -930,9 +956,23 @@ void CrasAudioHandler::UpdateDevicesAndSwitchActive(
       NotifyActiveNodeChanged(false);
     } else {
       // ditto input node case.
-      if (!active_output_node_id_ || hotplug_output_nodes.empty() ||
-          IsNodeInTheList(output_devices_pq_.top().id, hotplug_output_nodes)) {
+      if (!active_output_node_id_ || hotplug_output_nodes.empty()) {
         SwitchToDevice(output_devices_pq_.top(), true);
+      } else {
+        AudioDeviceState last_state =
+            audio_pref_handler_->GetDeviceState(hotplug_output_nodes.top());
+        switch (last_state) {
+        case AUDIO_STATE_ACTIVE:
+          SwitchToDevice(hotplug_output_nodes.top(), true);
+          break;
+        case AUDIO_STATE_NOT_AVAILABLE:
+          if (output_devices_pq_.top().id == hotplug_output_nodes.top().id)
+            SwitchToDevice(hotplug_output_nodes.top(), true);
+          break;
+        case AUDIO_STATE_INACTIVE:

[jennyz, 2015/12/17 22:50:21]

ditto

[hychao, 2015/12/29 10:57:25]

Although the comments are not copied over, I keep this line for symmetry to the
input logic.

+        default:
+          break;
+        }
       }
     }
   }