Mir

Merge lp:~mir-team/mir/multiple-change-callbacks into lp:mir

multiple-change-callbacks
Merge into development-branch

Proposed by Robert Carr on 2014-04-03

Status:	Work in progress
Proposed branch:	lp:~mir-team/mir/multiple-change-callbacks
Merge into:	lp:mir
Diff against target:	411 lines (+172/-20) 11 files modified include/server/mir/compositor/scene.h (+8/-2) include/server/mir/scene/observer_id.h (+34/-0) include/test/mir_test_doubles/mock_scene.h (+4/-1) src/server/compositor/multi_threaded_compositor.cpp (+2/-2) src/server/compositor/multi_threaded_compositor.h (+3/-0) src/server/scene/surface_stack.cpp (+20/-4) src/server/scene/surface_stack.h (+7/-3) tests/integration-tests/test_surface_first_frame_sync.cpp (+4/-2) tests/unit-tests/compositor/test_default_display_buffer_compositor.cpp (+10/-2) tests/unit-tests/compositor/test_multi_threaded_compositor.cpp (+17/-4) tests/unit-tests/scene/test_surface_stack.cpp (+63/-0)
To merge this branch:	bzr merge lp:~mir-team/mir/multiple-change-callbacks
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Alan Griffiths			Abstain on 2014-04-15
Daniel van Vugt			Abstain on 2014-04-15
Alberto Aguirre (community)		2014-04-03	Approve on 2014-04-15
PS Jenkins bot (community)	continuous-integration		Approve on 2014-04-15
Review via email: mp+214131@code.launchpad.net

Commit message

Rework Scene::set_change_callback to add/remove change callback in order to support multiple subscribers.

Description of the change

As part of the cursor spike support registering multiple change callbacks to the surface stack/scene. In the cursor spike these methods are added to mi::InputTargets so that the cursor controller can update the image when surfaces dissapear, etc...

Also told AlbertA needs this for some screencast stuff?

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2014-04-03:

"Also told AlbertA needs this for some screencast stuff?"

Right, I want to optionally be able to get the screencast compositor to listen to scene changes so it can capture at scene change rate.

Lines 8-15, 114, 117, 119, 131, seems like a wrong merge resolution.

Also what about testing for multiple observer notifications? Maybe this:

TEST_F(SurfaceStack, notifies_multiple_observers)
{
    using namespace ::testing;
    MockCallback mock_cb1;
    MockCallback mock_cb2;

EXPECT_CALL(mock_cb1, call()).Times(1);
EXPECT_CALL(mock_cb2, call()).Times(1);

ms::SurfaceStack stack(mt::fake_shared(input_registrar), report);

    auto id1 = stack.add_change_callback(std::bind(&MockCallback::call, &mock_cb1));
    auto id2 = stack.add_change_callback(std::bind(&MockCallback::call, &mock_cb2));
    stack.add_surface(stub_surface1, default_params.depth, default_params.input_mode);
    stack.remove_change_callback(id1);
    stack.remove_change_callback(id2);

    EXPECT_CALL(mock_cb1, call()).Times(0);
    EXPECT_CALL(mock_cb2, call()).Times(0);
    stack.remove_surface(stub_surface1);
}

review: Needs Fixing

lp:~mir-team/mir/multiple-change-callbacks updated on 2014-04-04

1530. By Alan Griffiths on 2014-04-04

frontend, shell: frontend doesn't need send_display_config() in the Session interface.

Approved by Alberto Aguirre, Kevin DuBois, PS Jenkins bot.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-04:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1245/rebuild

review: Approve (continuous-integration)

lp:~mir-team/mir/multiple-change-callbacks updated on 2014-04-04

1531. By Alan Griffiths on 2014-04-04

scene: Remove spurious BasicSurface::frame_posted() function and rework tests that used it.

Approved by PS Jenkins bot, Daniel van Vugt, Alexandros Frantzis, Kevin DuBois.

1532. By Kevin DuBois on 2014-04-04

compositor: transition bypass detection to operating on RenderableList instead of using operator/filter on mc::Scene.

Approved by Alberto Aguirre, Alan Griffiths, PS Jenkins bot.

1533. By Daniel van Vugt on 2014-04-04

Add a simple typedef Renderable::ID to make distinguishing renderables
slightly more explicit.
.

Approved by PS Jenkins bot, Alberto Aguirre, Kevin DuBois.

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-04-04:

8 +set (OLD_CMAKE_CXX_FLAGS ${CMAKE_CXX_FLAGS})
9 +# Don't treat warnings as errors in 3rd_party/{gmock}
10 +string (REPLACE " -Werror " " " CMAKE_CXX_FLAGS ${CMAKE_CXX_FLAGS})
11 +
12 +
13 +# Restore -Werror for non-3rd-party code
14 +set (CMAKE_CXX_FLAGS ${OLD_CMAKE_CXX_FLAGS})
15 +

Spurious

~~~~

114 - miroptions
115 mirplatform
116 mirsharedandroid
117 - mirsharedenv
118
119 - ${Boost_PROGRAM_OPTIONS_LIBRARY}
120 ${LIBHARDWARE_LIBRARIES}
121 ${EGL_LDFLAGS} ${EGL_LIBRARIES}
122 ${GLESv2_LDFLAGS} ${GLESv2_LIBRARIES}
123
124 === modified file 'src/platform/graphics/mesa/CMakeLists.txt'
125 --- src/platform/graphics/mesa/CMakeLists.txt 2014-04-03 02:01:49 +0000
126 +++ src/platform/graphics/mesa/CMakeLists.txt 2014-04-03 22:20:09 +0000
127 @@ -47,7 +47,6 @@
128 mirplatformgraphicsmesa
129 mirplatform
130
131 - ${Boost_PROGRAM_OPTIONS_LIBRARY}

Puzzling

~~~~

87 +typedef IntWrapper<detail::ObserverIdTag> ObserverId;

I'm not convinced we need an Observer ID but other ids capitalize the "D"

review: Needs Fixing

lp:~mir-team/mir/multiple-change-callbacks updated on 2014-04-05

1534. By Alberto Aguirre on 2014-04-04

Fill the mode refresh rate field for android platform display configurations.

Approved by Alan Griffiths, Alexandros Frantzis, PS Jenkins bot, Kevin DuBois.

1535. By Alan Griffiths on 2014-04-04

shell: Eliminate shell::SurfaceFactory and shell::Surface and use scene::SurfaceCoordinator and scene::Surface instead.

Approved by Andreas Pokorny, Gerry Boland, Alexandros Frantzis, PS Jenkins bot.

1536. By Kevin DuBois on 2014-04-04

Remove the RenderingOperator and VisibilityFilter classes in favor of rendering on the generated list of renderables from the scene.

Approved by Alan Griffiths, Alexandros Frantzis, PS Jenkins bot, Alberto Aguirre.

1537. By Alberto Aguirre on 2014-04-05

