Merge lp:~alan-griffiths/mir/improved-Rectangles into lp:mir

[Alan Griffiths]

geometry: add a remove() method to the Rectangles class

geometry: add a remove() method to the Rectangles class

This provides better support for tracking e.g. the display rectangles

[Alberto Aguirre (albaguirre)]

LGTM

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2245
http://jenkins.qa.ubuntu.com/job/mir-ci/2745/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/971
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/971
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/933
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/742
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/742/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/933
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/933/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4029
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/17292

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/2745/rebuild

review: Approve (continuous-integration)

[Kevin DuBois (kdub)]

It is was somewhat unclear what remove does if you add multiple same-sized-rectangles, maybe a test that checks that all rectangles of the same size are removed (or never double-added)?

review: Needs Fixing

[Kevin DuBois (kdub)]

> It is was somewhat unclear what remove does if you add multiple same-sized-
*It is somewhat...
https://www.youtube.com/watch?v=KzyCi1BFATA

[Daniel van Vugt (vanvugt)]

This potentially makes us sensitive to the order in which tests are run, so should probably be undone:

173 +struct TestRectangles : Test
174 +{
175 + Rectangles rectangles;

202 - Rectangles rectangles;
203 -
...

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

I think what Kevin is asking for is a: std::set<Rectangle>

[Daniel van Vugt (vanvugt)]

Hmm, why don't we just have a set<Rectangle> or unordered_set<Rectangle> ?

The required comparison operator for Rectangle to make it work would be simple to add.

[Alexandros Frantzis (afrantzis)]

Looks good.

> It is somewhat unclear what remove does if you add multiple same-sized-rectangles,
> maybe a test that checks that all rectangles of the same size are removed

It would be good to have a test and comment for this.

> This potentially makes us sensitive to the order in which tests are run,
> so should probably be undone:
>
> 173 +struct TestRectangles : Test
> 174 +{
> 175 + Rectangles rectangles;

A new fixture instance is created for each test case.

> Hmm, why don't we just have a set<Rectangle> or unordered_set<Rectangle> ?

Rectangles is designed to hold multiple same-sized-rectangles. We could use a (unordered_)multiset, but in terms of performance it's probably an overkill (vs vector) for the sizes we care about. I am fine either way.

review: Approve

[Alan Griffiths (alan-griffiths)]

> Looks good.
>
> > It is somewhat unclear what remove does if you add multiple same-sized-
> rectangles,
> > maybe a test that checks that all rectangles of the same size are removed
>
> It would be good to have a test and comment for this.

Done

[Alan Griffiths (alan-griffiths)]

> This potentially makes us sensitive to the order in which tests are run, so
> should probably be undone:
>
> 173 +struct TestRectangles : Test
> 174 +{
> 175 + Rectangles rectangles;
>
> 202 - Rectangles rectangles;
> 203 -
> ...

Oh no it doesn't.

[Alexandros Frantzis (afrantzis)]

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/mir-autolanding/1047/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1008
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1007/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/969
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-autolanding/227
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-autolanding/227/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/969
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/969/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4056
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/17352

review: Needs Fixing (continuous-integration)

[Kevin DuBois (kdub)]

The intended behavior is clearer with the tests, so lgtm!

review: Approve

[Alan Griffiths (alan-griffiths)]

[ 71%] Building CXX object tests/integration-tests/CMakeFiles/mir_integration_tests.dir/test_session_manager.cpp.o
FATAL: Unable to delete script file /tmp/hudson7970247705980537602.sh
hudson.util.IOException2: remote file operation failed: /tmp/hudson7970247705980537602.sh at hudson.remoting.Channel@3f39e40c:cloud-worker-10

reapproving

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2248
http://jenkins.qa.ubuntu.com/job/mir-ci/2775/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1011
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1010
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/972
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/772
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/772/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/972
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/972/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4059
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/17359

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/2775/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/mir-autolanding/1051/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1017
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1016
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/978/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-autolanding/231
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-autolanding/231/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/978/console

review: Needs Fixing (continuous-integration)

[Daniel van Vugt (vanvugt)]

I meant this increases the future risk of us becoming sensitive to the order of test case execution. Even if the current test cases guard against that, it's more defensive to make it impossible for future maintainers to make such a mistake. And would provide higher cohesion of tests too...

192 +struct TestRectangles : Test
193 +{
194 + Rectangles rectangles;

So TEST_F becomes TEST again.

[Alan Griffiths (alan-griffiths)]

> I meant this increases the future risk of us becoming sensitive to the order
> of test case execution. Even if the current test cases guard against that,
> it's more defensive to make it impossible for future maintainers to make such
> a mistake. And would provide higher cohesion of tests too...
>
> 192 +struct TestRectangles : Test
> 193 +{
> 194 + Rectangles rectangles;
>
> So TEST_F becomes TEST again.

Daniel, I really don't understand your concern. Are you misreading

> 194 + Rectangles rectangles;

as this?
               static Rectangles rectangles;

Moving rectangles from function scope to instance scope doesn't share it between different instances.

=== modified file 'common-ABI-sha1sums'
--- common-ABI-sha1sums	2015-01-20 03:21:20 +0000
+++ common-ABI-sha1sums	2015-01-22 10:38:23 +0000
@@ -6,7 +6,7 @@
 bfd87d9fb800c1c1b528c00185570abac569caf2  include/common/mir/geometry/length.h
 d954464ef2d20c2876db68c94512a443186da09b  include/common/mir/geometry/point.h
 dc7c62d3916eec025e8e7deaf57e06077ce38928  include/common/mir/geometry/rectangle.h
-5161774957e3ca4f5fa4e7db025d0978d2bbef06  include/common/mir/geometry/rectangles.h
+41446a309ee25148a9b1a66ebee205c57e014a8f  include/common/mir/geometry/rectangles.h
 20fa71bf856b1f391ad84b0afd8c197e0ca9e982  include/common/mir/geometry/size.h
 e1be9faee8b844ca2ce617f8fd82c9ee08d56bed  include/common/mir/graphics/native_buffer.h
 dcf8b8982f138bdde39a241825c610e955cd5e33  include/common/mir/input/input_platform.h

=== modified file 'examples/server_example_window_management.cpp'
--- examples/server_example_window_management.cpp	2015-01-16 02:57:31 +0000
+++ examples/server_example_window_management.cpp	2015-01-22 10:38:23 +0000
@@ -170,15 +170,14 @@
     void add_display(Rectangle const& area) override
     {
         std::lock_guard<decltype(mutex)> lock(mutex);
-        displays.push_back(area);
+        displays.add(area);
         update_tiles();
     }
 
     void remove_display(Rectangle const& area) override
     {
         std::lock_guard<decltype(mutex)> lock(mutex);
-        auto const i = std::find(begin(displays), end(displays), area);
-        if (i != end(displays)) displays.erase(i);
+        displays.remove(area);
         update_tiles();
     }
 
@@ -308,12 +307,8 @@
         if (session_info.size() < 1 || displays.size() < 1) return;
 
         auto const sessions = session_info.size();
-        Rectangles view;
-
-        for (auto const& display : displays)
-            view.add(display);
-
-        auto const bounding_rect = view.bounding_rectangle();
+
+        auto const bounding_rect = displays.bounding_rectangle();
 
         auto const total_width  = bounding_rect.size.width.as_int();
         auto const total_height = bounding_rect.size.height.as_int();
@@ -558,7 +553,7 @@
     FocusControllerFactory const focus_controller;
 
     std::mutex mutex;
-    std::vector<Rectangle> displays;
+    Rectangles displays;
 
     std::map<ms::Session const*, SessionInfo> session_info;
     std::map<std::weak_ptr<ms::Surface>, SurfaceInfo, std::owner_less<std::weak_ptr<ms::Surface>>> surface_info;

=== modified file 'include/common/mir/geometry/rectangles.h'
--- include/common/mir/geometry/rectangles.h	2015-01-14 06:39:13 +0000
+++ include/common/mir/geometry/rectangles.h	2015-01-22 10:38:23 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2013 Canonical Ltd.
+ * Copyright © 2013-2015 Canonical Ltd.
  *
  * This program is free software: you can redistribute it and/or modify it
  * under the terms of the GNU Lesser General Public License version 3,
@@ -14,9 +14,6 @@
  * along with this program. If not, see <http://www.gnu.org/licenses/>.
  *
  * Authored by: Alexandros Frantzis <[e-mail]>
- *
- * XXX This header is only used externally in one location in QtMir. It could
- *     possibly be made private with only minor changes.
  */
 
 #ifndef MIR_GEOMETRY_RECTANGLES_H_
@@ -33,6 +30,7 @@
 namespace geometry
 {
 
+/// A collection of rectangles (with possible duplicates).
 class Rectangles
 {
 public:
@@ -41,6 +39,8 @@
     /* We want to keep implicit copy and move methods */
 
     void add(Rectangle const& rect);
+    /// removes at most one matching rectangle
+    void remove(Rectangle const& rect);
     void clear();
     Rectangle bounding_rectangle() const;
     void confine(Point& point) const;

=== modified file 'platform-ABI-sha1sums'
--- platform-ABI-sha1sums	2015-01-20 03:21:20 +0000
+++ platform-ABI-sha1sums	2015-01-22 10:38:23 +0000
@@ -6,7 +6,7 @@
 bfd87d9fb800c1c1b528c00185570abac569caf2  include/common/mir/geometry/length.h
 d954464ef2d20c2876db68c94512a443186da09b  include/common/mir/geometry/point.h
 dc7c62d3916eec025e8e7deaf57e06077ce38928  include/common/mir/geometry/rectangle.h
-5161774957e3ca4f5fa4e7db025d0978d2bbef06  include/common/mir/geometry/rectangles.h
+41446a309ee25148a9b1a66ebee205c57e014a8f  include/common/mir/geometry/rectangles.h
 20fa71bf856b1f391ad84b0afd8c197e0ca9e982  include/common/mir/geometry/size.h
 e1be9faee8b844ca2ce617f8fd82c9ee08d56bed  include/common/mir/graphics/native_buffer.h
 dcf8b8982f138bdde39a241825c610e955cd5e33  include/common/mir/input/input_platform.h

=== modified file 'server-ABI-sha1sums'
--- server-ABI-sha1sums	2015-01-22 03:10:13 +0000
+++ server-ABI-sha1sums	2015-01-22 10:38:23 +0000
@@ -6,7 +6,7 @@
 bfd87d9fb800c1c1b528c00185570abac569caf2  include/common/mir/geometry/length.h
 d954464ef2d20c2876db68c94512a443186da09b  include/common/mir/geometry/point.h
 dc7c62d3916eec025e8e7deaf57e06077ce38928  include/common/mir/geometry/rectangle.h
-5161774957e3ca4f5fa4e7db025d0978d2bbef06  include/common/mir/geometry/rectangles.h
+41446a309ee25148a9b1a66ebee205c57e014a8f  include/common/mir/geometry/rectangles.h
 20fa71bf856b1f391ad84b0afd8c197e0ca9e982  include/common/mir/geometry/size.h
 e1be9faee8b844ca2ce617f8fd82c9ee08d56bed  include/common/mir/graphics/native_buffer.h
 dcf8b8982f138bdde39a241825c610e955cd5e33  include/common/mir/input/input_platform.h

=== modified file 'src/common/geometry/rectangles.cpp'
--- src/common/geometry/rectangles.cpp	2015-01-14 06:39:13 +0000
+++ src/common/geometry/rectangles.cpp	2015-01-22 10:38:23 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2013 Canonical Ltd.
+ * Copyright © 2013-2015 Canonical Ltd.
  *
  * This program is free software: you can redistribute it and/or modify it
  * under the terms of the GNU Lesser General Public License version 3,
@@ -55,6 +55,12 @@
     rectangles.push_back(rect);
 }
 
+void geom::Rectangles::remove(Rectangle const& rect)
+{
+    auto const i = std::find(rectangles.begin(), rectangles.end(), rect);
+    if (i != rectangles.end()) rectangles.erase(i);
+}
+
 void geom::Rectangles::clear()
 {
     rectangles.clear();

=== modified file 'src/common/symbols.map'
--- src/common/symbols.map	2015-01-14 06:39:13 +0000
+++ src/common/symbols.map	2015-01-22 10:38:23 +0000
@@ -31,6 +31,7 @@
     mir::geometry::Rectangles::end*;
     mir::geometry::Rectangles::operator*;
     mir::geometry::Rectangles::Rectangles*;
+    mir::geometry::Rectangles::remove*;
     mir::geometry::Rectangles::size*;
     mir::geometry::Rectangle::top_right*;
     mir::geometry::Size::operator*;

=== modified file 'tests/unit-tests/geometry/test-rectangles.cpp'
--- tests/unit-tests/geometry/test-rectangles.cpp	2013-08-28 03:41:48 +0000
+++ tests/unit-tests/geometry/test-rectangles.cpp	2015-01-22 10:38:23 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2013 Canonical Ltd.
+ * Copyright © 2013-2015 Canonical Ltd.
  *
  * This program is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
@@ -19,34 +19,42 @@
 #include "mir/geometry/rectangles.h"
 
 #include <gtest/gtest.h>
+#include <gmock/gmock.h>
 
 #include <iterator>
 #include <algorithm>
 
-namespace geom = mir::geometry;
-
-TEST(geometry, rectangles_empty)
-{
-    using namespace geom;
-
-    Rectangles const rectangles;
-
+using namespace mir::geometry;
+using namespace testing;
+
+namespace
+{
+struct TestRectangles : Test
+{
+    Rectangles rectangles;
+
+    auto contents_of(Rectangles const& rects) ->
+        std::vector<Rectangle>
+    {
+        return {std::begin(rects), std::end(rects)};
+    }
+};
+}
+
+TEST_F(TestRectangles, rectangles_empty)
+{
     EXPECT_EQ(0, std::distance(rectangles.begin(), rectangles.end()));
     EXPECT_EQ(0u, rectangles.size());
 }
 
-TEST(geometry, rectangles_not_empty)
+TEST_F(TestRectangles, rectangles_not_empty)
 {
-    using namespace geom;
-
     std::vector<Rectangle> const rectangles_vec = {
         {Point{1, 2}, Size{Width{10}, Height{11}}},
         {Point{3, 4}, Size{Width{12}, Height{13}}},
         {Point{5, 6}, Size{Width{14}, Height{15}}}
     };
 
-    Rectangles rectangles;
-
     for (auto const& rect : rectangles_vec)
         rectangles.add(rect);
 
@@ -70,17 +78,14 @@
                             [](bool b){return b;}));
 }
 
-TEST(geometry, rectangles_clear)
+TEST_F(TestRectangles, rectangles_clear)
 {
-    using namespace geom;
-
     std::vector<Rectangle> const rectangles_vec = {
         {Point{1, 2}, Size{Width{10}, Height{11}}},
         {Point{3, 4}, Size{Width{12}, Height{13}}},
         {Point{5, 6}, Size{Width{14}, Height{15}}}
     };
 
-    Rectangles rectangles;
     Rectangles const rectangles_empty;
 
     for (auto const& rect : rectangles_vec)
@@ -93,10 +98,8 @@
     EXPECT_EQ(rectangles_empty, rectangles);
 }
 
-TEST(geometry, rectangles_bounding_rectangle)
+TEST_F(TestRectangles, rectangles_bounding_rectangle)
 {
-    using namespace geom;
-
     std::vector<Rectangle> const rectangles_vec = {
         {Point{0, 0}, Size{Width{10}, Height{5}}},
         {Point{2, 2}, Size{Width{4}, Height{1}}},
@@ -115,8 +118,6 @@
         {Point{-2, -3}, Size{Width{14}, Height{10}}}
     };
 
-    Rectangles rectangles;
-
     EXPECT_EQ(Rectangle(), rectangles.bounding_rectangle());
 
     for (size_t i = 0; i < rectangles_vec.size(); i++)
@@ -126,10 +127,8 @@
     }
 }
 
-TEST(geometry, rectangles_equality)
+TEST_F(TestRectangles, rectangles_equality)
 {
-    using namespace geom;
-
     std::vector<Rectangle> const rectangles_vec1 = {
         {Point{0, 0}, Size{Width{10}, Height{5}}},
         {Point{2, 2}, Size{Width{4}, Height{1}}},
@@ -169,10 +168,8 @@
     EXPECT_NE(rectangles1, rectangles_empty);
 }
 
-TEST(geometry, rectangles_copy_assign)
+TEST_F(TestRectangles, rectangles_copy_assign)
 {
-    using namespace geom;
-
     std::vector<Rectangle> const rectangles_vec = {
         {Point{0, 0}, Size{Width{10}, Height{5}}},
         {Point{2, 2}, Size{Width{4}, Height{1}}},
@@ -200,40 +197,91 @@
     EXPECT_EQ(rectangles2.bounding_rectangle(), rectangles3.bounding_rectangle());
 }
 
-TEST(geometry, rectangles_confine)
+TEST_F(TestRectangles, rectangles_confine)
 {
-    using namespace geom;
-
     std::vector<Rectangle> const rectangles_vec = {
-        {geom::Point{0,0}, geom::Size{800,600}},
-        {geom::Point{0,600}, geom::Size{100,100}},
-        {geom::Point{800,0}, geom::Size{100,100}}
-    };
-
-    std::vector<std::tuple<geom::Point,geom::Point>> const point_tuples{
-        std::make_tuple(geom::Point{0,0}, geom::Point{0,0}),
-        std::make_tuple(geom::Point{900,50}, geom::Point{899,50}),
-        std::make_tuple(geom::Point{850,100}, geom::Point{850,99}),
-        std::make_tuple(geom::Point{801,100}, geom::Point{801,99}),
-        std::make_tuple(geom::Point{800,101}, geom::Point{799,101}),
-        std::make_tuple(geom::Point{800,600}, geom::Point{799,599}),
-        std::make_tuple(geom::Point{-1,700}, geom::Point{0,699}),
-        std::make_tuple(geom::Point{-1,-1}, geom::Point{0,0}),
-        std::make_tuple(geom::Point{-1,50}, geom::Point{0,50}),
-        std::make_tuple(geom::Point{799,-1}, geom::Point{799,0}),
-        std::make_tuple(geom::Point{800,-1}, geom::Point{800,0})
-    };
-
-    Rectangles rectangles;
+        {Point{0,0}, Size{800,600}},
+        {Point{0,600}, Size{100,100}},
+        {Point{800,0}, Size{100,100}}
+    };
+
+    std::vector<std::tuple<Point,Point>> const point_tuples{
+        std::make_tuple(Point{0,0}, Point{0,0}),
+        std::make_tuple(Point{900,50}, Point{899,50}),
+        std::make_tuple(Point{850,100}, Point{850,99}),
+        std::make_tuple(Point{801,100}, Point{801,99}),
+        std::make_tuple(Point{800,101}, Point{799,101}),
+        std::make_tuple(Point{800,600}, Point{799,599}),
+        std::make_tuple(Point{-1,700}, Point{0,699}),
+        std::make_tuple(Point{-1,-1}, Point{0,0}),
+        std::make_tuple(Point{-1,50}, Point{0,50}),
+        std::make_tuple(Point{799,-1}, Point{799,0}),
+        std::make_tuple(Point{800,-1}, Point{800,0})
+    };
 
     for (auto const& rect : rectangles_vec)
         rectangles.add(rect);
 
     for (auto const& t : point_tuples)
     {
-        geom::Point confined_point{std::get<0>(t)};
-        geom::Point const expected_point{std::get<1>(t)};
+        Point confined_point{std::get<0>(t)};
+        Point const expected_point{std::get<1>(t)};
         rectangles.confine(confined_point);
         EXPECT_EQ(expected_point, confined_point);
     }
 }
+
+TEST_F(TestRectangles, tracks_add_and_remove)
+{
+    Rectangle const rect[]{
+        {{  0,   0}, {800, 600}},
+        {{  0, 600}, {100, 100}},
+        {{800,   0}, {100, 100}}};
+
+    rectangles = Rectangles{rect[0], rect[1], rect[2]};
+
+    EXPECT_THAT(contents_of(rectangles), UnorderedElementsAre(rect[0], rect[1], rect[2]));
+    EXPECT_THAT(rectangles.bounding_rectangle(), Eq(Rectangle{{0,0}, {900,700}}));
+
+    rectangles.remove(rect[1]);
+
+    EXPECT_THAT(contents_of(rectangles), UnorderedElementsAre(rect[0], rect[2]));
+    EXPECT_THAT(rectangles.bounding_rectangle(), Eq(Rectangle{{0,0}, {900,600}}));
+
+    rectangles.add(rect[2]);
+
+    EXPECT_THAT(contents_of(rectangles), UnorderedElementsAre(rect[0], rect[2], rect[2]));
+    EXPECT_THAT(rectangles.bounding_rectangle(), Eq(Rectangle{{0,0}, {900,600}}));
+
+    rectangles.add(rect[1]);
+    rectangles.remove(rect[2]);
+
+    EXPECT_THAT(contents_of(rectangles), UnorderedElementsAre(rect[0], rect[1], rect[2]));
+    EXPECT_THAT(rectangles.bounding_rectangle(), Eq(Rectangle{{0,0}, {900,700}}));
+}
+
+TEST_F(TestRectangles, can_add_same_rectangle_many_times)
+{
+    int const many_times = 10;
+    Rectangle const rectangle{{  0,   0}, {800, 600}};
+
+    for (auto i = 0; i != many_times; ++i)
+        rectangles.add(rectangle);
+
+    EXPECT_THAT(rectangles.size(), Eq(many_times));
+}
+
+TEST_F(TestRectangles, remove_only_removes_one_instance)
+{
+    int const many_times = 10;
+    Rectangle const rectangle{{  0,   0}, {800, 600}};
+
+    for (auto i = 0; i != many_times; ++i)
+        rectangles.add(rectangle);
+
+    for (auto i = many_times; i-- != 0;)
+    {
+        rectangles.remove(rectangle);
+        EXPECT_THAT(rectangles.size(), Eq(i));
+    }
+}