Display current output device icon in the floating volume row when switching output devices

ash/system/audio/volume_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.

[tdanderson, 2017/04/13 16:15:52]

nit: Please wrap the lines in the CL description at 80 characters (this makes it
easier for people to read when examining git logs).

 
 #include "ash/system/audio/volume_view.h"
 
 #include <algorithm>
 
 #include "ash/metrics/user_metrics_action.h"
 #include "ash/resources/vector_icons/vector_icons.h"
 #include "ash/shell_port.h"
 #include "ash/strings/grit/ash_strings.h"
 #include "ash/system/tray/actionable_view.h"
@@ skipping 110 matching lines @@
   icon_ = new VolumeButton(owner, this);
   tri_view_->AddView(TriView::Container::START, icon_);
 
   slider_ = TrayPopupUtils::CreateSlider(this);
   slider_->SetValue(CrasAudioHandler::Get()->GetOutputVolumePercent() / 100.0f);
   slider_->SetAccessibleName(
       l10n_util::GetStringUTF16(IDS_ASH_STATUS_TRAY_VOLUME));
   tri_view_->AddView(TriView::Container::CENTER, slider_);
 
   set_background(views::Background::CreateSolidBackground(kBackgroundColor));
-
-  if (!is_default_view_) {
-    tri_view_->SetContainerVisible(TriView::Container::END, false);
-    Update();
-    return;
-  }
-
-  more_button_ = new ButtonListenerActionableView(
-      owner_, TrayPopupInkDropStyle::INSET_BOUNDS, this);
-  TrayPopupUtils::ConfigureContainer(TriView::Container::END, more_button_);
-
-  more_button_->SetInkDropMode(views::InkDropHostView::InkDropMode::ON);
-  more_button_->SetBorder(views::CreateEmptyBorder(gfx::Insets(
-      0, kTrayPopupButtonEndMargin)));
-  tri_view_->AddView(TriView::Container::END, more_button_);
-
-  device_type_ = TrayPopupUtils::CreateMoreImageView();
-  device_type_->SetVisible(false);
-  more_button_->AddChildView(device_type_);
-
-  more_button_->AddChildView(TrayPopupUtils::CreateMoreImageView());
-  more_button_->SetAccessibleName(
-      l10n_util::GetStringUTF16(IDS_ASH_STATUS_TRAY_AUDIO));
   Update();
 }
 
 VolumeView::~VolumeView() {}
 
 void VolumeView::Update() {
   icon_->Update();
   slider_->UpdateState(!CrasAudioHandler::Get()->IsOutputMuted());
   UpdateDeviceTypeAndMore();
   Layout();
@@ skipping 12 matching lines @@
   slider_->SetValue(percent);
   // It is possible that the volume was (un)muted, but the actual volume level
   // did not change. In that case, setting the value of the slider won't
   // trigger an update. So explicitly trigger an update.
   Update();
   slider_->set_enable_accessibility_events(true);
 }
 
 void VolumeView::UpdateDeviceTypeAndMore() {
   CrasAudioHandler* audio_handler = CrasAudioHandler::Get();
-  bool show_more =
-      is_default_view_ && (audio_handler->has_alternative_output() ||
-                           audio_handler->has_alternative_input());
-
-  if (!show_more)
+  // None output device is connected

[tdanderson, 2017/04/13 16:15:52]

I really appreciate that you are including comments, thanks! As a suggestion,
consider adding a bit more description. As a guideline, comments are often more
helpful if they answer the "why" instead of (or in addition to) the "what". So
here, consider something like:

"There is no point in letting the user open the audio detailed submenu if there
are no alternative output devices present, so do not show the 'more' button in
the default audio row."

+  if (is_default_view_ && !audio_handler->has_alternative_output()) {

[tdanderson, 2017/04/13 16:15:52]

I'm not sure I understand why you aren't also checking &&
!audio_handler->has_alternative_input() here too. A device may only have an
internal speaker available as output, but it could have different options for
input type, in which case we should be showing the 'more' arrow and letting the
user access the audio submenu.

[minch1, 2017/04/13 17:47:39]

If I check !audio_handler->has_alternative_input() here too. Then when I do
testing, I found that it will show 'more button' even there is only
'Microphone(internal)' input shown in UI. I checked the following cl,
https://codereview.chromium.org/1116643005. I think the reason is that the input
type AUDIO_TYPE_HOTWORD, AUDIO_TYPE_POST_MIX_LOOPBACK and
AUDIO_TYPE_POST_DSP_LOOPBACK will not be shown in UI even though there maybe
exist.
So I didn't consider alternative input here. I think another solution should be
modify the logic of has_alternative_input(). To add the above three input types.
Don't consider them as alternative input. Maybe this will be better.

[tdanderson, 2017/04/13 20:54:56]

Ah, now I understand. I think what you're suggesting is correct;
has_alternative_input() should return false if the alternative input is one of
the types you've listed. You should be able to do this in
CrasAudioHandler::UpdateDevicesAndSwitchActive() with just a couple of lines.
Can you try that, add !has_alternative_input() to line 167, and see if that
works as intended?

+    tri_view_->SetContainerVisible(TriView::Container::END, false);
+    if (more_button_)
+      more_button_->InvalidateLayout();

[tdanderson, 2017/04/13 16:15:52]

What happens if you don't invalidate the layout here (and also on line 207)?

[minch1, 2017/04/13 17:47:39]

Open the ash system menu,
1. No alternative output, the slider will extend to the end of the audio row.
2. Plug the headphone, it will add and show the more button (and headphone icon
if headphone is selected).
3. Unplug the headphone, the more button should disappear and slider should
extend to the end of the audio row again. If I didn't invalidate the layout
here. Then the more button will disappear (invisible), but the slider will not
extend to the end of the audio row here. The same as line 207, audio slider will
not become shorter there after plug the headphone.
This is the same as line 214. The END part will become wider with device icon.
Then the audio slider will become shorter corresponding.

[tdanderson, 2017/04/13 20:54:56]

Thanks for the detailed explanation. Can you try calling
tri_view_->InvalidateLayout() instead on line 170 and let me know if it has the
same effect?

I think you have discovered a bug in TriView itself here; clients shouldn't have
to manually invalidate layout after changing container visibility. Can you
please add the following comment here:

"// TODO(tdanderson): TriView itself should trigger a relayout when the
visibility of one of its containers is changed"

and then I will look into that myself as a follow-up.

[minch1, 2017/04/13 23:16:01]

Yes, tri_view_->InvalidateLayout() has the same effect. And I call it manually
now every time after I change the visibility of one of the containers. 

     return;
+  }
 
   const gfx::VectorIcon& device_icon = GetActiveOutputDeviceVectorIcon();
   const bool target_visibility = !device_icon.is_empty();
+
+  if (!is_default_view_) {

[tdanderson, 2017/04/13 16:15:52]

minor nit: it is usually preferred from a style and readability point of view to
structure if-else as:

if (x) {
...
} else {
...
}

rather than

if (!x) {
...
} else {
...
}

+    // Floating audio menu with Speaker(internal) output
+    if (!target_visibility) {

[tdanderson, 2017/04/13 16:15:52]

I find this variable to be a bit awkwardly-named and unnecessary. Consider
deleting |target_visibility| and just saying "if (device_icon.is_empty())" here.
That seems much more readable, WDYT?

[minch1, 2017/04/13 17:47:39]

Yeap, I think so. I moved this from the constructor. Let me do it in next patch.

[minch1, 2017/04/13 23:16:01]

I forgot |target_visibility| will be used in line 212 and 213. I think it will
be a little bit weird to use device_icon.is_empty() there. So, I updated the
name of this variable only.

+      tri_view_->SetContainerVisible(TriView::Container::END, false);
+      return;
+    }
+    device_type_ = TrayPopupUtils::CreateMoreImageView();
+    device_type_->SetVisible(false);

[tdanderson, 2017/04/13 16:15:52]

I'm not sure why you're setting visibility to false here.

[minch1, 2017/04/13 17:47:39]

Actually I think to initialize device_type_ as CreateMoreImageView (the arrow)
here is just to occupy the space. Set it as invisible because we don't need to
show an arrow here. And it will be set to the corresponding icon later (line
213).

[tdanderson, 2017/04/13 20:54:56]

Ah yes, you're right. I missed the fact that CreateMoreImageView() does set the
arrow. So I think this is the correct thing to do.

+    tri_view_->AddView(TriView::Container::END, device_type_);

[tdanderson, 2017/04/13 16:15:52]

It might be a good idea to call
tri_view_->SetContainerVisible(TriView::Container::END, true) here as well (is
it possible we could reach this line after a previous call to
UpdateDeviceTypeAndMore() has set the container to be invisible?)

[minch1, 2017/04/13 17:47:39]

For the floating audio row. I think It will create a new tri_view_ every time
(it will disappear automatically after a little bit while). So I think it should
be impossible to set it as invisible previous when achieve here. But I am not
sure whether we need to do it. Since in line 213, we set the visibility of the
device_type_ according to target_visibility. 

[tdanderson, 2017/04/13 20:54:56]

The only time I could see there being a potential issue is if we were to plug or
unplug an external audio device (headphones, for instance) WHILE the floating
audio row was visible. Does that behave as expected?

[minch1, 2017/04/13 23:16:01]

Yes, you are right. I forgot to plug or unplug the external audio devices while
the floating audio row is visible. I updated it in ps#2.

+  } else {
+    if (!more_button_) {
+      more_button_ = new ButtonListenerActionableView(
+          owner_, TrayPopupInkDropStyle::INSET_BOUNDS, this);
+      TrayPopupUtils::ConfigureContainer(TriView::Container::END, more_button_);
+
+      more_button_->SetInkDropMode(views::InkDropHostView::InkDropMode::ON);
+      more_button_->SetBorder(
+          views::CreateEmptyBorder(gfx::Insets(0, kTrayPopupButtonEndMargin)));
+      tri_view_->AddView(TriView::Container::END, more_button_);
+
+      device_type_ = TrayPopupUtils::CreateMoreImageView();
+      device_type_->SetVisible(false);
+      more_button_->AddChildView(device_type_);
+
+      more_button_->AddChildView(TrayPopupUtils::CreateMoreImageView());
+      more_button_->SetAccessibleName(
+          l10n_util::GetStringUTF16(IDS_ASH_STATUS_TRAY_AUDIO));
+    }
+
+    tri_view_->SetContainerVisible(TriView::Container::END, true);
+    more_button_->InvalidateLayout();
+  }
+
   if (target_visibility)
     device_type_->SetImage(gfx::CreateVectorIcon(device_icon, kMenuIconColor));
   if (device_type_->visible() != target_visibility) {
     device_type_->SetVisible(target_visibility);
     device_type_->InvalidateLayout();
   }
 }
 
 void VolumeView::HandleVolumeUp(int level) {
   CrasAudioHandler* audio_handler = CrasAudioHandler::Get();
@@ skipping 46 matching lines @@
     if (new_volume > current_volume)
       HandleVolumeUp(new_volume);
     else
       HandleVolumeDown(new_volume);
   }
   icon_->Update();
 }
 
 }  // namespace tray
 }  // namespace ash