New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 785416 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug

Blocking:
issue 794831
issue 607844



Sign in to add a comment

Enhancement to Launch bug: Mirroring Across 2 and more external displays

Project Member Reported by ovanieva@chromium.org, Nov 15 2017

Issue description

This is an enhancement for launch bug: 776442.

Under current implementation whenever user plugs in Display A and chooses to Mirror; whenever he/she plugs in second Display B, all displays go into Extended Mode. 

Instead, please ensure that once user chose to Mirror to one display and connects additional displays, Mirroring is applies across all displays.


 
Status: Started (was: Assigned)
Blocking: 607844
Components: UI>Shell>MultipleMonitor
Labels: M-64 Multi-Display-Mirroring OS-Chrome

Comment 3 by osh...@chromium.org, Nov 16 2017

This was by design because we assumed that what you want depends on physical display settings. You may want to use mirroring when presenting, but most likely want to use extended when you come back to your desk.

If we want to do this, then we may want to change the settings UI to control the mirroring even without external display so that you can set which mode you want when the display is connected.

This implementation mostly covers kiosk use cases- where mirroring will be applied across all display right away. For desktop use case: my rational is that when user has mirroring set to one external display, and plugs in another, one would assume they would like to continue mirroring. Going back to default settings of extended display will add extra step to the user. I don't think we need UI control to mirror when no external display is detected - that would prove to be confusing. happy to chat.

Comment 5 by osh...@chromium.org, Nov 22 2017

The current CL does more than what you described. The mirroring is now device settings, not per displays. That is, 

1) you connect display A.
2) switch to mirror
3) disconnect display A
4) connect display B.
5) you're in mirror mode


Just want to make sure what's what you want.
Yes, that's right. The mirror mode should be on if there are 2+ display and the user turns on mirror mode. Otherwise, mirror mode should be off.

Comment 7 by osh...@chromium.org, Nov 22 2017

What I meant is that now the mirroring is per device, you can turn off it before you connect a display. You may not want to show what's on your screen when you project, for example.

Comment 8 by oshima@google.com, Nov 22 2017

By the way, I'm fine if we decided not to change UI. My question was if #5 is what we want.
Cc: osh...@chromium.org
Do you think we could show the mirror mode checkbox even when there's only one display? In the scenario in #5, the user could turn off mirror mode before 4) if the user does not want to mirror B.
ovanieva@, does the scenario in #5 sound reasonable to you?
what I described is not what oshima@ listed in comment 5.
what i meant is:
1) you connect display A.
2) switch to mirror
3) connect display B.
4) you're in mirror mode

there is no step of disconnecting display A. 

Scenario in Comment 9 does not make sense - mirroring option will appear only if 1 or more external displays connected. 
My ask for the bug is only for cases when 1 external display is mirroring. Whenever another display is added -mirroring should persist. 

let me know if that makes sense


the desired implementation has been agreed and signed off by UI review - please let us know when we can get OK on the Cl.
My question is what I described in #5 is what you want? If not, then we should discuss because that's the side effect this bug will cause.
Status: WontFix (was: Started)
After reviewing the impact of this change earlier today, let's revert to first implementation in crbug/776442. I'm closing this bug as won't fix. 
Just got this bug. 

Olga is right, what is described in #11 makes sense to me.
Oshima is right too that what is described in #5 is not what we should do. 

Are you saying we can't do #11 without getting #5? The problem is that I find going from mirrored to extended weird when I just attach a new devices (again, this is only concerning the 3 monitor case, for other cases, starting out with extended for new displays is reasonable)
It should be possible although it has some side effects. Let's have a quick chat tomorrow morning.
Status: Started (was: WontFix)
Reopen this bug after offline discussion:
The rule for turning off/on mirror after display configuration changes (i.e. remove/add a display) is:
current_mirror_mode := previous_mirror_mode AND num_of_displays > 1
Cc: jennschen@google.com kuscher@chromium.org
after multiple chats summarizing our latest agreement:

User is connected to External Display A.
User turns on Mirroring. 
User disconnects from External Display A and reconnects back to A.
RESULT: Mirroring on A.

User is connected to External Display A.
User turns on Mirroring.
User connects second External Display B.
RESULT: Mirroring on A and B.

