QtMir

Merge lp://qastaging/~unity-team/qtmir/qtmir.api into lp://qastaging/qtmir

qtmir.api
Merge into trunk

Proposed by Nick Dedekind on 2017-01-26

Status:	Needs review
Proposed branch:	lp://qastaging/~unity-team/qtmir/qtmir.api
Merge into:	lp://qastaging/qtmir
Prerequisite:	lp://qastaging/~nick-dedekind/qtmir/miral-DisplayConfigurationStorage
Diff against target:	3478 lines (+1743/-612) 53 files modified CMakeLists.txt (+3/-1) debian/changelog (+7/-0) debian/control (+29/-0) debian/gles-patches/convert-to-gles.patch (+27/-17) debian/libqtmirserver-dev.install (+3/-0) debian/libqtmirserver1.install (+1/-0) debian/qtmir-tests.install (+1/-1) debian/rules (+2/-0) demos/CMakeLists.txt (+1/-1) demos/qml-demo-client/main.cpp (+2/-2) demos/qml-demo-shell/CMakeLists.txt (+9/-0) demos/qml-demo-shell/main.cpp (+83/-10) include/qtmir/displayconfigurationpolicy.h (+70/-10) include/qtmir/displayconfigurationstorage.h (+61/-0) include/qtmir/miral/display_configuration_policy.h (+42/-0) include/qtmir/miral/display_configuration_storage.h (+6/-1) include/qtmir/mirserverapplication.h (+49/-0) include/qtmir/sessionauthorizer.h (+63/-20) include/qtmir/windowmanagementpolicy.h (+129/-0) src/modules/Unity/Application/CMakeLists.txt (+3/-1) src/modules/Unity/Application/taskcontroller.cpp (+1/-1) src/modules/Unity/Screens/CMakeLists.txt (+1/-1) src/platforms/mirserver/CMakeLists.txt (+103/-44) src/platforms/mirserver/displayconfigurationpolicy.cpp (+53/-39) src/platforms/mirserver/displayconfigurationstorage.cpp (+46/-0) src/platforms/mirserver/miral/CMakeLists.txt (+7/-2) src/platforms/mirserver/miral/display_configuration_policy.cpp (+21/-0) src/platforms/mirserver/miral/edid.cpp (+1/-1) src/platforms/mirserver/miral/persist_display_config.cpp (+57/-22) src/platforms/mirserver/miral/persist_display_config.h (+3/-4) src/platforms/mirserver/mirserverapplication.cpp (+53/-0) src/platforms/mirserver/mirserverintegration.cpp (+1/-1) src/platforms/mirserver/mirserverintegration.h (+2/-1) src/platforms/mirserver/qmirserver.cpp (+51/-0) src/platforms/mirserver/qmirserver.h (+24/-3) src/platforms/mirserver/qmirserver_p.cpp (+57/-16) src/platforms/mirserver/qmirserver_p.h (+12/-6) src/platforms/mirserver/qtmirserver.pc.in (+9/-0) src/platforms/mirserver/screensmodel.cpp (+1/-2) src/platforms/mirserver/windowcontroller.cpp (+2/-2) src/platforms/mirserver/windowcontroller.h (+4/-4) src/platforms/mirserver/wrappedsessionauthorizer.cpp (+134/-72) src/platforms/mirserver/wrappedsessionauthorizer.h (+39/-0) src/platforms/mirserver/wrappedwindowmanagementpolicy.cpp (+446/-297) src/platforms/mirserver/wrappedwindowmanagementpolicy.h (+13/-22) tests/mirserver/EventBuilder/CMakeLists.txt (+1/-1) tests/mirserver/QtEventFeeder/CMakeLists.txt (+1/-1) tests/mirserver/Screen/CMakeLists.txt (+3/-1) tests/mirserver/ScreensModel/CMakeLists.txt (+1/-1) tests/mirserver/miral/CMakeLists.txt (+2/-2) tests/mirserver/miral/edid_test.cpp (+1/-1) tests/modules/Application/CMakeLists.txt (+1/-1) tests/modules/WindowManager/CMakeLists.txt (+1/-0)
To merge this branch:	bzr merge lp://qastaging/~unity-team/qtmir/qtmir.api
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Unity8 CI Bot (community)	continuous-integration		Approve on 2017-04-05
Gerry Boland (community)			Needs Fixing on 2017-03-22
Daniel d'Andrada (community)			Abstain on 2017-02-22
Michael Terry (community)	packaging	2017-01-26	Needs Fixing on 2017-02-15
Review via email: mp+315681@code.qastaging.launchpad.net

Commit message

API for qtmir

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-02-14:

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

review: Needs Fixing (continuous-integration)

lp://qastaging/~unity-team/qtmir/qtmir.api updated on 2017-02-14

625. By Nick Dedekind on 2017-02-14: merged parent

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-02-14:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Gerry Boland (gerboland) wrote on 2017-02-14:

=== modified file 'debian/control'
+Package: qtmir-dev
I suspect qtmir will evolve into a more typical library in time. Can you rename to libqtmir-dev

+Replaces: qtmir-android-dev,
where did you find this package? qtmir never had a -dev package before.

Revision history for this message

Gerry Boland (gerboland) wrote on 2017-02-14:

Copyrights need bumping to 2017

Revision history for this message

Gerry Boland (gerboland) wrote on 2017-02-14:

== added file 'include/qtmir/guiserverapplication.h'
+class GuiServerApplication : public QGuiApplication

"Gui" is redundant here, I'd rather "MirServerApplication"

Good work on establishing an API that is as flexible as MirAL's.

Revision history for this message

Gerry Boland (gerboland) wrote on 2017-02-14:

+++ src/platforms/mirserver/displayconfigurationstorage.cpp

last line
+}
missing a "// namespace qtmir"

+++ src/platforms/mirserver/guiserverapplication.cpp
+QSharedPointer<QMirServer> mirServer;
does it need to be shared?

around line 36, please add "// namespace" after the anonymous closing brace. And at EOF, another comment please. I get lost with namespaces.

+ : QGuiApplication((init(argc, argv, options), argc), argv) // comma operator to ensure init called before QGuiApplication
clever!

+++ src/platforms/mirserver/miral/persist_display_config.cpp
+ if (mode.size == newMode.size && mode.vrefresh_hz == newMode.refresh_rate) {
refresh_rate comparison is a floating point comparison, could be unreliable. Can you use something like qFuzzyCompare?

+++ src/platforms/mirserver/mirserverintegration.h
- QScopedPointer<QMirServer> m_mirServer;
+ QSharedPointer<QMirServer> m_mirServer;
Does it really need to be shared? AFAICS nobody else makes a copy of the QSharedPointer after construction.

+++ src/platforms/mirserver/qmirserver.h
+ static QSharedPointer<QMirServer> create(int &argc,
+ char **argv);
It's your big entry point header file, make it pretty! Do line up these arguments, or just have on single line.

+++ src/platforms/mirserver/qmirserver_p.cpp
+auto buildDisplayConfigurationPolicy()
+-> std::shared_ptr<miral::DisplayConfigurationPolicy>
I'm not a fan of this method definition style. Please stick to the old fashioned one.

Revision history for this message

Gerry Boland (gerboland) wrote on 2017-02-14:

+++ src/platforms/mirserver/qtmirserver.pc.in
We need to standardize names. "qtmir" was never a great name, it is unclear if mir server or client. "qtmirserver" is ideal, but I know I am annoyed when the debian package name doesn't match the name used for the pkgconfig.

Should we call the debian package libqtmirserver then?

+Requires: miral
We should require miral version 1.1

Revision history for this message

Gerry Boland (gerboland) wrote on 2017-02-14:

+ WrappedWindowManagementPolicy(const miral::WindowManagerTools &tools,
+ qtmir::WindowModelNotifier &windowModel,
+ qtmir::WindowController &windowController,
+ qtmir::AppNotifier &appNotifier,
+ const std::shared_ptr<QtEventFeeder>& eventFeeder,
+ const qtmir::WindowManagmentPolicyBuilder& wmBuilder);

Last two lines need & on the right, to be consistent

Revision history for this message

Michael Terry (mterry) wrote on 2017-02-15:

Looking at the packaging...

+Package: qtmir-dev

This should probably be called libqtmirserver-dev instead.

+Replaces: qtmir-android-dev,

What's qtmir-android-dev from?

+Depends: qtmir-desktop (= ${source:Version}) | qtmir-android (= ${source:Version}),

I believe you want binary:Version here.

+Description: Developer files for the Qt platform abstraction plugin for a
+ Mir server.
+ .
+ Contains header files required for development using QtMir.

The first line of the description is special, don't break it across lines. I'd do something like:

Description: Header files for QtMir
QtMir is a set of Qt5 components to enable one to write a Mir server with Qt.
It contains a QPA (Qt Platform Abstraction) plugin which creates and manages
a Mir server. It also exposes some internal Mir functionality.
.
This package contains the library headers for developers.

review: Needs Fixing (packaging)

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2017-02-21:

There are conflicts with latest trunk.

"""
Text conflict in src/modules/Unity/Application/CMakeLists.txt
Text conflict in src/platforms/mirserver/CMakeLists.txt
Text conflict in src/platforms/mirserver/wrappedwindowmanagementpolicy.cpp
"""

review: Needs Fixing

lp://qastaging/~unity-team/qtmir/qtmir.api updated on 2017-02-22

626. By Nick Dedekind on 2017-02-22: merged trunk
627. By Nick Dedekind on 2017-02-22: packaging fixes
628. By Nick Dedekind on 2017-02-22: updated gles patch
629. By Nick Dedekind on 2017-02-22: GuiServerApplication->MirServerApplication
630. By Nick Dedekind on 2017-02-22: comment updates

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-02-22:

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

review: Approve (continuous-integration)

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-02-22:

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

review: Approve (continuous-integration)

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2017-02-22:

Merges fine now.

review: Abstain

lp://qastaging/~unity-team/qtmir/qtmir.api updated on 2017-03-01

631. By Nick Dedekind on 2017-03-01: merged with parent

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-03-01:

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

review: Approve (continuous-integration)

lp://qastaging/~unity-team/qtmir/qtmir.api updated on 2017-03-03

632. By Michael Zanetti on 2017-03-03: add a dep to libmiral-dev

Revision history for this message

Michael Zanetti (mzanetti) wrote on 2017-03-03:

I've added a dep from to libqtmirserver-dev for libmiral-dev as pkgconfig for libqtmirserver requires that.

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-03-03:

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

review: Approve (continuous-integration)

lp://qastaging/~unity-team/qtmir/qtmir.api updated on 2017-03-10

633. By Nick Dedekind on 2017-03-10: libqtmirserver1
634. By Nick Dedekind on 2017-03-10: updated gles patch

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-03-10:

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

review: Approve (continuous-integration)

lp://qastaging/~unity-team/qtmir/qtmir.api updated on 2017-03-16

635. By Nick Dedekind on 2017-03-16: merged with parent

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-03-16:

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

review: Approve (continuous-integration)

lp://qastaging/~unity-team/qtmir/qtmir.api updated on 2017-03-20

636. By Nick Dedekind on 2017-03-20: merged with parent
637. By Nick Dedekind on 2017-03-20: removed appnotifier and window notifier

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-03-20:

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

review: Approve (continuous-integration)

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-03-20:

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

review: Approve (continuous-integration)

lp://qastaging/~unity-team/qtmir/qtmir.api updated on 2017-03-21

638. By Nick Dedekind on 2017-03-21: review comments
639. By Nick Dedekind on 2017-03-21: consistency

Revision history for this message

Nick Dedekind (nick-dedekind) wrote on 2017-03-21:

> +++ src/platforms/mirserver/displayconfigurationstorage.cpp
>
> last line
> +}
> missing a "// namespace qtmir"
>
>
> +++ src/platforms/mirserver/guiserverapplication.cpp
> +QSharedPointer<QMirServer> mirServer;
> does it need to be shared?

