Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2812223005:
Display current output device icon in the floating volume row when switching output devices (Closed)
DescriptionDisplay current output device icon in the floating volume row when
switching output devices.
Changes in this cl,
1. Remove the arrow when none output devices are connected.
2. When there is external output device connected, show the device icon
in the floating audio row.
3. Make the slider of brightness row in system tray always extend to
the end of the row. Keep the same length as the audio slider when none
external output / input devices are connected.
4. Input devices type as AUDIO_TYPE_HOTWORD,
AUDIO_TYPE_POST_MIX_LOOPBACK and AUDIO_TYPE_POST_DSP_LOOPBACK
will not be shown in the UI. Treat them as non external input devices.
Since the arrow (more_button_) is part of the update.
Move the logic of more_button_ to UpdateDeviceTypeAndMore().
BUG=686946
Review-Url: https://codereview.chromium.org/2812223005
Cr-Commit-Position: refs/heads/master@{#465877}
Committed: https://chromium.googlesource.com/chromium/src/+/976586028ad74aceffb12622af62305a1a00c163
Patch Set 1 #
Total comments: 20
Patch Set 2 : Address comments. #
Total comments: 6
Patch Set 3 : Address nits. #
Messages
Total messages: 41 (29 generated)
The CQ bit was checked by minch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Display current output device icon in the floating volume row when switching output devices BUG=686946 ========== to ========== Display current output device icon in the floating volume row when switching output devices. Changes in this cl, 1. Remove the arrow when none output device is connected. 2. When some output device is connected, show the device icon in the floating audio row. Since the arrow (more_button_) is part of the update. Move the logic of more_button_ to UpdateDeviceTypeAndMore(). BUG=686946 ==========
minch@chromium.org changed reviewers: + tdanderson@chromium.org
Hi, tdanderson@, can you help review? Thanks.
Description was changed from ========== Display current output device icon in the floating volume row when switching output devices. Changes in this cl, 1. Remove the arrow when none output device is connected. 2. When some output device is connected, show the device icon in the floating audio row. Since the arrow (more_button_) is part of the update. Move the logic of more_button_ to UpdateDeviceTypeAndMore(). BUG=686946 ========== to ========== Display current output device icon in the floating volume row when switching output devices. Changes in this cl, 1. Remove the arrow when none output device is connected. 2. When some output device is connected, show the device icon in the floating audio row. Since the arrow (more_button_) is part of the update. Move the logic of more_button_ to UpdateDeviceTypeAndMore(). Only consider no alternative output here (has_alternative_output()). Since AUDIO_TYPE_HOTWORD, AUDIO_TYPE_POST_MIX_LOOPBACK and AUDIO_TYPE_POST_DSP_LOOPBACK will be treated as alternative input but will not shown in the input detailed view in the ash. BUG=686946 ==========
Description was changed from ========== Display current output device icon in the floating volume row when switching output devices. Changes in this cl, 1. Remove the arrow when none output device is connected. 2. When some output device is connected, show the device icon in the floating audio row. Since the arrow (more_button_) is part of the update. Move the logic of more_button_ to UpdateDeviceTypeAndMore(). Only consider no alternative output here (has_alternative_output()). Since AUDIO_TYPE_HOTWORD, AUDIO_TYPE_POST_MIX_LOOPBACK and AUDIO_TYPE_POST_DSP_LOOPBACK will be treated as alternative input but will not shown in the input detailed view in the ash. BUG=686946 ========== to ========== Display current output device icon in the floating volume row when switching output devices. Changes in this cl, 1. Remove the arrow when none output device is connected. 2. When some output device is connected, show the device icon in the floating audio row. Since the arrow (more_button_) is part of the update. Move the logic of more_button_ to UpdateDeviceTypeAndMore(). Only consider no alternative output here (has_alternative_output()). Since AUDIO_TYPE_HOTWORD, AUDIO_TYPE_POST_MIX_LOOPBACK and AUDIO_TYPE_POST_DSP_LOOPBACK will be treated as alternative input but will not shown in the input detailed view in the ash system tray. BUG=686946 ==========
Please see my comments below. https://codereview.chromium.org/2812223005/diff/1/ash/system/audio/volume_vie... File ash/system/audio/volume_view.cc (right): https://codereview.chromium.org/2812223005/diff/1/ash/system/audio/volume_vie... ash/system/audio/volume_view.cc:3: // found in the LICENSE file. nit: Please wrap the lines in the CL description at 80 characters (this makes it easier for people to read when examining git logs). https://codereview.chromium.org/2812223005/diff/1/ash/system/audio/volume_vie... ash/system/audio/volume_view.cc:166: // None output device is connected 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." https://codereview.chromium.org/2812223005/diff/1/ash/system/audio/volume_vie... ash/system/audio/volume_view.cc:167: if (is_default_view_ && !audio_handler->has_alternative_output()) { 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. https://codereview.chromium.org/2812223005/diff/1/ash/system/audio/volume_vie... ash/system/audio/volume_view.cc:170: more_button_->InvalidateLayout(); What happens if you don't invalidate the layout here (and also on line 207)? https://codereview.chromium.org/2812223005/diff/1/ash/system/audio/volume_vie... ash/system/audio/volume_view.cc:177: if (!is_default_view_) { 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 { ... } https://codereview.chromium.org/2812223005/diff/1/ash/system/audio/volume_vie... ash/system/audio/volume_view.cc:179: if (!target_visibility) { 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? https://codereview.chromium.org/2812223005/diff/1/ash/system/audio/volume_vie... ash/system/audio/volume_view.cc:184: device_type_->SetVisible(false); I'm not sure why you're setting visibility to false here. https://codereview.chromium.org/2812223005/diff/1/ash/system/audio/volume_vie... ash/system/audio/volume_view.cc:185: tri_view_->AddView(TriView::Container::END, device_type_); 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?)
tdanderson@, can you help check my replies? Thanks. https://codereview.chromium.org/2812223005/diff/1/ash/system/audio/volume_vie... File ash/system/audio/volume_view.cc (right): https://codereview.chromium.org/2812223005/diff/1/ash/system/audio/volume_vie... ash/system/audio/volume_view.cc:167: if (is_default_view_ && !audio_handler->has_alternative_output()) { On 2017/04/13 16:15:52, tdanderson wrote: > 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. 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. https://codereview.chromium.org/2812223005/diff/1/ash/system/audio/volume_vie... ash/system/audio/volume_view.cc:170: more_button_->InvalidateLayout(); On 2017/04/13 16:15:52, tdanderson wrote: > What happens if you don't invalidate the layout here (and also on line 207)? 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. https://codereview.chromium.org/2812223005/diff/1/ash/system/audio/volume_vie... ash/system/audio/volume_view.cc:179: if (!target_visibility) { On 2017/04/13 16:15:52, tdanderson wrote: > 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? Yeap, I think so. I moved this from the constructor. Let me do it in next patch. https://codereview.chromium.org/2812223005/diff/1/ash/system/audio/volume_vie... ash/system/audio/volume_view.cc:184: device_type_->SetVisible(false); On 2017/04/13 16:15:52, tdanderson wrote: > I'm not sure why you're setting visibility to false here. 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). https://codereview.chromium.org/2812223005/diff/1/ash/system/audio/volume_vie... ash/system/audio/volume_view.cc:185: tri_view_->AddView(TriView::Container::END, device_type_); On 2017/04/13 16:15:52, tdanderson wrote: > 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?) 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.
Description was changed from ========== Display current output device icon in the floating volume row when switching output devices. Changes in this cl, 1. Remove the arrow when none output device is connected. 2. When some output device is connected, show the device icon in the floating audio row. Since the arrow (more_button_) is part of the update. Move the logic of more_button_ to UpdateDeviceTypeAndMore(). Only consider no alternative output here (has_alternative_output()). Since AUDIO_TYPE_HOTWORD, AUDIO_TYPE_POST_MIX_LOOPBACK and AUDIO_TYPE_POST_DSP_LOOPBACK will be treated as alternative input but will not shown in the input detailed view in the ash system tray. BUG=686946 ========== to ========== Display current output device icon in the floating volume row when switching output devices. Changes in this cl, 1. Remove the arrow when none output device is connected. 2. When some output device is connected, show the device icon in the floating audio row. Since the arrow (more_button_) is part of the update. Move the logic of more_button_ to UpdateDeviceTypeAndMore(). Only consider no alternative output here (has_alternative_output()). Since AUDIO_TYPE_HOTWORD, AUDIO_TYPE_POST_MIX_LOOPBACK and AUDIO_TYPE_POST_DSP_LOOPBACK will be treated as alternative input but will not shown in the input detailed view in the ash system tray. BUG=686946 ==========
Description was changed from ========== Display current output device icon in the floating volume row when switching output devices. Changes in this cl, 1. Remove the arrow when none output device is connected. 2. When some output device is connected, show the device icon in the floating audio row. Since the arrow (more_button_) is part of the update. Move the logic of more_button_ to UpdateDeviceTypeAndMore(). Only consider no alternative output here (has_alternative_output()). Since AUDIO_TYPE_HOTWORD, AUDIO_TYPE_POST_MIX_LOOPBACK and AUDIO_TYPE_POST_DSP_LOOPBACK will be treated as alternative input but will not shown in the input detailed view in the ash system tray. BUG=686946 ========== to ========== Display current output device icon in the floating volume row when switching output devices. Changes in this cl, 1. Remove the arrow when none output device is connected. 2. When some output device is connected, show the device icon in the floating audio row. Since the arrow (more_button_) is part of the update. Move the logic of more_button_ to UpdateDeviceTypeAndMore(). Only consider no alternative output here (has_alternative_output()). Since AUDIO_TYPE_HOTWORD, AUDIO_TYPE_POST_MIX_LOOPBACK and AUDIO_TYPE_POST_DSP_LOOPBACK will be treated as alternative input but will not shown in the input detailed view in the ash system tray. BUG=686946 ==========
https://codereview.chromium.org/2812223005/diff/1/ash/system/audio/volume_vie... File ash/system/audio/volume_view.cc (right): https://codereview.chromium.org/2812223005/diff/1/ash/system/audio/volume_vie... ash/system/audio/volume_view.cc:167: if (is_default_view_ && !audio_handler->has_alternative_output()) { On 2017/04/13 17:47:39, minch1 wrote: > On 2017/04/13 16:15:52, tdanderson wrote: > > 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. > > 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?_ga=1.120203275.202906500.1349447210. 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. 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? https://codereview.chromium.org/2812223005/diff/1/ash/system/audio/volume_vie... ash/system/audio/volume_view.cc:170: more_button_->InvalidateLayout(); On 2017/04/13 17:47:39, minch1 wrote: > On 2017/04/13 16:15:52, tdanderson wrote: > > What happens if you don't invalidate the layout here (and also on line 207)? > > 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. 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. https://codereview.chromium.org/2812223005/diff/1/ash/system/audio/volume_vie... ash/system/audio/volume_view.cc:184: device_type_->SetVisible(false); On 2017/04/13 17:47:39, minch1 wrote: > On 2017/04/13 16:15:52, tdanderson wrote: > > I'm not sure why you're setting visibility to false here. > > 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). 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. https://codereview.chromium.org/2812223005/diff/1/ash/system/audio/volume_vie... ash/system/audio/volume_view.cc:185: tri_view_->AddView(TriView::Container::END, device_type_); On 2017/04/13 17:47:39, minch1 wrote: > On 2017/04/13 16:15:52, tdanderson wrote: > > 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?) > > 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. 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?
The CQ bit was checked by minch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Display current output device icon in the floating volume row when switching output devices. Changes in this cl, 1. Remove the arrow when none output device is connected. 2. When some output device is connected, show the device icon in the floating audio row. Since the arrow (more_button_) is part of the update. Move the logic of more_button_ to UpdateDeviceTypeAndMore(). Only consider no alternative output here (has_alternative_output()). Since AUDIO_TYPE_HOTWORD, AUDIO_TYPE_POST_MIX_LOOPBACK and AUDIO_TYPE_POST_DSP_LOOPBACK will be treated as alternative input but will not shown in the input detailed view in the ash system tray. BUG=686946 ========== to ========== Display current output device icon in the floating volume row when switching output devices. Changes in this cl, 1. Remove the arrow when none output devices are connected. 2. When there is external output device connected, show the device icon in the floating audio row. 3. Make the slider of brightness row in system tray always extend to the end of the row. Keep the same length as the audio slider when none external output / input devices are connected. 4. Input devices type as AUDIO_TYPE_HOTWORD, AUDIO_TYPE_POST_MIX_LOOPBACK and AUDIO_TYPE_POST_DSP_LOOPBACK will not be shown in the UI. Make them as non external input devices. Since the arrow (more_button_) is part of the update. Move the logic of more_button_ to UpdateDeviceTypeAndMore(). BUG=686946 ==========
minch@chromium.org changed reviewers: + jennyz@chromium.org
Description was changed from ========== Display current output device icon in the floating volume row when switching output devices. Changes in this cl, 1. Remove the arrow when none output devices are connected. 2. When there is external output device connected, show the device icon in the floating audio row. 3. Make the slider of brightness row in system tray always extend to the end of the row. Keep the same length as the audio slider when none external output / input devices are connected. 4. Input devices type as AUDIO_TYPE_HOTWORD, AUDIO_TYPE_POST_MIX_LOOPBACK and AUDIO_TYPE_POST_DSP_LOOPBACK will not be shown in the UI. Make them as non external input devices. Since the arrow (more_button_) is part of the update. Move the logic of more_button_ to UpdateDeviceTypeAndMore(). BUG=686946 ========== to ========== Display current output device icon in the floating volume row when switching output devices. Changes in this cl, 1. Remove the arrow when none output devices are connected. 2. When there is external output device connected, show the device icon in the floating audio row. 3. Make the slider of brightness row in system tray always extend to the end of the row. Keep the same length as the audio slider when none external output / input devices are connected. 4. Input devices type as AUDIO_TYPE_HOTWORD, AUDIO_TYPE_POST_MIX_LOOPBACK and AUDIO_TYPE_POST_DSP_LOOPBACK will not be shown in the UI. Make them as non external input devices. Since the arrow (more_button_) is part of the update. Move the logic of more_button_ to UpdateDeviceTypeAndMore(). BUG=686946 ==========
Description was changed from ========== Display current output device icon in the floating volume row when switching output devices. Changes in this cl, 1. Remove the arrow when none output devices are connected. 2. When there is external output device connected, show the device icon in the floating audio row. 3. Make the slider of brightness row in system tray always extend to the end of the row. Keep the same length as the audio slider when none external output / input devices are connected. 4. Input devices type as AUDIO_TYPE_HOTWORD, AUDIO_TYPE_POST_MIX_LOOPBACK and AUDIO_TYPE_POST_DSP_LOOPBACK will not be shown in the UI. Make them as non external input devices. Since the arrow (more_button_) is part of the update. Move the logic of more_button_ to UpdateDeviceTypeAndMore(). BUG=686946 ========== to ========== Display current output device icon in the floating volume row when switching output devices. Changes in this cl, 1. Remove the arrow when none output devices are connected. 2. When there is external output device connected, show the device icon in the floating audio row. 3. Make the slider of brightness row in system tray always extend to the end of the row. Keep the same length as the audio slider when none external output / input devices are connected. 4. Input devices type as AUDIO_TYPE_HOTWORD, AUDIO_TYPE_POST_MIX_LOOPBACK and AUDIO_TYPE_POST_DSP_LOOPBACK will not be shown in the UI. Make them as non external input devices. Since the arrow (more_button_) is part of the update. Move the logic of more_button_ to UpdateDeviceTypeAndMore(). BUG=686946 ==========
Description was changed from ========== Display current output device icon in the floating volume row when switching output devices. Changes in this cl, 1. Remove the arrow when none output devices are connected. 2. When there is external output device connected, show the device icon in the floating audio row. 3. Make the slider of brightness row in system tray always extend to the end of the row. Keep the same length as the audio slider when none external output / input devices are connected. 4. Input devices type as AUDIO_TYPE_HOTWORD, AUDIO_TYPE_POST_MIX_LOOPBACK and AUDIO_TYPE_POST_DSP_LOOPBACK will not be shown in the UI. Make them as non external input devices. Since the arrow (more_button_) is part of the update. Move the logic of more_button_ to UpdateDeviceTypeAndMore(). BUG=686946 ========== to ========== Display current output device icon in the floating volume row when switching output devices. Changes in this cl, 1. Remove the arrow when none output devices are connected. 2. When there is external output device connected, show the device icon in the floating audio row. 3. Make the slider of brightness row in system tray always extend to the end of the row. Keep the same length as the audio slider when none external output / input devices are connected. 4. Input devices type as AUDIO_TYPE_HOTWORD, AUDIO_TYPE_POST_MIX_LOOPBACK and AUDIO_TYPE_POST_DSP_LOOPBACK will not be shown in the UI. Treat them as non external input devices. Since the arrow (more_button_) is part of the update. Move the logic of more_button_ to UpdateDeviceTypeAndMore(). BUG=686946 ==========
Hi, tdanderson@, jennyz@, can you help review? Thanks. https://codereview.chromium.org/2812223005/diff/1/ash/system/audio/volume_vie... File ash/system/audio/volume_view.cc (right): https://codereview.chromium.org/2812223005/diff/1/ash/system/audio/volume_vie... ash/system/audio/volume_view.cc:170: more_button_->InvalidateLayout(); On 2017/04/13 20:54:56, tdanderson wrote: > On 2017/04/13 17:47:39, minch1 wrote: > > On 2017/04/13 16:15:52, tdanderson wrote: > > > What happens if you don't invalidate the layout here (and also on line 207)? > > > > 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. > > 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. 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. https://codereview.chromium.org/2812223005/diff/1/ash/system/audio/volume_vie... ash/system/audio/volume_view.cc:179: if (!target_visibility) { On 2017/04/13 17:47:39, minch1 wrote: > On 2017/04/13 16:15:52, tdanderson wrote: > > 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? > > Yeap, I think so. I moved this from the constructor. Let me do it in next patch. 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. https://codereview.chromium.org/2812223005/diff/1/ash/system/audio/volume_vie... ash/system/audio/volume_view.cc:185: tri_view_->AddView(TriView::Container::END, device_type_); On 2017/04/13 20:54:56, tdanderson wrote: > On 2017/04/13 17:47:39, minch1 wrote: > > On 2017/04/13 16:15:52, tdanderson wrote: > > > 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?) > > > > 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. > > 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? 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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Display current output device icon in the floating volume row when switching output devices. Changes in this cl, 1. Remove the arrow when none output devices are connected. 2. When there is external output device connected, show the device icon in the floating audio row. 3. Make the slider of brightness row in system tray always extend to the end of the row. Keep the same length as the audio slider when none external output / input devices are connected. 4. Input devices type as AUDIO_TYPE_HOTWORD, AUDIO_TYPE_POST_MIX_LOOPBACK and AUDIO_TYPE_POST_DSP_LOOPBACK will not be shown in the UI. Treat them as non external input devices. Since the arrow (more_button_) is part of the update. Move the logic of more_button_ to UpdateDeviceTypeAndMore(). BUG=686946 ========== to ========== Display current output device icon in the floating volume row when switching output devices. Changes in this cl, 1. Remove the arrow when none output devices are connected. 2. When there is external output device connected, show the device icon in the floating audio row. 3. Make the slider of brightness row in system tray always extend to the end of the row. Keep the same length as the audio slider when none external output / input devices are connected. 4. Input devices type as AUDIO_TYPE_HOTWORD, AUDIO_TYPE_POST_MIX_LOOPBACK and AUDIO_TYPE_POST_DSP_LOOPBACK will not be shown in the UI. Treat them as non external input devices. Since the arrow (more_button_) is part of the update. Move the logic of more_button_ to UpdateDeviceTypeAndMore(). BUG=686946 ==========
minch@chromium.org changed reviewers: + xiaoyinh@chromium.org
ash/ LGTM with a few nits addressed. Also, very nice CL description! https://codereview.chromium.org/2812223005/diff/20001/ash/system/audio/volume... File ash/system/audio/volume_view.cc (right): https://codereview.chromium.org/2812223005/diff/20001/ash/system/audio/volume... ash/system/audio/volume_view.cc:167: // there are no alternative output devices present, so do not show the 'more' nit: "if there are no alternative input or output devices present" https://codereview.chromium.org/2812223005/diff/20001/ash/system/audio/volume... ash/system/audio/volume_view.cc:169: if (is_default_view_ && !audio_handler->has_alternative_output() && nit: can you run 'git cl format ash' on this? I think the conditional should be formatted as: if (A && B && C) { } https://codereview.chromium.org/2812223005/diff/20001/ash/system/audio/volume... ash/system/audio/volume_view.cc:200: tri_view_->SetContainerVisible(TriView::Container::END, true); Lines 200-201 and lines 213-214 are identical and appear at the end of both 'if' and 'else' blocks. Can you move that logic to line 216 (outside of the if-else blocks) instead? https://codereview.chromium.org/2812223005/diff/20001/ash/system/brightness/t... File ash/system/brightness/tray_brightness.cc (right): https://codereview.chromium.org/2812223005/diff/20001/ash/system/brightness/t... ash/system/brightness/tray_brightness.cc:121: tri_view->SetContainerVisible(TriView::Container::END, false); Nice catch, thanks!
The CQ bit was checked by minch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi, jennyz@, xiaoyinh@, can you help take a look? Thanks. https://codereview.chromium.org/2812223005/diff/20001/ash/system/audio/volume... File ash/system/audio/volume_view.cc (right): https://codereview.chromium.org/2812223005/diff/20001/ash/system/audio/volume... ash/system/audio/volume_view.cc:169: if (is_default_view_ && !audio_handler->has_alternative_output() && On 2017/04/18 23:50:29, tdanderson wrote: > nit: can you run 'git cl format ash' on this? I think the conditional should be > formatted as: > > if (A && > B && > C) { > } I run 'git cl format' before upload the cl. It still looks like this. I guess it should because the first condition is too short?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2812223005/diff/20001/ash/system/audio/volume... File ash/system/audio/volume_view.cc (right): https://codereview.chromium.org/2812223005/diff/20001/ash/system/audio/volume... ash/system/audio/volume_view.cc:169: if (is_default_view_ && !audio_handler->has_alternative_output() && On 2017/04/19 00:01:41, minch1 wrote: > On 2017/04/18 23:50:29, tdanderson wrote: > > nit: can you run 'git cl format ash' on this? I think the conditional should > be > > formatted as: > > > > if (A && > > B && > > C) { > > } > > I run 'git cl format' before upload the cl. It still looks like this. I guess it > should because the first condition is too short? Ah OK, if git cl format is fine with it, then I'm fine with it too :)
lgtm
lgtm
The CQ bit was checked by minch@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdanderson@chromium.org Link to the patchset: https://codereview.chromium.org/2812223005/#ps40001 (title: "Address nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492655173919880,
"parent_rev": "ecbdd0c68e34edbb28283db17f52cb3fa7f95afc", "commit_rev":
"976586028ad74aceffb12622af62305a1a00c163"}
Message was sent while issue was closed.
Description was changed from ========== Display current output device icon in the floating volume row when switching output devices. Changes in this cl, 1. Remove the arrow when none output devices are connected. 2. When there is external output device connected, show the device icon in the floating audio row. 3. Make the slider of brightness row in system tray always extend to the end of the row. Keep the same length as the audio slider when none external output / input devices are connected. 4. Input devices type as AUDIO_TYPE_HOTWORD, AUDIO_TYPE_POST_MIX_LOOPBACK and AUDIO_TYPE_POST_DSP_LOOPBACK will not be shown in the UI. Treat them as non external input devices. Since the arrow (more_button_) is part of the update. Move the logic of more_button_ to UpdateDeviceTypeAndMore(). BUG=686946 ========== to ========== Display current output device icon in the floating volume row when switching output devices. Changes in this cl, 1. Remove the arrow when none output devices are connected. 2. When there is external output device connected, show the device icon in the floating audio row. 3. Make the slider of brightness row in system tray always extend to the end of the row. Keep the same length as the audio slider when none external output / input devices are connected. 4. Input devices type as AUDIO_TYPE_HOTWORD, AUDIO_TYPE_POST_MIX_LOOPBACK and AUDIO_TYPE_POST_DSP_LOOPBACK will not be shown in the UI. Treat them as non external input devices. Since the arrow (more_button_) is part of the update. Move the logic of more_button_ to UpdateDeviceTypeAndMore(). BUG=686946 Review-Url: https://codereview.chromium.org/2812223005 Cr-Commit-Position: refs/heads/master@{#465877} Committed: https://chromium.googlesource.com/chromium/src/+/976586028ad74aceffb12622af62... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/976586028ad74aceffb12622af62... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