User is connected to External Display A. 
User turns on Mirroring.
User disconnects from External Display A and connects to External Display C. 
RESULT: Extended Mode on C. 

User is connected to External Display A. 
User turns on Mirroring.
User turns off MIrroring.
User connects to External Display B.
RESULT: Extended Mode on B.


Boot with A, which was previously mirrored, and B, which was previously extended
RESULT: Mirroring on A. 

Blocking: 794831
Project Member

Comment 20 by bugdroid1@chromium.org, Dec 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fa8dfaa621c1701dce841f76effecd064ff3753e

commit fa8dfaa621c1701dce841f76effecd064ff3753e
Author: Weidong Guo <weidongg@chromium.org>
Date: Sun Dec 17 20:43:27 2017

Display mirroring persistency changes

Changes:
1. Change mirroring persistency rule to:
If previous mirror mode is not set (e.g. no external display in previous
configuration), turn on mirror mode if at least one of the external
displays was in mirror mode before and turn off mirror mode otherwise.
If previous mirror mode is set, mirror mode remains its previous value
as long as there are more than 1 displays.
For example:
Case 1: Connect display A; Turn on mirror mode; Connect display B. B
should be in mirror mode as well.
Case 2: Connect display A; Turn on mirror mode; Remove A; Reconnect A; A
should be in mirror mode.
Case 3: Connect display A; Turn on mirror mode; Remove A; Connect
display B. B should not be in mirror mode.
Case 4: Connect display A; Turn on mirror mode; Connect display B;
Remove B; Remove A; Reconnect B; B should be in mirror mode.

2. Fix a bug in MirrorWindowController: mirroring source display may be
changed in mirror mode (e.g. Connect two external displays; Turn on
mirror mode; Close internal display lid; Then the internal display will
be replaced with one external display as the mirroring source.),
reflector and mirror windows should be updated in this case.

3. Fix broken test.

Bug:  785416 ,792207
Test: MultiMirroringTest.*
Change-Id: Ia1da9f25df2bcc04f98ab087478a14685fff7708
Reviewed-on: https://chromium-review.googlesource.com/804647
Commit-Queue: Weidong Guo <weidongg@chromium.org>
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524627}
[modify] https://crrev.com/fa8dfaa621c1701dce841f76effecd064ff3753e/ash/display/display_manager_unittest.cc
[modify] https://crrev.com/fa8dfaa621c1701dce841f76effecd064ff3753e/ash/display/mirror_window_controller.cc
[modify] https://crrev.com/fa8dfaa621c1701dce841f76effecd064ff3753e/ash/display/mirror_window_controller.h
[modify] https://crrev.com/fa8dfaa621c1701dce841f76effecd064ff3753e/ash/display/mirror_window_controller_unittest.cc
[modify] https://crrev.com/fa8dfaa621c1701dce841f76effecd064ff3753e/ash/display/root_window_transformers_unittest.cc
[modify] https://crrev.com/fa8dfaa621c1701dce841f76effecd064ff3753e/ash/system/screen_layout_observer_unittest.cc
[modify] https://crrev.com/fa8dfaa621c1701dce841f76effecd064ff3753e/ash/system/web_notification/web_notification_tray_unittest.cc
[modify] https://crrev.com/fa8dfaa621c1701dce841f76effecd064ff3753e/ash/touch/touch_observer_hud_unittest.cc
[modify] https://crrev.com/fa8dfaa621c1701dce841f76effecd064ff3753e/chrome/browser/chromeos/display/display_preferences.cc
[modify] https://crrev.com/fa8dfaa621c1701dce841f76effecd064ff3753e/chrome/browser/chromeos/display/display_preferences_unittest.cc
[modify] https://crrev.com/fa8dfaa621c1701dce841f76effecd064ff3753e/chrome/common/pref_names.cc
[modify] https://crrev.com/fa8dfaa621c1701dce841f76effecd064ff3753e/chrome/common/pref_names.h
[modify] https://crrev.com/fa8dfaa621c1701dce841f76effecd064ff3753e/ui/display/display_layout.h
[modify] https://crrev.com/fa8dfaa621c1701dce841f76effecd064ff3753e/ui/display/manager/chromeos/display_change_observer.cc
[modify] https://crrev.com/fa8dfaa621c1701dce841f76effecd064ff3753e/ui/display/manager/chromeos/display_configurator.cc
[modify] https://crrev.com/fa8dfaa621c1701dce841f76effecd064ff3753e/ui/display/manager/display_layout_store.h
[modify] https://crrev.com/fa8dfaa621c1701dce841f76effecd064ff3753e/ui/display/manager/display_manager.cc
[modify] https://crrev.com/fa8dfaa621c1701dce841f76effecd064ff3753e/ui/display/manager/display_manager.h
[modify] https://crrev.com/fa8dfaa621c1701dce841f76effecd064ff3753e/ui/display/manager/display_manager_utilities.cc
[modify] https://crrev.com/fa8dfaa621c1701dce841f76effecd064ff3753e/ui/display/manager/display_manager_utilities.h
[modify] https://crrev.com/fa8dfaa621c1701dce841f76effecd064ff3753e/ui/display/types/display_constants.h

Project Member

Comment 21 by bugdroid1@chromium.org, Dec 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ba99d1924ab00321c5faca2d61957fdebb37c89d

commit ba99d1924ab00321c5faca2d61957fdebb37c89d
Author: Findit <findit-for-me@appspot.gserviceaccount.com>
Date: Sun Dec 17 21:47:26 2017

Revert "Display mirroring persistency changes"

This reverts commit fa8dfaa621c1701dce841f76effecd064ff3753e.

Reason for revert:

Findit (https://goo.gl/kROfz5) identified CL at revision 524627 as the
culprit for failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyRAsSDVdmU3VzcGVjdGVkQ0wiMWNocm9taXVtL2ZhOGRmYWE2MjFjMTcwMWRjZTg0MWY3NmVmZmVjZDA2NGZmMzc1M2UM

Sample Failed Build: https://ci.chromium.org/buildbot/chromium.chromiumos/linux-chromeos-dbg/3252

Original change's description:
> Display mirroring persistency changes
> 
> Changes:
> 1. Change mirroring persistency rule to:
> If previous mirror mode is not set (e.g. no external display in previous
> configuration), turn on mirror mode if at least one of the external
> displays was in mirror mode before and turn off mirror mode otherwise.
> If previous mirror mode is set, mirror mode remains its previous value
> as long as there are more than 1 displays.
> For example:
> Case 1: Connect display A; Turn on mirror mode; Connect display B. B
> should be in mirror mode as well.
> Case 2: Connect display A; Turn on mirror mode; Remove A; Reconnect A; A
> should be in mirror mode.
> Case 3: Connect display A; Turn on mirror mode; Remove A; Connect
> display B. B should not be in mirror mode.
> Case 4: Connect display A; Turn on mirror mode; Connect display B;
> Remove B; Remove A; Reconnect B; B should be in mirror mode.
> 
> 2. Fix a bug in MirrorWindowController: mirroring source display may be
> changed in mirror mode (e.g. Connect two external displays; Turn on
> mirror mode; Close internal display lid; Then the internal display will
> be replaced with one external display as the mirroring source.),
> reflector and mirror windows should be updated in this case.
> 
> 3. Fix broken test.
> 
> Bug:  785416 ,792207
> Test: MultiMirroringTest.*
> Change-Id: Ia1da9f25df2bcc04f98ab087478a14685fff7708
> Reviewed-on: https://chromium-review.googlesource.com/804647
> Commit-Queue: Weidong Guo <weidongg@chromium.org>
> Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
> Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#524627}

Change-Id: Ie18b8dc47d031984590ed98af331fdb659b17941
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  785416 ,792207
Reviewed-on: https://chromium-review.googlesource.com/831290
Cr-Commit-Position: refs/heads/master@{#524629}
[modify] https://crrev.com/ba99d1924ab00321c5faca2d61957fdebb37c89d/ash/display/display_manager_unittest.cc
[modify] https://crrev.com/ba99d1924ab00321c5faca2d61957fdebb37c89d/ash/display/mirror_window_controller.cc
[modify] https://crrev.com/ba99d1924ab00321c5faca2d61957fdebb37c89d/ash/display/mirror_window_controller.h
[modify] https://crrev.com/ba99d1924ab00321c5faca2d61957fdebb37c89d/ash/display/mirror_window_controller_unittest.cc
[modify] https://crrev.com/ba99d1924ab00321c5faca2d61957fdebb37c89d/ash/display/root_window_transformers_unittest.cc
[modify] https://crrev.com/ba99d1924ab00321c5faca2d61957fdebb37c89d/ash/system/screen_layout_observer_unittest.cc
[modify] https://crrev.com/ba99d1924ab00321c5faca2d61957fdebb37c89d/ash/system/web_notification/web_notification_tray_unittest.cc
[modify] https://crrev.com/ba99d1924ab00321c5faca2d61957fdebb37c89d/ash/touch/touch_observer_hud_unittest.cc
[modify] https://crrev.com/ba99d1924ab00321c5faca2d61957fdebb37c89d/chrome/browser/chromeos/display/display_preferences.cc
[modify] https://crrev.com/ba99d1924ab00321c5faca2d61957fdebb37c89d/chrome/browser/chromeos/display/display_preferences_unittest.cc
[modify] https://crrev.com/ba99d1924ab00321c5faca2d61957fdebb37c89d/chrome/common/pref_names.cc
[modify] https://crrev.com/ba99d1924ab00321c5faca2d61957fdebb37c89d/chrome/common/pref_names.h
[modify] https://crrev.com/ba99d1924ab00321c5faca2d61957fdebb37c89d/ui/display/display_layout.h
[modify] https://crrev.com/ba99d1924ab00321c5faca2d61957fdebb37c89d/ui/display/manager/chromeos/display_change_observer.cc
[modify] https://crrev.com/ba99d1924ab00321c5faca2d61957fdebb37c89d/ui/display/manager/chromeos/display_configurator.cc
[modify] https://crrev.com/ba99d1924ab00321c5faca2d61957fdebb37c89d/ui/display/manager/display_layout_store.h
[modify] https://crrev.com/ba99d1924ab00321c5faca2d61957fdebb37c89d/ui/display/manager/display_manager.cc
[modify] https://crrev.com/ba99d1924ab00321c5faca2d61957fdebb37c89d/ui/display/manager/display_manager.h
[modify] https://crrev.com/ba99d1924ab00321c5faca2d61957fdebb37c89d/ui/display/manager/display_manager_utilities.cc
[modify] https://crrev.com/ba99d1924ab00321c5faca2d61957fdebb37c89d/ui/display/manager/display_manager_utilities.h
[modify] https://crrev.com/ba99d1924ab00321c5faca2d61957fdebb37c89d/ui/display/types/display_constants.h

Project Member

Comment 22 by bugdroid1@chromium.org, Dec 18 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/78d6fb65c66a4c6f2aecc882768a9002aaa8aeae

commit 78d6fb65c66a4c6f2aecc882768a9002aaa8aeae
Author: Weidong Guo <weidongg@chromium.org>
Date: Mon Dec 18 01:47:56 2017

Reland "Display mirroring persistency changes"

This is a reland of fa8dfaa621c1701dce841f76effecd064ff3753e
Original change's description:
> Display mirroring persistency changes
>
> Changes:
> 1. Change mirroring persistency rule to:
> If previous mirror mode is not set (e.g. no external display in previous
> configuration), turn on mirror mode if at least one of the external
> displays was in mirror mode before and turn off mirror mode otherwise.
> If previous mirror mode is set, mirror mode remains its previous value
> as long as there are more than 1 displays.
> For example:
> Case 1: Connect display A; Turn on mirror mode; Connect display B. B
> should be in mirror mode as well.
> Case 2: Connect display A; Turn on mirror mode; Remove A; Reconnect A; A
> should be in mirror mode.
> Case 3: Connect display A; Turn on mirror mode; Remove A; Connect
> display B. B should not be in mirror mode.
> Case 4: Connect display A; Turn on mirror mode; Connect display B;
> Remove B; Remove A; Reconnect B; B should be in mirror mode.
>
> 2. Fix a bug in MirrorWindowController: mirroring source display may be
> changed in mirror mode (e.g. Connect two external displays; Turn on
> mirror mode; Close internal display lid; Then the internal display will
> be replaced with one external display as the mirroring source.),
> reflector and mirror windows should be updated in this case.
>
> 3. Fix broken test.
>
> Bug:  785416 ,792207
> Test: MultiMirroringTest.*
> Change-Id: Ia1da9f25df2bcc04f98ab087478a14685fff7708
> Reviewed-on: https://chromium-review.googlesource.com/804647
> Commit-Queue: Weidong Guo <weidongg@chromium.org>
> Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
> Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#524627}

TBR=afakhry@chromium.org, oshima@chromium.org

Bug:  785416 , 792207
Change-Id: I6f0c7b4169f5a0e0ab8a8f97a073cd165d21622d
Reviewed-on: https://chromium-review.googlesource.com/831666
Reviewed-by: Weidong Guo <weidongg@chromium.org>
Commit-Queue: Weidong Guo <weidongg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524636}
[modify] https://crrev.com/78d6fb65c66a4c6f2aecc882768a9002aaa8aeae/ash/display/display_manager_unittest.cc
[modify] https://crrev.com/78d6fb65c66a4c6f2aecc882768a9002aaa8aeae/ash/display/mirror_window_controller.cc
[modify] https://crrev.com/78d6fb65c66a4c6f2aecc882768a9002aaa8aeae/ash/display/mirror_window_controller.h
[modify] https://crrev.com/78d6fb65c66a4c6f2aecc882768a9002aaa8aeae/ash/display/mirror_window_controller_unittest.cc
[modify] https://crrev.com/78d6fb65c66a4c6f2aecc882768a9002aaa8aeae/ash/display/root_window_transformers_unittest.cc
[modify] https://crrev.com/78d6fb65c66a4c6f2aecc882768a9002aaa8aeae/ash/system/screen_layout_observer_unittest.cc
[modify] https://crrev.com/78d6fb65c66a4c6f2aecc882768a9002aaa8aeae/ash/system/web_notification/web_notification_tray_unittest.cc
[modify] https://crrev.com/78d6fb65c66a4c6f2aecc882768a9002aaa8aeae/ash/touch/touch_observer_hud_unittest.cc
[modify] https://crrev.com/78d6fb65c66a4c6f2aecc882768a9002aaa8aeae/chrome/browser/chromeos/display/display_preferences.cc
[modify] https://crrev.com/78d6fb65c66a4c6f2aecc882768a9002aaa8aeae/chrome/browser/chromeos/display/display_preferences_unittest.cc
[modify] https://crrev.com/78d6fb65c66a4c6f2aecc882768a9002aaa8aeae/chrome/common/pref_names.cc
[modify] https://crrev.com/78d6fb65c66a4c6f2aecc882768a9002aaa8aeae/chrome/common/pref_names.h
[modify] https://crrev.com/78d6fb65c66a4c6f2aecc882768a9002aaa8aeae/ui/display/display_layout.h
[modify] https://crrev.com/78d6fb65c66a4c6f2aecc882768a9002aaa8aeae/ui/display/manager/chromeos/display_change_observer.cc
[modify] https://crrev.com/78d6fb65c66a4c6f2aecc882768a9002aaa8aeae/ui/display/manager/chromeos/display_configurator.cc
[modify] https://crrev.com/78d6fb65c66a4c6f2aecc882768a9002aaa8aeae/ui/display/manager/display_layout_store.h
[modify] https://crrev.com/78d6fb65c66a4c6f2aecc882768a9002aaa8aeae/ui/display/manager/display_manager.cc
[modify] https://crrev.com/78d6fb65c66a4c6f2aecc882768a9002aaa8aeae/ui/display/manager/display_manager.h
[modify] https://crrev.com/78d6fb65c66a4c6f2aecc882768a9002aaa8aeae/ui/display/manager/display_manager_utilities.cc
[modify] https://crrev.com/78d6fb65c66a4c6f2aecc882768a9002aaa8aeae/ui/display/manager/display_manager_utilities.h
[modify] https://crrev.com/78d6fb65c66a4c6f2aecc882768a9002aaa8aeae/ui/display/types/display_constants.h

Status: Fixed (was: Started)

Sign in to add a comment