Both the QGuiApp and MirServerIntegration call QMirServer::create, which only holds a weak pointer. If the GuiApp or MirServerIntegration didnt hold shared pointers the QMirServer would be released once the init function is completed and then re-created by the QMirServerIntegration.

>
> around line 36, please add "// namespace" after the anonymous closing brace.
> And at EOF, another comment please. I get lost with namespaces.
>
>
> + : QGuiApplication((init(argc, argv, options), argc), argv) // comma operator
> to ensure init called before QGuiApplication
> clever!
>
>
> +++ src/platforms/mirserver/miral/persist_display_config.cpp
> + if (mode.size == newMode.size && mode.vrefresh_hz == newMode.refresh_rate) {
> refresh_rate comparison is a floating point comparison, could be unreliable.
> Can you use something like qFuzzyCompare?
>

I've included qglobal and used qFuzzyCompare. Shouldn't really be doing that in the miral namespace. Mir doesn't seem to care about dodgey float compare :/

>
> +++ src/platforms/mirserver/mirserverintegration.h
> - QScopedPointer<QMirServer> m_mirServer;
> + QSharedPointer<QMirServer> m_mirServer;
> Does it really need to be shared? AFAICS nobody else makes a copy of the
> QSharedPointer after construction.

It's held by the MirServerApplication if it's being used.

>
>
> +++ src/platforms/mirserver/qmirserver.h
> + static QSharedPointer<QMirServer> create(int &argc,
> + char **argv);
> It's your big entry point header file, make it pretty! Do line up these
> arguments, or just have on single line.
>

Done

>
> +++ src/platforms/mirserver/qmirserver_p.cpp
> +auto buildDisplayConfigurationPolicy()
> +-> std::shared_ptr<miral::DisplayConfigurationPolicy>
> I'm not a fan of this method definition style. Please stick to the old
> fashioned one.

Done

> +++ src/platforms/mirserver/displayconfigurationstorage.cpp
> 
> last line
> +}
> missing a "// namespace qtmir"
> 
> 
> +++ src/platforms/mirserver/guiserverapplication.cpp
> +QSharedPointer<QMirServer> mirServer;
> does it need to be shared?

> 
> around line 36, please add "// namespace" after the anonymous closing brace.
> And at EOF, another comment please. I get lost with namespaces.
> 
> 
> + : QGuiApplication((init(argc, argv, options), argc), argv) // comma operator
> to ensure init called before QGuiApplication
> clever!
> 
> 
> +++ src/platforms/mirserver/miral/persist_display_config.cpp
> + if (mode.size == newMode.size && mode.vrefresh_hz == newMode.refresh_rate) {
> refresh_rate comparison is a floating point comparison, could be unreliable.
> Can you use something like qFuzzyCompare?
>

I've included qglobal and used qFuzzyCompare. Shouldn't really be doing that in the miral namespace. Mir doesn't seem to care about dodgey float compare :/

> 
> +++ src/platforms/mirserver/mirserverintegration.h
> -    QScopedPointer<QMirServer> m_mirServer;
> +    QSharedPointer<QMirServer> m_mirServer;
> Does it really need to be shared? AFAICS nobody else makes a copy of the
> QSharedPointer after construction.

It's held by the MirServerApplication if it's being used.

> 
> 
> +++ src/platforms/mirserver/qmirserver.h
> +    static QSharedPointer<QMirServer> create(int &argc,
> +                                           char **argv);
> It's your big entry point header file, make it pretty! Do line up these
> arguments, or just have on single line.
>

Done

> 
> +++ src/platforms/mirserver/qmirserver_p.cpp
> +auto buildDisplayConfigurationPolicy()
> +-> std::shared_ptr<miral::DisplayConfigurationPolicy>
> I'm not a fan of this method definition style. Please stick to the old
> fashioned one.

Done

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-03-21:

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

review: Approve (continuous-integration)

Revision history for this message

Gerry Boland (gerboland) wrote on 2017-03-22:

+class BasicSetDisplayConfigurationStorage
+{
+public:
+ explicit BasicSetDisplayConfigurationStorage(DisplayConfigurationStorageBuilder const& builder);
+ ~BasicSetDisplayConfigurationStorage() = default;
+
+ void operator()(QMirServer& server);

You're making your life harder having the builder-applyer take QMirServer, instead of mir::Server. All of MirAL's building blocks use mir::Server, so if we go this route, we end up having to replace all of MirAL's bits. I think that's a bad idea.

review: Needs Fixing

Revision history for this message

Gerry Boland (gerboland) wrote on 2017-03-22:

Ok, on further reflection I think I see why you've done that. Because you want to register that implementation with QMirServer, so that you can refer to it later (i.e. it doesn't disappear into the depths of Mir can we cannot get at it again).

Can MirAL give us more api maybe? /me needs to think

Revision history for this message

Albert Astals Cid (aacid) wrote on 2017-03-24:

Text conflict in src/platforms/mirserver/miral/CMakeLists.txt
Text conflict in src/platforms/mirserver/qmirserver_p.cpp
Text conflict in src/platforms/mirserver/wrappedwindowmanagementpolicy.cpp
3 conflicts encountered.

lp://qastaging/~unity-team/qtmir/qtmir.api updated on 2017-03-24

640. By Nick Dedekind on 2017-03-24: merged parent

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-03-24:

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

review: Approve (continuous-integration)

lp://qastaging/~unity-team/qtmir/qtmir.api updated on 2017-03-24

641. By Nick Dedekind on 2017-03-24: added build dir to cmake path

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-03-24:

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

review: Approve (continuous-integration)

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-03-31:

FAILED: Continuous integration, rev:641
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/649/
Executed test runs:
FAILURE: https://unity8-jenkins.ubuntu.com/job/build/4797/console
FAILURE: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/4825/console

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

review: Needs Fixing (continuous-integration)

lp://qastaging/~unity-team/qtmir/qtmir.api updated on 2017-04-03

642. By Nick Dedekind on 2017-04-03: merged with parent

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-04-03:

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

review: Approve (continuous-integration)

lp://qastaging/~unity-team/qtmir/qtmir.api updated on 2017-04-04

643. By Nick Dedekind on 2017-04-04: reduce external WindowManagementPolicy API

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-04-04:

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

review: Approve (continuous-integration)

lp://qastaging/~unity-team/qtmir/qtmir.api updated on 2017-04-05

644. By Nick Dedekind on 2017-04-05: fixed miral include file

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-04-05:

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

review: Approve (continuous-integration)

Unmerged revisions

644. By Nick Dedekind on 2017-04-05: fixed miral include file
643. By Nick Dedekind on 2017-04-04: reduce external WindowManagementPolicy API
642. By Nick Dedekind on 2017-04-03: merged with parent
641. By Nick Dedekind on 2017-03-24: added build dir to cmake path
640. By Nick Dedekind on 2017-03-24: merged parent
639. By Nick Dedekind on 2017-03-21: consistency
638. By Nick Dedekind on 2017-03-21: review comments
637. By Nick Dedekind on 2017-03-20: removed appnotifier and window notifier
636. By Nick Dedekind on 2017-03-20: merged with parent
635. By Nick Dedekind on 2017-03-16: merged with parent

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

to all changes:

Daniel d'Andrada

Mir development team

Unity Team

QtMir

Merge lp://qastaging/~unity-team/qtmir/qtmir.api into lp://qastaging/qtmir

Commit message

Description of the change

Unmerged revisions

Preview Diff

Subscribers