make current before deleting gl resources in GLPixelBuffer (LP: #1256360). Fixes: https://bugs.launchpad.net/bugs/1256360.

Approved by Alexandros Frantzis, PS Jenkins bot, Kevin DuBois.

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-04-07:

(1) As Alan said, the CMakeLists.txt changes are spurious reverts of changes we want to keep.

(2) Needs Information: We potentially don't need to use ObserverId. The std::function is already unique and has operator== so could be used as the identifier itself...
48 + virtual scene::ObserverId add_change_callback(std::function<void()> const& f) = 0;
49 + virtual void remove_change_callback(scene::ObserverId id) = 0;
Then notify_change_by_id could become a std::set with a custom Compare template parameter. Or more simply a std::list with std::find.

(3) observer_id.h: Wrong copyright but then (2) suggests the whole file isn't required.
59 + * Copyright © 2013 Canonical Ltd.

(4) "notify_change_by_id" is a confusing name for a map. At least the "notify_change" part. Maybe "callbacks_by_id" or "observers_by_id".

review: Needs Fixing

lp:~mir-team/mir/multiple-change-callbacks updated on 2014-04-09

1538. By Robert Carr on 2014-04-09: Rework Scene::set_change_callback to add/remove change callback to support multiple subscribers
1539. By Robert Carr on 2014-04-09: ObserverId/ObserverID
1540. By Robert Carr on 2014-04-09: observer_id.h: Fix copyright
1541. By Robert Carr on 2014-04-09: notify_change_by_id->observers_by_id
1542. By Robert Carr on 2014-04-09: test_surface_stack.cpp: Add test, notifies_multiple_observers

Revision history for this message

Robert Carr (robertcarr) wrote on 2014-04-09:

Sorry about CMakeList changes...pushed wrong branch. Fixed copyright on observer_id.h. Renamed notify_change_by_id -> observers by id. ObserverId->ObserverID. Added multiple observer test.

I think utilizing std::function as the identifier is a bad idea because its convenient to just be able to pass an anonymous lambda.

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2014-04-09:

> (2) Needs Information: We potentially don't need to use ObserverId. The
> std::function is already unique and has operator== so could be used as the
> identifier itself...
> 48 + virtual scene::ObserverId
> add_change_callback(std::function<void()> const& f) = 0;
> 49 + virtual void remove_change_callback(scene::ObserverId id) = 0;
> Then notify_change_by_id could become a std::set with a custom Compare
> template parameter. Or more simply a std::list with std::find.
>

This imposes the caller to save the std::function it passed even if they are lambdas, which is probably more awkward than obtaining an id.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-09:

FAILED: Continuous integration, rev:1542
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1288/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1547/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1545/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/1120/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1020/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1025/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1121/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1288/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2014-04-09:

Unresolved conflicts:

Text conflict in include/test/mir_test_doubles/mock_scene.h
Text conflict in src/server/scene/surface_stack.cpp
Text conflict in src/server/scene/surface_stack.h
Text conflict in tests/unit-tests/compositor/test_default_display_buffer_compositor.cpp
Text conflict in tests/unit-tests/compositor/test_multi_threaded_compositor.cpp
Text conflict in tests/unit-tests/scene/test_surface_stack.cpp

review: Needs Fixing

Revision history for this message

Robert Carr (robertcarr) wrote on 2014-04-14:

Whoops...I got distracted in the middle of merging devel in to this during the sprint and thought I had finished it.

lp:~mir-team/mir/multiple-change-callbacks updated on 2014-04-14

1543. By Robert Carr on 2014-04-14: Merge trunk

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-15:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1319/rebuild

review: Approve (continuous-integration)

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2014-04-15:

LGTM,

Just fyi, this will conflict with the refactoring in:
https://code.launchpad.net/~albaguirre/mir/fix-1296544/+merge/214456

review: Approve

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-04-15:

I see activity but no time to re-review right now.

review: Abstain

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-04-15:

I'm still not convinced by ObserverID - other elements of the scene are managing observers by smart pointers to the observer object (Surface is close to its final state, Session still needs some work).

In all cases the legacy approach of "something changed but I won't tell you what" notification is getting reworked. So these changes will be swept away by that anyway.

review: Abstain

Unmerged revisions

1543. By Robert Carr on 2014-04-14: Merge trunk
1542. By Robert Carr on 2014-04-09: test_surface_stack.cpp: Add test, notifies_multiple_observers
1541. By Robert Carr on 2014-04-09: notify_change_by_id->observers_by_id
1539. By Robert Carr on 2014-04-09: ObserverId/ObserverID
1538. By Robert Carr on 2014-04-09: Rework Scene::set_change_callback to add/remove change callback to support multiple subscribers

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Emanuele Antonio Faraone

Gerry Boland

Mir development team

 === modified file 'include/server/mir/compositor/scene.h'
 --- include/server/mir/compositor/scene.h	2014-04-04 17:48:23 +0000
 +++ include/server/mir/compositor/scene.h	2014-04-14 21:47:44 +0000
@@ -21,6 +21,7 @@
  #include "mir/geometry/forward.h"
  #include "mir/graphics/renderable.h"
++#include "mir/scene/observer_id.h"
  #include <memory>
  #include <functional>
@@ -42,14 +43,19 @@
      virtual graphics::RenderableList generate_renderable_list() const = 0;
      /**
--     * Sets a callback to be called whenever the state of the
++     * Registers a callback to be called whenever the state of the
       * Scene changes.
+      *
++     * The returned ObserverID may be passed to remove_change_callback
++     * to unregister for change notification.
++     *
       * The supplied callback should not directly or indirectly (e.g.,
       * by changing a property of a surface) change the state of
       * the Scene, otherwise a deadlock may occur.
++     *
       */
--    virtual void set_change_callback(std::function<void()> const& f) = 0;
++    virtual scene::ObserverID add_change_callback(std::function<void()> const& f) = 0;
++    virtual void remove_change_callback(scene::ObserverID id) = 0;
  protected:
      Scene() = default;
 === added file 'include/server/mir/scene/observer_id.h'
 --- include/server/mir/scene/observer_id.h	1970-01-01 00:00:00 +0000
 +++ include/server/mir/scene/observer_id.h	2014-04-14 21:47:44 +0000
@@ -0,0 +1,34 @@
++/*
++ * Copyright © 2014 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 published by the Free Software Foundation.
++ *
++ * This program is distributed in the hope that it will be useful,
++ * but WITHOUT ANY WARRANTY; without even the implied warranty of
++ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
++ * GNU General Public License for more details.
++ *
++ * You should have received a copy of the GNU General Public License
++ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
++ *
++ * Authored By: Robert Carr <racarr@canonical.com>
++ */
++
++#ifndef MIR_SCENE_OBSERVER_NOTIFICATION_ID_H_
++#define MIR_SCENE_OBSERVER_NOTIFICATION_ID_H_
++
++#include "mir/int_wrapper.h"
++
++namespace mir
++{
++namespace scene
++{
++namespace detail { struct ObserverIDTag; }
++
++typedef IntWrapper<detail::ObserverIDTag> ObserverID;
++}
++} // namespace mir
++
++#endif // MIR_SCENE_OBSERVER_NOTIFICATION_ID_H_
 === modified file 'include/test/mir_test_doubles/mock_scene.h'
 --- include/test/mir_test_doubles/mock_scene.h	2014-04-03 00:14:45 +0000
 +++ include/test/mir_test_doubles/mock_scene.h	2014-04-14 21:47:44 +0000
@@ -38,7 +38,10 @@
              .WillByDefault(testing::Return(graphics::RenderableList{}));
+     }
      MOCK_CONST_METHOD0(generate_renderable_list, graphics::RenderableList());
--    MOCK_METHOD1(set_change_callback, void(std::function<void()> const&));
++
++    MOCK_METHOD1(add_change_callback, scene::ObserverID(std::function<void()> const&));
++    MOCK_METHOD1(remove_change_callback, void(scene::ObserverID));
++
      MOCK_METHOD0(lock, void());
      MOCK_METHOD0(unlock, void());
  };
 === modified file 'src/server/compositor/multi_threaded_compositor.cpp'
 --- src/server/compositor/multi_threaded_compositor.cpp	2014-04-09 17:39:31 +0000
 +++ src/server/compositor/multi_threaded_compositor.cpp	2014-04-14 21:47:44 +0000
@@ -271,7 +271,7 @@
+     }
      /* Recomposite whenever the scene changes */
--    scene->set_change_callback([this]()
++    change_registration = scene->add_change_callback([this]()
+     {
          schedule_compositing();
      });
@@ -295,7 +295,7 @@
+     }
      lk.unlock();
--    scene->set_change_callback([]{});
++    scene->remove_change_callback(change_registration);
      lk.lock();
      for (auto& f : thread_functors)
 === modified file 'src/server/compositor/multi_threaded_compositor.h'
 --- src/server/compositor/multi_threaded_compositor.h	2014-03-07 03:15:55 +0000
 +++ src/server/compositor/multi_threaded_compositor.h	2014-04-14 21:47:44 +0000
@@ -20,6 +20,7 @@
  #define MIR_COMPOSITOR_MULTI_THREADED_COMPOSITOR_H_
  #include "mir/compositor/compositor.h"
++#include "mir/scene/observer_id.h"
  #include <mutex>
  #include <memory>
@@ -67,6 +68,8 @@
      bool compose_on_start;
      void schedule_compositing();
++
++    scene::ObserverID change_registration;
  };
+ }
 === modified file 'src/server/scene/surface_stack.cpp'
 --- src/server/scene/surface_stack.cpp	2014-04-14 11:31:04 +0000
 +++ src/server/scene/surface_stack.cpp	2014-04-14 21:47:44 +0000
@@ -54,7 +54,7 @@
      input_registrar{input_registrar},
      report{report},
      change_cb{[this]() { emit_change_notification(); }},
--    notify_change{[]{}}
++    next_change_callback_id(0)
+ {
+ }
@@ -125,11 +125,26 @@
      return list;
+ }
--void ms::SurfaceStack::set_change_callback(std::function<void()> const& f)
++ms::ObserverID ms::SurfaceStack::add_change_callback(std::function<void()> const& f)
+ {
      std::lock_guard<decltype(notify_change_mutex)> lg(notify_change_mutex);
      assert(f);
--    notify_change = f;
++
++    auto id = ms::ObserverID{++next_change_callback_id};
++    observers_by_id[id] = f;
++
++    return id;
++}
++
++void ms::SurfaceStack::remove_change_callback(ms::ObserverID id)
++{
++    std::lock_guard<std::mutex> lg{notify_change_mutex};
++
++    auto it = observers_by_id.find(id);
++    if (it == observers_by_id.end())
++        BOOST_THROW_EXCEPTION(std::logic_error(
++            "Invalid change notification id (not previously registered, or already unregistered)"));
++    observers_by_id.erase(it);
+ }
  void ms::SurfaceStack::add_surface(
@@ -183,7 +198,8 @@
  void ms::SurfaceStack::emit_change_notification()
+ {
      std::lock_guard<decltype(notify_change_mutex)> lg(notify_change_mutex);
--    notify_change();
++    for (auto kv : observers_by_id)
++        kv.second();
+ }
  void ms::SurfaceStack::for_each(std::function<void(std::shared_ptr<mi::InputChannel> const&)> const& callback)
 === modified file 'src/server/scene/surface_stack.h'
 --- src/server/scene/surface_stack.h	2014-04-04 22:32:51 +0000
 +++ src/server/scene/surface_stack.h	2014-04-14 21:47:44 +0000
@@ -23,6 +23,7 @@
  #include "mir/compositor/scene.h"
  #include "mir/scene/depth_id.h"
++#include "mir/scene/observer_id.h"
  #include "mir/input/input_targets.h"
  #include <memory>
@@ -67,8 +68,10 @@
      // From Scene
      graphics::RenderableList generate_renderable_list() const;
--    virtual void set_change_callback(std::function<void()> const& f);
--
++
++    virtual scene::ObserverID add_change_callback(std::function<void()> const& f);
++    virtual void remove_change_callback(scene::ObserverID id);
++
      // From InputTargets
      void for_each(std::function<void(std::shared_ptr<input::InputChannel> const&)> const& callback);
@@ -96,7 +99,8 @@
      std::map<DepthId, Layer> layers_by_depth;
      std::mutex notify_change_mutex;
--    std::function<void()> notify_change;
++    int next_change_callback_id;
++    std::map<ObserverID, std::function<void()>> observers_by_id;
  };
