QtMir

Merge lp://staging/~nick-dedekind/qtmir/WindowNotifierObserver into lp://staging/qtmir

  1. WindowNotifierObserver
  2. Merge into trunk
Proposed by Nick Dedekind
Status: Work in progress
Proposed branch: lp://staging/~nick-dedekind/qtmir/WindowNotifierObserver
Merge into: lp://staging/qtmir
Diff against target: 501 lines (+202/-140)
9 files modified
src/modules/Unity/Application/mirsurface.cpp (+29/-0)
src/modules/Unity/Application/mirsurface.h (+2/-0)
src/modules/Unity/Application/surfacemanager.cpp (+37/-90)
src/modules/Unity/Application/surfacemanager.h (+4/-10)
src/modules/Unity/Application/windowmodel.cpp (+2/-34)
src/modules/Unity/Application/windowmodel.h (+0/-4)
src/platforms/mirserver/CMakeLists.txt (+1/-1)
src/platforms/mirserver/windowmodelnotifier.cpp (+108/-0)
src/platforms/mirserver/windowmodelnotifier.h (+19/-1)
To merge this branch: bzr merge lp://staging/~nick-dedekind/qtmir/WindowNotifierObserver
Reviewer Review Type Date Requested Status
Gerry Boland (community) Needs Information
Unity8 CI Bot (community) continuous-integration Approve
Review via email: mp+318884@code.staging.launchpad.net

Commit message

Added WindowNotifierObserver to replace SurfaceManager surface changes.

Description of the change

Added WindowNotifierObserver to reduce responsibility of the SurfaceManager.

Intent is to eventually move the rest of the SurfaceManager to Unity8.
Surfaces are now observers for the WindowModelNotifier.
Improved SurfaceManager surface lookup.

To post a comment you must log in.
lp://staging/~nick-dedekind/qtmir/WindowNotifierObserver updated
609. By Nick Dedekind

WindowNotifierObserver

610. By Nick Dedekind

removed unnecessary static fn

Revision history for this message
Unity8 CI Bot (unity8-ci-bot) wrote :

PASSED: Continuous integration, rev:609
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/549/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build/4302
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/4330
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4164
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4164/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4164
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4164/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4164
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4164/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4164
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4164/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4164
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4164/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4164
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4164/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/549/rebuild

review: Approve (continuous-integration)
Reply
Revision history for this message
Unity8 CI Bot (unity8-ci-bot) wrote :

PASSED: Continuous integration, rev:610
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/551/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build/4304
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/4332
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4166
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4166/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4166
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4166/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4166
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4166/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4166
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4166/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4166
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4166/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4166
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4166/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/551/rebuild

review: Approve (continuous-integration)
Reply
Revision history for this message
Gerry Boland (gerboland) wrote :

+ std::shared_ptr<mir::scene::Surface> msSurface = surface->window();
Can avoid the copy with:
const std::shared_ptr<mir::scene::Surface> &msSurface = surface->window();

+ for (size_t i = 0; i < windows.size(); i++) {
for (const auto& window : windows)
Why not use the C++11 niceties?!

Using QHash a good idea, much faster lookups.

Reply
Revision history for this message
Gerry Boland (gerboland) wrote :

+class WindowNotifierObserver : public QObject
+{
+ Q_OBJECT
+public:
+ WindowNotifierObserver(const miral::Window &window);
+ virtual ~WindowNotifierObserver();
+
+Q_SIGNALS:
+ void surfaceCreated();
+ void surfaceRemoved();
...

Since you use inheritance for the Impl, I find it wasteful to use signal/slots - you could just define virtual functions in the interface that the implementation can override. So

+class WindowNotifierObserver : public QObject
+{
+ Q_OBJECT
+public:
+ WindowNotifierObserver(const miral::Window &window);
+ virtual ~WindowNotifierObserver();
+
+ virtual void surfaceCreated();
+ virtual void surfaceRemoved();
...

Also instead of having a Hash table of miral::Window to observers, could you use the miral::WindowInfo::user_data, and attach the Observer to that? As you've said before, we don't really need multiple observers. Would mean one less data structure to maintain.

Would still need to take care with threading tho. WDYT?

review: Needs Information
Reply

Unmerged revisions

610. By Nick Dedekind

removed unnecessary static fn

609. By Nick Dedekind

WindowNotifierObserver

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches