Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1740)

Issues Search
    My Issues | Starred     Open | Closed | All

Unified Diff: ash/display/display_manager_unittest.cc

Issue 2573673003: Detect and fix overlapping displays (Closed)
Patch Set: Extract Reparent Created 3 years, 11 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « ash/display/display_configuration_controller.cc ('k') | chrome/browser/ui/webui/options/chromeos/display_options_handler.cc » ('j') | ui/display/display_layout.cc » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: ash/display/display_manager_unittest.cc
diff --git a/ash/display/display_manager_unittest.cc b/ash/display/display_manager_unittest.cc
index e6d7313e0ccca7c69c00351217c0e2ff9b88f966..d50984e4d548f64a453ad7f4408448395e78a984 100644
--- a/ash/display/display_manager_unittest.cc
+++ b/ash/display/display_manager_unittest.cc
@@ -362,7 +362,11 @@ TEST_P(DisplayManagerTest, LayoutMorethanThreeDisplaysTest) {
display_manager()->GetDisplayAt(0).bounds().ToString());
EXPECT_EQ("-320,10 320x200",
display_manager()->GetDisplayAt(1).bounds().ToString());
- EXPECT_EQ("-310,-290 400x300",
+
+ // The above layout causes an overlap between [P] and [2], making [2]'s
+ // bounds be "-310,-290 400x300" if the overlap is not fixed. The overlap
+ // must be detected and fixed and [2] is shifted up to remove the overlap.
+ EXPECT_EQ("-310,-300 400x300",
display_manager()->GetDisplayAt(2).bounds().ToString());
}
{
@@ -451,6 +455,295 @@ TEST_P(DisplayManagerTest, LayoutMorethanThreeDisplaysTest) {
}
}
+// Makes sure that layouts with overlapped displays are detected and fixed when
+// applied.
+TEST_P(DisplayManagerTest, NoOverlappedDisplays) {
+ int64_t primary_id = display::Screen::GetScreen()->GetPrimaryDisplay().id();
+ {
+ // Layout with multiple overlaps and special cases:
+ //
+ // +-----+
+ // +----+-+6 |
+ // | 5 | | |
+ // +----+----+ | |
+ // | 7 | | | |
+ // +----+----+-+---+---+-+---------+
+ // | | P | | 2 |
+ // +------+ | | +----------+
+ // | | | | 3 |
+ // | | | | |
+ // +--+----+-+-+-+-----+--+ |
+ // | 1 | | 4 | | |
+ // | | | +--+-------+
+ // | | | |
+ // +--------+ +--------+
+
+ display::DisplayIdList list = display::test::CreateDisplayIdListN(
+ 8, primary_id, primary_id + 1, primary_id + 2, primary_id + 3,
+ primary_id + 4, primary_id + 5, primary_id + 6, primary_id + 7);
+ display::DisplayLayoutBuilder builder(primary_id);
+ builder.AddDisplayPlacement(list[1], primary_id,
+ display::DisplayPlacement::BOTTOM, 50);
+ builder.AddDisplayPlacement(list[2], list[1],
+ display::DisplayPlacement::TOP, 300);
+ builder.AddDisplayPlacement(list[3], list[2],
+ display::DisplayPlacement::RIGHT, 30);
+ builder.AddDisplayPlacement(list[4], list[2],
+ display::DisplayPlacement::BOTTOM, 400);
+ builder.AddDisplayPlacement(list[5], primary_id,
+ display::DisplayPlacement::LEFT, -300);
+ builder.AddDisplayPlacement(list[6], primary_id,
+ display::DisplayPlacement::TOP, -250);
+ builder.AddDisplayPlacement(list[7], list[6],
+ display::DisplayPlacement::LEFT, 250);
+ display_manager()->layout_store()->RegisterLayoutForDisplayIdList(
+ list, builder.Build());
+
+ UpdateDisplay(
+ "480x400,480x400,480x400,480x400,480x400,480x400,480x400,530x150");
+
+ // The resulting layout after overlaps had been removed:
+ //
+ //
+ // +---------+
+ // | 7 +-----+
+ // +-+-------+ 6 |
+ // | 5 | |
+ // | | |
+ // | | |
+ // | |-+---+----+---------+
+ // | | | P | 2 |
+ // +-------+ | | +----------+
+ // | | | 3 |
+ // | | | |
+ // +--+-----+-+-------+ |
+ // | 1 | | |
+ // | | +----+---+------+
+ // | | | 4 |
+ // +-------+ | |
+ // | |
+ // +--------+
+
+ EXPECT_EQ(8U, display_manager()->GetNumDisplays());
+
+ EXPECT_EQ("0,0 480x400",
+ display_manager()->GetDisplayAt(0).bounds().ToString());
+ EXPECT_EQ("50,400 480x400",
+ display_manager()->GetDisplayAt(1).bounds().ToString());
+ EXPECT_EQ("480,0 480x400",
+ display_manager()->GetDisplayAt(2).bounds().ToString());
+ EXPECT_EQ("960,30 480x400",
+ display_manager()->GetDisplayAt(3).bounds().ToString());
+ EXPECT_EQ("730,430 480x400",
+ display_manager()->GetDisplayAt(4).bounds().ToString());
+ EXPECT_EQ("-730,-300 480x400",
+ display_manager()->GetDisplayAt(5).bounds().ToString());
+ EXPECT_EQ("-250,-400 480x400",
+ display_manager()->GetDisplayAt(6).bounds().ToString());
+ EXPECT_EQ("-780,-450 530x150",
+ display_manager()->GetDisplayAt(7).bounds().ToString());
+
+ // Expect that the displays have been reparented correctly, such that a
+ // child is always touching its parent.
+ display::DisplayLayoutBuilder expected_layout_builder(primary_id);
+ expected_layout_builder.AddDisplayPlacement(
+ list[1], primary_id, display::DisplayPlacement::BOTTOM, 50);
+ expected_layout_builder.AddDisplayPlacement(
+ list[2], list[1], display::DisplayPlacement::TOP, 430);
+ expected_layout_builder.AddDisplayPlacement(
+ list[3], list[2], display::DisplayPlacement::RIGHT, 30);
+ // [4] became a child of [3] instead of [2] as they no longer touch.
+ expected_layout_builder.AddDisplayPlacement(
+ list[4], list[3], display::DisplayPlacement::BOTTOM, -230);
+ // [5] became a child of [6] instead of [P] as they no longer touch.
+ expected_layout_builder.AddDisplayPlacement(
+ list[5], list[6], display::DisplayPlacement::LEFT, 100);
+ expected_layout_builder.AddDisplayPlacement(
+ list[6], primary_id, display::DisplayPlacement::TOP, -250);
+ expected_layout_builder.AddDisplayPlacement(
+ list[7], list[6], display::DisplayPlacement::LEFT, -50);
+
+ const display::DisplayLayout& layout =
+ display_manager()->GetCurrentResolvedDisplayLayout();
+
+ EXPECT_TRUE(
+ layout.HasSamePlacementList(*(expected_layout_builder.Build())));
+ }
+
+ {
+ // The following is a special case where a child display is closer to the
+ // origin than its parent. Test that we can handle it successfully without
+ // introducing a circular dependency.
+ //
+ // +---------+
+ // | P | +---------+
+ // | | | 3 |
+ // | | | |
+ // | | | |
+ // +------+--+----+ |
+ // | 1 | | 2 +---------+
+ // | | | |
+ // | | | |
+ // | | | |
+ // +------+--+----+
+ //
+
+ display::DisplayIdList list = display::test::CreateDisplayIdListN(
+ 4, primary_id, primary_id + 1, primary_id + 2, primary_id + 3);
+ display::DisplayLayoutBuilder builder(primary_id);
+ builder.AddDisplayPlacement(list[1], primary_id,
+ display::DisplayPlacement::BOTTOM, 0);
+ builder.AddDisplayPlacement(list[2], primary_id,
+ display::DisplayPlacement::BOTTOM, 464);
+ builder.AddDisplayPlacement(list[3], list[2],
+ display::DisplayPlacement::RIGHT, -700);
+ display_manager()->layout_store()->RegisterLayoutForDisplayIdList(
+ list, builder.Build());
+ UpdateDisplay("696x800,696x800,300x800,696x800");
+
+ // The expected layout should be:
+ //
+ // +---------+
+ // | P | +---------+
+ // | | | 3 |
+ // | | | |
+ // | | | |
+ // +---------+-------+ |
+ // | 1 | 2 +---------+
+ // | | |
+ // | | |
+ // | | |
+ // +---------+-------+
+ //
+ //
+
+ EXPECT_EQ(4U, display_manager()->GetNumDisplays());
+ EXPECT_EQ("0,0 696x800",
+ display_manager()->GetDisplayAt(0).bounds().ToString());
+ EXPECT_EQ("0,800 696x800",
+ display_manager()->GetDisplayAt(1).bounds().ToString());
+ EXPECT_EQ("696,800 300x800",
+ display_manager()->GetDisplayAt(2).bounds().ToString());
+ EXPECT_EQ("996,100 696x800",
+ display_manager()->GetDisplayAt(3).bounds().ToString());
+
+ // This case if not handled correctly might lead to a cyclic dependency.
+ // Make sure this doesn't happen.
+ display::DisplayLayoutBuilder expected_layout_builder(primary_id);
+ expected_layout_builder.AddDisplayPlacement(
+ list[1], primary_id, display::DisplayPlacement::BOTTOM, 0);
+ expected_layout_builder.AddDisplayPlacement(
+ list[2], primary_id, display::DisplayPlacement::BOTTOM, 696);
+ expected_layout_builder.AddDisplayPlacement(
+ list[3], list[2], display::DisplayPlacement::RIGHT, -700);
+
+ const display::DisplayLayout& layout =
+ display_manager()->GetCurrentResolvedDisplayLayout();
+ EXPECT_TRUE(
+ layout.HasSamePlacementList(*(expected_layout_builder.Build())));
+ }
+
+ {
+ // The following is a layout with an overlap to the left of the primary
+ // display.
+ //
+ // +---------+---------+
+ // | 1 | P |
+ // | | |
+ // +---------+ |
+ // | | |
+ // +---------+---------+
+ // | 2 |
+ // | |
+ // +---------+
+
+ display::DisplayIdList list = display::test::CreateDisplayIdListN(
+ 3, primary_id, primary_id + 1, primary_id + 2);
+ display::DisplayLayoutBuilder builder(primary_id);
+ builder.AddDisplayPlacement(list[1], primary_id,
+ display::DisplayPlacement::LEFT, 0);
+ builder.AddDisplayPlacement(list[2], primary_id,
+ display::DisplayPlacement::LEFT, 250);
+ display_manager()->layout_store()->RegisterLayoutForDisplayIdList(
+ list, builder.Build());
+ UpdateDisplay("696x500,696x500,696x500");
+
+ // The expected layout should be:
+ //
+ // +---------+---------+
+ // | 1 | P |
+ // | | |
+ // | | |
+ // | | |
+ // +---------+---------+
+ // | 2 |
+ // | |
+ // | |
+ // | |
+ // +---------+
+
+ EXPECT_EQ(3U, display_manager()->GetNumDisplays());
+ EXPECT_EQ("0,0 696x500",
+ display_manager()->GetDisplayAt(0).bounds().ToString());
+ EXPECT_EQ("-696,0 696x500",
+ display_manager()->GetDisplayAt(1).bounds().ToString());
+ EXPECT_EQ("-696,500 696x500",
+ display_manager()->GetDisplayAt(2).bounds().ToString());
+ }
+
+ {
+ // The following is a layout with an overlap occuring above the primary
+ // display.
+ //
+ // +------+--+------+
+ // | 2 | | 1 |
+ // | | | |
+ // | | | |
+ // | | | |
+ // +------+--+------+
+ // | P |
+ // | |
+ // | |
+ // | |
+ // +---------+
+ //
+
+ display::DisplayIdList list = display::test::CreateDisplayIdListN(
+ 3, primary_id, primary_id + 1, primary_id + 2);
+ display::DisplayLayoutBuilder builder(primary_id);
+ builder.AddDisplayPlacement(list[1], primary_id,
+ display::DisplayPlacement::TOP, 0);
+ builder.AddDisplayPlacement(list[2], primary_id,
+ display::DisplayPlacement::TOP, -348);
+ display_manager()->layout_store()->RegisterLayoutForDisplayIdList(
+ list, builder.Build());
+ UpdateDisplay("696x500,696x500,696x500");
+
+ // The expected layout should be:
+ //
+ // +---------+---------+
+ // | 2 | 1 |
+ // | | |
+ // | | |
+ // | | |
+ // +---------+---------+
+ // | P |
+ // | |
+ // | |
+ // | |
+ // +---------+
+ //
+
+ EXPECT_EQ(3U, display_manager()->GetNumDisplays());
+ EXPECT_EQ("0,0 696x500",
+ display_manager()->GetDisplayAt(0).bounds().ToString());
+ EXPECT_EQ("0,-500 696x500",
+ display_manager()->GetDisplayAt(1).bounds().ToString());
+ EXPECT_EQ("-696,-500 696x500",
+ display_manager()->GetDisplayAt(2).bounds().ToString());
+ }
oshima 2017/01/31 16:55:43 can you also add the scenario like this [1][2][3
afakhry 2017/01/31 19:19:32 Done.
+}
+
TEST_P(DisplayManagerTest, NoMirrorInThreeDisplays) {
UpdateDisplay("640x480,320x200,400x300");
ash::Shell::GetInstance()->display_configuration_controller()->SetMirrorMode(
« no previous file with comments | « ash/display/display_configuration_controller.cc ('k') | chrome/browser/ui/webui/options/chromeos/display_options_handler.cc » ('j') | ui/display/display_layout.cc » ('J')

Powered by Google App Engine
This is Rietveld 408576698