Refactor AddScrollListItem() in system menu detailed views

ash/system/audio/audio_detailed_view.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ash/system/audio/audio_detailed_view.h"
 
 #include "ash/resources/vector_icons/vector_icons.h"
 #include "ash/strings/grit/ash_strings.h"
 #include "ash/system/tray/hover_highlight_view.h"
 #include "ash/system/tray/tray_popup_utils.h"
 #include "ash/system/tray/tri_view.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chromeos/audio/cras_audio_handler.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/gfx/color_palette.h"
 #include "ui/gfx/paint_vector_icon.h"
-#include "ui/native_theme/native_theme.h"
 #include "ui/views/controls/image_view.h"
-#include "ui/views/controls/label.h"
 #include "ui/views/controls/scroll_view.h"
 #include "ui/views/controls/separator.h"
 
 namespace {
 
 base::string16 GetAudioDeviceName(const chromeos::AudioDevice& device) {
   switch (device.type) {
     case chromeos::AUDIO_TYPE_FRONT_MIC:
       return l10n_util::GetStringUTF16(IDS_ASH_STATUS_TRAY_AUDIO_FRONT_MIC);
     case chromeos::AUDIO_TYPE_HEADPHONE:
@@ skipping 36 matching lines @@
   Update();
 }
 
 AudioDetailedView::~AudioDetailedView() {}
 
 void AudioDetailedView::Update() {
   UpdateAudioDevices();
   Layout();
 }
 
-void AudioDetailedView::AddInputHeader() {
-  AddScrollListInfoItem(IDS_ASH_STATUS_TRAY_AUDIO_INPUT,
-                        kSystemMenuAudioInputIcon);
-}
-
-void AudioDetailedView::AddOutputHeader() {
-  AddScrollListInfoItem(IDS_ASH_STATUS_TRAY_AUDIO_OUTPUT,
-                        kSystemMenuAudioOutputIcon);
-}
-
-void AudioDetailedView::AddScrollListInfoItem(int text_id,
-                                              const gfx::VectorIcon& icon) {
-  TriView* header = TrayPopupUtils::CreateDefaultRowView();

[tdanderson, 2017/04/24 15:43:39]

Just wanted to double-check that this works as intended, since this function
calls CreateDefaultRowView() but its replacement (AddScrollListSubHeader()) is
calling CreateSubHeaderRowView().

[mohsen, 2017/04/25 05:10:01]

Yes, different layout managers are installed with different paddings. However,
they produce very similar output for our purposes. I talked with bruthig@ and
decided that CreateSubHeaderRowView() should actually use CreateDefaultRoView()
instead of directly using CreateMultiTargetRowView() so that similar padding and
layout management is used for sub-header rows, too. Done. WDYT?

[tdanderson, 2017/04/25 14:32:48]

By 'very similar' do you mean that the implementation in Patch Set 4 will give
the incorrect layout for the audio subheader rows?
Unless the code in patch set 4 actually gives the incorrect layout, I would
prefer for the time being to avoid calling CreateDefaultRowView() directly from
CreateSubHeaderRowView() like you have in patch set 6. If there is an
opportunity to share code between the two then by all means create a private
helper function, but I see it as a bit confusing to call a function designed for
use by default views directly from a function designed for use by detailed
views.

I am also happy to table this discussion for a follow-on CL, provided that
whatever you're landing in this CL will still follow design specs.

[mohsen, 2017/04/25 19:23:47]

Yes, the label padding was 4 pixels off.
I don't think CreateDefaultRowView() is designed only for default views. It is
also used for rows in detailed views, as far as I can tell. And it seems
reasonable to me to use it for sub-header rows as we want icons and labels in
sub-headers and regular rows be aligned properly.

[tdanderson, 2017/04/25 21:00:27]

Yes you're right. I was reading the function as "create (default row) view"
rather than "create default (row view)". Doing this sgtm.

-  TrayPopupUtils::ConfigureAsStickyHeader(header);
-  views::ImageView* image_view = TrayPopupUtils::CreateMainImageView();
-  image_view->SetImage(gfx::CreateVectorIcon(
-      icon, GetNativeTheme()->GetSystemColor(
-                ui::NativeTheme::kColorId_ProminentButtonColor)));
-  header->AddView(TriView::Container::START, image_view);
-
-  views::Label* label = TrayPopupUtils::CreateDefaultLabel();
-  label->SetText(l10n_util::GetStringUTF16(text_id));
-  TrayPopupItemStyle style(TrayPopupItemStyle::FontStyle::SUB_HEADER);
-  style.SetupLabel(label);
-  header->AddView(TriView::Container::CENTER, label);
-
+void AudioDetailedView::AddAudioSubHeader(const gfx::VectorIcon& icon,
+                                          int text_id) {
+  const base::string16 text = l10n_util::GetStringUTF16(text_id);
+  TriView* header = AddScrollListSubHeader(icon, text);

[tdanderson, 2017/04/24 15:43:39]

Consider passing in the text_id to AddScrollListSubHeader instead of the string
itself, which will eliminate the need for each call site to call
l10_util::GetStringUTF16(). This assumes that all of our subheaders have (and
will have) a pre-defined string id, which is probably the case. WDYT?

[mohsen, 2017/04/25 05:10:01]

Done. I wanted AddScrollListSubHeader() to be more consistent with
AddScrollListItem() which cannot take a resource id. But, your suggestion also
makes sense and I don't have any preference.

[tdanderson, 2017/04/25 14:32:48]

Ah I see, I guess we could have gone either way with this.

   header->SetContainerVisible(TriView::Container::END, false);
-  scroll_content()->AddChildView(header);
-}
-
-HoverHighlightView* AudioDetailedView::AddScrollListItem(
-    const base::string16& text,
-    bool highlight,
-    bool checked) {
-  HoverHighlightView* container = new HoverHighlightView(this);
-
-  container->AddLabelRow(text);
-  TrayPopupUtils::InitializeAsCheckableRow(container, checked);
-
-  scroll_content()->AddChildView(container);
-  return container;
 }
 
 void AudioDetailedView::CreateItems() {
   CreateScrollableList();
   CreateTitleRow(IDS_ASH_STATUS_TRAY_AUDIO);
 }
 
 void AudioDetailedView::UpdateAudioDevices() {
   output_devices_.clear();
   input_devices_.clear();
   chromeos::AudioDeviceList devices;
   CrasAudioHandler* audio_handler = CrasAudioHandler::Get();
   audio_handler->GetAudioDevices(&devices);
   bool has_dual_internal_mic = audio_handler->HasDualInternalMic();
   bool is_front_or_rear_mic_active = false;
-  for (size_t i = 0; i < devices.size(); ++i) {
+  for (const auto& device : devices) {
     // Don't display keyboard mic or aokr type.
-    if (!devices[i].is_for_simple_usage())
+    if (!device.is_for_simple_usage())
       continue;
-    if (devices[i].is_input) {
+    if (device.is_input) {
       // Do not expose the internal front and rear mic to UI.
-      if (has_dual_internal_mic &&
-          audio_handler->IsFrontOrRearMic(devices[i])) {
-        if (devices[i].active)
+      if (has_dual_internal_mic && audio_handler->IsFrontOrRearMic(device)) {
+        if (device.active)
           is_front_or_rear_mic_active = true;
         continue;
       }
-      input_devices_.push_back(devices[i]);
+      input_devices_.push_back(device);
     } else {
-      output_devices_.push_back(devices[i]);
+      output_devices_.push_back(device);
     }
   }
 
   // Expose the dual internal mics as one device (internal mic) to user.
   if (has_dual_internal_mic) {
     // Create stub internal mic entry for UI rendering, which representing
     // both internal front and rear mics.
     chromeos::AudioDevice internal_mic;
     internal_mic.is_input = true;
     internal_mic.stable_device_id_version = 2;
     internal_mic.type = chromeos::AUDIO_TYPE_INTERNAL_MIC;
     internal_mic.active = is_front_or_rear_mic_active;
     input_devices_.push_back(internal_mic);
   }
 
   UpdateScrollableList();
 }
 
 void AudioDetailedView::UpdateScrollableList() {
   scroll_content()->RemoveAllChildViews(true);
   device_map_.clear();
 
   // Add audio output devices.
   const bool has_output_devices = output_devices_.size() > 0;
-  if (has_output_devices)
-    AddOutputHeader();
+  if (has_output_devices) {
+    AddAudioSubHeader(kSystemMenuAudioOutputIcon,
+                      IDS_ASH_STATUS_TRAY_AUDIO_OUTPUT);
+  }
 
-  for (size_t i = 0; i < output_devices_.size(); ++i) {
-    HoverHighlightView* container = AddScrollListItem(
-        GetAudioDeviceName(output_devices_[i]), false /* highlight */,
-        output_devices_[i].active); /* checkmark if active */
-    device_map_[container] = output_devices_[i];
+  for (const auto& device : output_devices_) {
+    HoverHighlightView* container = AddScrollListCheckableItem(
+        gfx::kNoneIcon, GetAudioDeviceName(device), device.active);
+    device_map_[container] = device;
   }
 
   if (has_output_devices) {
     scroll_content()->AddChildView(
         TrayPopupUtils::CreateListSubHeaderSeparator());
   }
 
   // Add audio input devices.
   const bool has_input_devices = input_devices_.size() > 0;
-  if (has_input_devices)
-    AddInputHeader();
+  if (has_input_devices) {
+    AddAudioSubHeader(kSystemMenuAudioInputIcon,
+                      IDS_ASH_STATUS_TRAY_AUDIO_INPUT);
+  }
 
-  for (size_t i = 0; i < input_devices_.size(); ++i) {
-    HoverHighlightView* container = AddScrollListItem(
-        GetAudioDeviceName(input_devices_[i]), false /* highlight */,
-        input_devices_[i].active); /* checkmark if active */
-    device_map_[container] = input_devices_[i];
+  for (const auto& device : input_devices_) {
+    HoverHighlightView* container = AddScrollListCheckableItem(
+        gfx::kNoneIcon, GetAudioDeviceName(device), device.active);
+    device_map_[container] = device;
   }
 
   scroll_content()->SizeToPreferredSize();
   scroller()->Layout();
 }
 
 void AudioDetailedView::HandleViewClicked(views::View* view) {
   AudioDeviceMap::iterator iter = device_map_.find(view);
   if (iter == device_map_.end())
     return;
   chromeos::AudioDevice device = iter->second;
   CrasAudioHandler* audio_handler = CrasAudioHandler::Get();
   if (device.type == chromeos::AUDIO_TYPE_INTERNAL_MIC &&
       audio_handler->HasDualInternalMic()) {
     audio_handler->SwitchToFrontOrRearMic();
   } else {
     audio_handler->SwitchToDevice(device, true,
                                   CrasAudioHandler::ACTIVATE_BY_USER);
   }
 }
 
 }  // namespace tray
 }  // namespace ash