+ }
 === modified file 'tests/integration-tests/test_surface_first_frame_sync.cpp'
 --- tests/integration-tests/test_surface_first_frame_sync.cpp	2014-04-04 21:48:32 +0000
 +++ tests/integration-tests/test_surface_first_frame_sync.cpp	2014-04-14 21:47:44 +0000
@@ -69,7 +69,7 @@
      void start()
+     {
--        scene->set_change_callback([this]()
++        change_id = scene->add_change_callback([this]()
+         {
              display->for_each_display_buffer([this](mg::DisplayBuffer& display_buffer)
+             {
@@ -80,13 +80,15 @@
      void stop()
+     {
--        scene->set_change_callback([]{});
++        scene->remove_change_callback(change_id);
+     }
  private:
      std::shared_ptr<mg::Display> const display;
      std::shared_ptr<mc::Scene> const scene;
      std::unordered_map<mg::DisplayBuffer*,std::unique_ptr<mc::DisplayBufferCompositor>> display_buffer_compositor_map;
++
++    ms::ObserverID change_id;
  };
  class StubRenderer : public mtd::StubRenderer
 === modified file 'tests/unit-tests/compositor/test_default_display_buffer_compositor.cpp'
 --- tests/unit-tests/compositor/test_default_display_buffer_compositor.cpp	2014-04-07 16:46:16 +0000
 +++ tests/unit-tests/compositor/test_default_display_buffer_compositor.cpp	2014-04-14 21:47:44 +0000
@@ -38,10 +38,11 @@
  namespace mg = mir::graphics;
  namespace mc = mir::compositor;
++namespace ms = mir::scene;
++namespace mr = mir::report;
  namespace geom = mir::geometry;
  namespace mt = mir::test;
  namespace mtd = mir::test::doubles;
--namespace mr = mir::report;
  namespace
+ {
@@ -58,7 +59,14 @@
          return renderlist;
+     }
--    void set_change_callback(std::function<void()> const&) {}
++    ms::ObserverID add_change_callback(std::function<void()> const&) override
++    {
++        return ms::ObserverID{};
++    }
++
++    void remove_change_callback(ms::ObserverID) override
++    {
++    }
      void change(mg::RenderableList const& new_renderlist)
+     {
 === modified file 'tests/unit-tests/compositor/test_multi_threaded_compositor.cpp'
 --- tests/unit-tests/compositor/test_multi_threaded_compositor.cpp	2014-04-09 11:40:25 +0000
 +++ tests/unit-tests/compositor/test_multi_threaded_compositor.cpp	2014-04-14 21:47:44 +0000
@@ -42,9 +42,10 @@
  namespace mc = mir::compositor;
  namespace mg = mir::graphics;
++namespace ms = mir::scene;
++namespace mr = mir::report;
  namespace geom = mir::geometry;
  namespace mtd = mir::test::doubles;
--namespace mr = mir::report;
  namespace
+ {
@@ -99,11 +100,20 @@
          return renderable_list;
+     }
--    void set_change_callback(std::function<void()> const& f)
++
++    ms::ObserverID add_change_callback(std::function<void()> const& f) override
+     {
          std::lock_guard<std::mutex> lock{callback_mutex};
          assert(f);
          callback = f;
++
++        // Test only needs a single change callback.
++        return ms::ObserverID{0};
++    }
++    void remove_change_callback(ms::ObserverID id) override
++    {
++        assert(id == ms::ObserverID{0});
++        callback = [](){};
+     }
      void emit_change_event()
@@ -563,12 +573,15 @@
      auto mock_scene = std::make_shared<mtd::MockScene>();
      auto db_compositor_factory = std::make_shared<mtd::NullDisplayBufferCompositorFactory>();
      auto mock_report = std::make_shared<testing::NiceMock<mtd::MockCompositorReport>>();
++
      EXPECT_CALL(*mock_report, started())
          .Times(1);
      EXPECT_CALL(*mock_report, stopped())
          .Times(1);
--    EXPECT_CALL(*mock_scene, set_change_callback(_))
--        .Times(2);
++    EXPECT_CALL(*mock_scene, add_change_callback(_))
++        .Times(1).WillOnce(Return(ms::ObserverID{}));
++    EXPECT_CALL(*mock_scene, remove_change_callback(_))
++        .Times(1);
      EXPECT_CALL(*mock_scene, generate_renderable_list())
          .Times(AtLeast(0))
          .WillRepeatedly(Return(mg::RenderableList{}));
 === modified file 'tests/unit-tests/scene/test_surface_stack.cpp'
 --- tests/unit-tests/scene/test_surface_stack.cpp	2014-04-14 11:31:04 +0000
 +++ tests/unit-tests/scene/test_surface_stack.cpp	2014-04-14 21:47:44 +0000
@@ -168,6 +168,69 @@
      EXPECT_THAT((*it)->id(), Eq(stub_surface3->id()));
+ }
++TEST_F(SurfaceStack, notify_on_create_and_destroy_surface)
++{
++    using namespace ::testing;
++    NiceMock<MockCallback> mock_cb;
++    EXPECT_CALL(mock_cb, call())
++        .Times(2);
++
++    ms::SurfaceStack stack(mt::fake_shared(input_registrar), report);
++
++    stack.add_change_callback(std::bind(&MockCallback::call, &mock_cb));
++    stack.add_surface(stub_surface1, default_params.depth, default_params.input_mode);
++    stack.remove_surface(stub_surface1);
++}
++
++TEST_F(SurfaceStack, remove_change_callback)
++{
++    using namespace ::testing;
++    MockCallback mock_cb;
++    EXPECT_CALL(mock_cb, call())
++        .Times(1);
++
++    ms::SurfaceStack stack(mt::fake_shared(input_registrar), report);
++
++    auto id = stack.add_change_callback(std::bind(&MockCallback::call, &mock_cb));
++    stack.add_surface(stub_surface1, default_params.depth, default_params.input_mode);
++    stack.remove_change_callback(id);
++
++    EXPECT_CALL(mock_cb, call()).Times(0);
++    stack.remove_surface(stub_surface1);
++}
++
++TEST_F(SurfaceStack, notifies_multiple_observers)
++{
++    using namespace ::testing;
++    MockCallback mock_cb1;
++    MockCallback mock_cb2;
++
++    EXPECT_CALL(mock_cb1, call()).Times(1);
++    EXPECT_CALL(mock_cb2, call()).Times(1);
++
++    ms::SurfaceStack stack(mt::fake_shared(input_registrar), report);
++
++    auto id1 = stack.add_change_callback(std::bind(&MockCallback::call, &mock_cb1));
++    auto id2 = stack.add_change_callback(std::bind(&MockCallback::call, &mock_cb2));
++    stack.add_surface(stub_surface1, default_params.depth, default_params.input_mode);
++    stack.remove_change_callback(id1);
++    stack.remove_change_callback(id2);
++
++    EXPECT_CALL(mock_cb1, call()).Times(0);
++    EXPECT_CALL(mock_cb2, call()).Times(0);
++    stack.remove_surface(stub_surface1);
++}
++
++TEST_F(SurfaceStack, remove_invalid_change_callback)
++{
++    using namespace ::testing;
++
++    ms::SurfaceStack stack(mt::fake_shared(input_registrar), report);
++    EXPECT_THROW({
++            stack.remove_change_callback(ms::ObserverID{1});
++    }, std::logic_error);
++}
++
  TEST_F(SurfaceStack, surfaces_are_emitted_by_layer)
+ {
      using namespace testing;