QtMir

Merge lp://staging/~dandrader/qtmir/fake-apps into lp://staging/qtmir

fake-apps
Merge into trunk

Proposed by Daniel d'Andrada on 2017-02-15

Status:	Needs review
Proposed branch:	lp://staging/~dandrader/qtmir/fake-apps
Merge into:	lp://staging/qtmir
Prerequisite:	lp://staging/~dandrader/qtmir/dontPassArgsToMir
Diff against target:	4643 lines (+3782/-59) 47 files modified CMakeLists.txt (+3/-2) debian/control (+3/-2) debian/gles-patches/convert-to-gles.patch (+1/-1) src/common/debughelpers.cpp (+47/-1) src/common/debughelpers.h (+3/-1) src/common/mirserverinterface.h (+46/-0) src/modules/Unity/Application/CMakeLists.txt (+10/-0) src/modules/Unity/Application/application.cpp (+1/-1) src/modules/Unity/Application/application.h (+0/-1) src/modules/Unity/Application/application_manager.cpp (+14/-4) src/modules/Unity/Application/fake/fakeapplicationinfo.cpp (+26/-0) src/modules/Unity/Application/fake/fakeapplicationinfo.h (+93/-0) src/modules/Unity/Application/fake/fakemaliit.cpp (+76/-0) src/modules/Unity/Application/fake/fakemaliit.h (+47/-0) src/modules/Unity/Application/fake/fakemirclient.cpp (+682/-0) src/modules/Unity/Application/fake/fakemirclient.h (+179/-0) src/modules/Unity/Application/fake/fakeprompt.cpp (+317/-0) src/modules/Unity/Application/fake/fakeprompt.h (+157/-0) src/modules/Unity/Application/fake/faketaskcontroller.cpp (+676/-0) src/modules/Unity/Application/fake/faketaskcontroller.h (+157/-0) src/modules/Unity/Application/fake/windowattributes.h (+47/-0) src/modules/Unity/Application/mirtestsingleton.cpp (+211/-0) src/modules/Unity/Application/mirtestsingleton.h (+116/-0) src/modules/Unity/Application/plugin.cpp (+11/-1) src/modules/Unity/Application/resources/screenshots/gmail-webapp.svg (+343/-0) src/modules/Unity/Application/resources/screenshots/ubuntu-weather-app.svg (+201/-0) src/modules/Unity/Application/resources/surfaces.qrc (+39/-0) src/modules/Unity/Application/taskcontroller.cpp (+11/-0) src/modules/Unity/Application/taskcontroller.h (+12/-5) src/platforms/mirserver/CMakeLists.txt (+1/-0) src/platforms/mirserver/cursor.cpp (+5/-4) src/platforms/mirserver/cursor.h (+2/-2) src/platforms/mirserver/logging.cpp (+2/-1) src/platforms/mirserver/logging.h (+2/-1) src/platforms/mirserver/mirserverhooks.cpp (+9/-1) src/platforms/mirserver/mirserverhooks.h (+2/-1) src/platforms/mirserver/mirsingleton.cpp (+29/-1) src/platforms/mirserver/mirsingleton.h (+8/-1) src/platforms/mirserver/pidfetcher.cpp (+34/-0) src/platforms/mirserver/pidfetcher.h (+43/-0) src/platforms/mirserver/qmirserver.cpp (+2/-0) src/platforms/mirserver/qmirserver_p.cpp (+31/-0) src/platforms/mirserver/qmirserver_p.h (+10/-1) src/platforms/mirserver/qteventfeeder.cpp (+63/-23) src/platforms/mirserver/qteventfeeder.h (+4/-0) src/platforms/mirserver/windowmanagementpolicy.cpp (+3/-1) tests/framework/mock_renderable.h (+3/-3)
To merge this branch:	bzr merge lp://staging/~dandrader/qtmir/fake-apps
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Albert Astals Cid (community)			Needs Fixing on 2017-03-24
Unity8 CI Bot (community)	continuous-integration	2017-02-15	Needs Fixing on 2017-03-01
Gerry Boland (community)		2017-02-15	Approve on 2017-03-01
Review via email: mp+317312@code.staging.launchpad.net

This proposal supersedes a proposal from 2017-02-14.

Commit message

Fake applications using internal mir clients when UNITY_TESTING is defined

Description of the change

Prereq-archive: ppa:ci-train-ppa-service/ubuntu/2457

* Are there any related MPs required for this MP to build/function as expected? Please list.
https://code.launchpad.net/~dandrader/unity-api/mirInputTweaks/+merge/316844
https://code.launchpad.net/~dandrader/unity8/testWithQtMir/+merge/316851

* Did you perform an exploratory manual test run of your code change and any related functionality?
Yes

* If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?
N/A

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-02-09: Posted in a previous version of this proposal

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-02-09: Posted in a previous version of this proposal

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Gerry Boland (gerboland) wrote on 2017-02-09: Posted in a previous version of this proposal

Download full text (3.2 KiB)

=== modified file 'src/common/windowcontrollerinterface.h'
+ virtual void modify(const miral::Window &window, miral::WindowSpecification) = 0;
This is opening large functionality to the shell, which makes me suspicious. Digging I see it only used by MirSurface::setMaximumWidth and friends, which are test-only.

I dislike that, as shell should not be updating MirAL's information on surface min/max/inc limits, instead it should be the client (like it will happen in reality).

Can you redo the min/max/inc limit setting to use the client api instead, and remove this change?

=== modified file 'src/modules/Unity/Application/application_manager.cpp'
+ taskController.reset(fakeTaskController);
+ procInfo = fakeTaskController->procInfo();
reset too, for consistency

=== modified file 'src/modules/Unity/Application/application_manager.h'
+ SessionInterface *findSession(const mir::scene::Session* session) const;
+ Application* findApplicationWithSession(const std::shared_ptr<mir::scene::Session> &session);
so these you've made public, but are also just for test use. Do comment about that fact then, just to make it clear.

Also, we need to avoid exposing mirserver apis and use MirAL's api instead. So use miral::Application instead of std::shared_ptr<mir::scene::Session>. findSession doesn't need to take a raw pointer either. In fact, are both needed at all?

+ void onSessionStarting(SessionInterface *session);
is this fixing a bug? If so, would be good to separate that bug fix into a separate MP with a test.

I'll look at the Fake stuff last.

=== modified file 'src/modules/Unity/Application/mirsurface.h'
+ Q_PROPERTY(int width READ width NOTIFY widthChanged)
+ Q_PROPERTY(int height READ height NOTIFY heightChanged)
why? size() gives this info out, and a single change signal too. You know I disapprove of this.

+ // Whether there's a MirSurfaceItem with active focus displaying this surface.
+ // This information is useful in shell tests
+ Q_PROPERTY(bool activeFocus READ activeFocus NOTIFY activeFocusChanged)
+
+ // Whether the MirSurface is exposed (true) or occluded (false)
+ // This information is useful in shell tests
+ Q_PROPERTY(bool exposed READ exposed NOTIFY exposedChanged)
Can you expose the client API for these? Wiring it through shell, you're missing out on testing the Mir->client communication.

+ // Used only by shell tests
+ Q_INVOKABLE void setMinimumWidth(int);
+ Q_INVOKABLE void setMaximumWidth(int);
+ Q_INVOKABLE void setMinimumHeight(int);
+ Q_INVOKABLE void setMaximumHeight(int);
+ Q_INVOKABLE void setWidthIncrement(int);
+ Q_INVOKABLE void setHeightIncrement(int);
Yeah as I said above, I dislike these here.

=== modified file 'src/modules/Unity/Application/mirsurfaceitem.cpp'
What is going on here? What is this mouse & touch counting stuff for? Can't these things could be done in the client, and avoid the test code in the shell?

=== modified file 'src/modules/Unity/Application/session.cpp'
+ const bool focusedBefore = focused();
and on.

Are you fixing a bug here? Can you separate that out into a separate MP with a test?

+ for (int i = 0; i < m_closingSurfaces.count(); ++...

I dislike that, as shell should not be updating MirAL's information on surface min/max/inc limits, instead it should be the client (like it will happen in reality).

Can you redo the min/max/inc limit setting to use the client api instead, and remove this change?

=== modified file 'src/modules/Unity/Application/application_manager.cpp'
+        taskController.reset(fakeTaskController);
+ procInfo = fakeTaskController->procInfo();
reset too, for consistency

=== modified file 'src/modules/Unity/Application/application_manager.h'
+    SessionInterface *findSession(const mir::scene::Session* session) const;
+ Application* findApplicationWithSession(const std::shared_ptr<mir::scene::Session> &session);
so these you've made public, but are also just for test use. Do comment about that fact then, just to make it clear.

+ void onSessionStarting(SessionInterface *session);
is this fixing a bug? If so, would be good to separate that bug fix into a separate MP with a test.

I'll look at the Fake stuff last.

=== modified file 'src/modules/Unity/Application/mirsurface.h'
+    Q_PROPERTY(int width READ width NOTIFY widthChanged)
+    Q_PROPERTY(int height READ height NOTIFY heightChanged)
why? size() gives this info out, and a single change signal too. You know I disapprove of this.

+    // Whether there's a MirSurfaceItem with active focus displaying this surface.
+    // This information is useful in shell tests
+    Q_PROPERTY(bool activeFocus READ activeFocus NOTIFY activeFocusChanged)
+
+    // Whether the MirSurface is exposed (true) or occluded (false)
+    // This information is useful in shell tests
+ Q_PROPERTY(bool exposed READ exposed NOTIFY exposedChanged)
Can you expose the client API for these? Wiring it through shell, you're missing out on testing the Mir->client communication.

+    // Used only by shell tests
+    Q_INVOKABLE void setMinimumWidth(int);
+    Q_INVOKABLE void setMaximumWidth(int);
+    Q_INVOKABLE void setMinimumHeight(int);
+    Q_INVOKABLE void setMaximumHeight(int);
+    Q_INVOKABLE void setWidthIncrement(int);
+    Q_INVOKABLE void setHeightIncrement(int);
Yeah as I said above, I dislike these here.

=== modified file 'src/modules/Unity/Application/session.cpp'
+ const bool focusedBefore = focused();
and on.

Are you fixing a bug here? Can you separate that out into a separate MP with a test?

+ for (int i = 0; i < m_closingSurfaces.count(); ++i) {
foreach nicer

More coming

review: Needs Fixing

Revision history for this message

Gerry Boland (gerboland) wrote on 2017-02-09: Posted in a previous version of this proposal

=== modified file 'src/platforms/mirserver/mirserverhooks.h'
+ mir::Server *server() const;
You need to justify this. Again it is exposing mir server api, which we're trying to avoid.

=== modified file 'src/platforms/mirserver/mirserverintegration.cpp'
-MirServerIntegration::MirServerIntegration(int &argc, char **argv)
+MirServerIntegration::MirServerIntegration()
Why?? This prevents Mir from processing any command line args. You really need to justify this reversal.

review: Needs Fixing

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2017-02-09: Posted in a previous version of this proposal

On 09/02/2017 14:17, Gerry Boland wrote:
> === modified file 'src/modules/Unity/Application/mirsurfaceitem.cpp'
> What is going on here? What is this mouse & touch counting stuff for? Can't these things could be done in the client, and avoid the test code in the shell?

That would be lovely. Any suggestions?

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2017-02-09: Posted in a previous version of this proposal

On 09/02/2017 14:17, Gerry Boland wrote:
> + for (int i = 0; i < m_closingSurfaces.count(); ++i) {
> foreach nicer

That's debatable. I personally find Q_FOREACH to be an eyesore.

Revision history for this message

Gerry Boland (gerboland) wrote on 2017-02-09: Posted in a previous version of this proposal

Instead of exporting the whole of MirServer through the QPA, would be cleaner if you could just expose the bare minimum api needed for the test bits

Revision history for this message

Gerry Boland (gerboland) wrote on 2017-02-09: Posted in a previous version of this proposal

Can you also split the SessionManager->TaskController transition into a separate MP.

Revision history for this message

Gerry Boland (gerboland) wrote on 2017-02-10: Posted in a previous version of this proposal

> On 09/02/2017 14:17, Gerry Boland wrote:
> > + for (int i = 0; i < m_closingSurfaces.count(); ++i) {
> > foreach nicer
>
> That's debatable. I personally find Q_FOREACH to be an eyesore.

I mis-spoke, I meant the C++11 ranged for is nicer.

Revision history for this message

Gerry Boland (gerboland) wrote on 2017-02-10: Posted in a previous version of this proposal

> On 09/02/2017 14:17, Gerry Boland wrote:
> > === modified file 'src/modules/Unity/Application/mirsurfaceitem.cpp'
> > What is going on here? What is this mouse & touch counting stuff for? Can't
> these things could be done in the client, and avoid the test code in the
> shell?
>
> That would be lovely. Any suggestions?

Well I see the fake MirClient::windowEvent receives input events, so that can keep a count. That does mean you need to expose the some test-only data about the mir client to the qmltest, but that should be possible. Quite probably a non-trivial amount of work.

I don't mind if the code isn't perfect first time, I understand that shortcuts need to be made for brevity. If you mention that limitations in the description and justify why, then I'll be happy and can review with that in mind. But if not, I will point them out and ask why.

Revision history for this message

Gerry Boland (gerboland) wrote on 2017-02-10: Posted in a previous version of this proposal

=== added file 'src/modules/Unity/Application/fake/fakemaliit.cpp'
+ MirBufferStream* buffer_stream = mir_window_get_buffer_stream(window);
+ // TODO sometimes buffer_stream is nullptr
+ // (Only observed when creating a lot of clients at once)
Just FYI, this is a deprecated method. The null thing is weird, I'd suggest logging a bug but if it is to be deprecated anyway, not worth the trouble.

+ for (int i = 0; i < m_windows.count(); ++i) {
+ MirWindow *window = m_windows[i];
ranged for a little nicer! It's not an eyesore IMO

=== added file 'src/modules/Unity/Application/fake/fakeprompt.cpp'
+ MirClientEvent(MirWindow* window, const MirEvent *event, QEvent::Type type)
+ : QEvent(type), window(window), mirEvent(event) {
+ mirEvent = mir_event_ref(event);
you're setting mirEvent twice afaics.

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2017-02-10: Posted in a previous version of this proposal

On 10/02/2017 11:53, Gerry Boland wrote:
> === added file 'src/modules/Unity/Application/fake/fakemaliit.cpp'
> + MirBufferStream* buffer_stream = mir_window_get_buffer_stream(window);
> + // TODO sometimes buffer_stream is nullptr
> + // (Only observed when creating a lot of clients at once)
> Just FYI, this is a deprecated method. The null thing is weird, I'd suggest logging a bug but if it is to be deprecated anyway, not worth the trouble.

That's copy-pasted code from alan_g in miral.

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-02-10: Posted in a previous version of this proposal

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

review: Approve (continuous-integration)

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-02-10: Posted in a previous version of this proposal

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

review: Approve (continuous-integration)

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-02-13: Posted in a previous version of this proposal

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-02-13: Posted in a previous version of this proposal

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-02-13: Posted in a previous version of this proposal

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-02-14: Posted in a previous version of this proposal

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-02-14: Posted in a previous version of this proposal

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2017-02-14: Posted in a previous version of this proposal

Download full text (3.7 KiB)

On 09/02/2017 14:17, Gerry Boland wrote:
> Review: Needs Fixing
>
> === modified file 'src/common/windowcontrollerinterface.h'
> + virtual void modify(const miral::Window &window, miral::WindowSpecification) = 0;
> This is opening large functionality to the shell, which makes me suspicious. Digging I see it only used by MirSurface::setMaximumWidth and friends, which are test-only.
>
> I dislike that, as shell should not be updating MirAL's information on surface min/max/inc limits, instead it should be the client (like it will happen in reality).
>
> Can you redo the min/max/inc limit setting to use the client api instead, and remove this change?

Done.

> === modified file 'src/modules/Unity/Application/application_manager.cpp'
> + taskController.reset(fakeTaskController);
> + procInfo = fakeTaskController->procInfo();
> reset too, for consistency
>

Can't do that. fakeTaskController->procInfo() is a QSharedPointer.

> === modified file 'src/modules/Unity/Application/application_manager.h'
> + SessionInterface *findSession(const mir::scene::Session* session) const;
> + Application* findApplicationWithSession(const std::shared_ptr<mir::scene::Session> &session);
> so these you've made public, but are also just for test use. Do comment about that fact then, just to make it clear.

ApplicationManager::findSession() is used by SurfaceManager now that
SessionManager is gone and its responsibilities split between
TaskController (most of it), SurfaceManager (just a little bit) and
ApplicationManager (another tiny piece).
Made findApplicationWithSession() private (I think it was used from
outside earlier in the development but not anymore).

> Also, we need to avoid exposing mirserver apis and use MirAL's api instead. So use miral::Application instead of std::shared_ptr<mir::scene::Session>. findSession doesn't need to take a raw pointer either. In fact, are both needed at all?

findSession() is needed as explained above.

> + void onSessionStarting(SessionInterface *session);
> is this fixing a bug? If so, would be good to separate that bug fix into a separate MP with a test.

No. Again, it's because of the removal of SessionManager.

> === modified file 'src/modules/Unity/Application/mirsurface.h'
> + Q_PROPERTY(int width READ width NOTIFY widthChanged)
> + Q_PROPERTY(int height READ height NOTIFY heightChanged)
> why? size() gives this info out, and a single change signal too. You know I disapprove of this.

Because test code uses it. Easier to copy/paste that from the mock
Unity.Application than to change test code.

Anyway, removed.

> + // Whether there's a MirSurfaceItem with active focus displaying this surface.
> + // This information is useful in shell tests
> + Q_PROPERTY(bool activeFocus READ activeFocus NOTIFY activeFocusChanged)
> +
> + // Whether the MirSurface is exposed (true) or occluded (false)
> + // This information is useful in shell tests
> + Q_PROPERTY(bool exposed READ exposed NOTIFY exposedChanged)
> Can you expose the client API for these? Wiring it through shell, you're missing out on testing the Mir->client communication.

Done.

> + // Used only by shell tests
> + Q_INVOKA...

Done.

> === modified file 'src/modules/Unity/Application/application_manager.cpp'
> +        taskController.reset(fakeTaskController);
> + procInfo = fakeTaskController->procInfo();
> reset too, for consistency
>

Can't do that. fakeTaskController->procInfo() is a QSharedPointer.

> === modified file 'src/modules/Unity/Application/application_manager.h'
> +    SessionInterface *findSession(const mir::scene::Session* session) const;
> + Application* findApplicationWithSession(const std::shared_ptr<mir::scene::Session> &session);
> so these you've made public, but are also just for test use. Do comment about that fact then, just to make it clear.

ApplicationManager::findSession() is used by SurfaceManager now that 
SessionManager is gone and its responsibilities split between 
TaskController (most of it), SurfaceManager (just a little bit) and 
ApplicationManager (another tiny piece).
Made findApplicationWithSession() private (I think it was used from 
outside earlier in the development but not anymore).

findSession() is needed as explained above.

> + void onSessionStarting(SessionInterface *session);
> is this fixing a bug? If so, would be good to separate that bug fix into a separate MP with a test.

No. Again, it's because of the removal of SessionManager.

> === modified file 'src/modules/Unity/Application/mirsurface.h'
> +    Q_PROPERTY(int width READ width NOTIFY widthChanged)
> +    Q_PROPERTY(int height READ height NOTIFY heightChanged)
> why? size() gives this info out, and a single change signal too. You know I disapprove of this.

Because test code uses it. Easier to copy/paste that from the mock 
Unity.Application than to change test code.

Anyway, removed.

> +    // Whether there's a MirSurfaceItem with active focus displaying this surface.
> +    // This information is useful in shell tests
> +    Q_PROPERTY(bool activeFocus READ activeFocus NOTIFY activeFocusChanged)
> +
> +    // Whether the MirSurface is exposed (true) or occluded (false)
> +    // This information is useful in shell tests
> + Q_PROPERTY(bool exposed READ exposed NOTIFY exposedChanged)
> Can you expose the client API for these? Wiring it through shell, you're missing out on testing the Mir->client communication.

Done.

> +    // Used only by shell tests
> +    Q_INVOKABLE void setMinimumWidth(int);
> +    Q_INVOKABLE void setMaximumWidth(int);
> +    Q_INVOKABLE void setMinimumHeight(int);
> +    Q_INVOKABLE void setMaximumHeight(int);
> +    Q_INVOKABLE void setWidthIncrement(int);
> +    Q_INVOKABLE void setHeightIncrement(int);
> Yeah as I said above, I dislike these here.

Fixed.

> === modified file 'src/modules/Unity/Application/session.cpp'
> + const bool focusedBefore = focused();
> and on.
>
> Are you fixing a bug here? Can you separate that out into a separate MP with a test?
>

Done: https://code.launchpad.net/~dandrader/qtmir/sessionFixes/+merge/317189

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2017-02-14: Posted in a previous version of this proposal

On 10/02/2017 11:35, Gerry Boland wrote:
>> On 09/02/2017 14:17, Gerry Boland wrote:
>>> === modified file 'src/modules/Unity/Application/mirsurfaceitem.cpp'
>>> What is going on here? What is this mouse & touch counting stuff for? Can't
>> these things could be done in the client, and avoid the test code in the
>> shell?
>>
>> That would be lovely. Any suggestions?
> Well I see the fake MirClient::windowEvent receives input events, so that can keep a count. That does mean you need to expose the some test-only data about the mir client to the qmltest, but that should be possible. Quite probably a non-trivial amount of work.

Done.

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-02-14: Posted in a previous version of this proposal

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2017-02-14: Posted in a previous version of this proposal

On 09/02/2017 16:26, Gerry Boland wrote:
> Can you also split the SessionManager->TaskController transition into a separate MP.

Done:
https://code.launchpad.net/~dandrader/qtmir/byeSessionManager/+merge/317209

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-02-14: Posted in a previous version of this proposal

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

review: Approve (continuous-integration)

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2017-02-14: Posted in a previous version of this proposal

On 09/02/2017 15:20, Gerry Boland wrote:
> Instead of exporting the whole of MirServer through the QPA, would be cleaner if you could just expose the bare minimum api needed for the test bits

Would it be ok if Unity.Application module got its hands on MirServer
via other means than Qt's NativeInterface? Is the problem that MirServer
is on the NativeInterface and so shell code could access it?

I'm a bit lost here. Not sure where you wanna get to and why. The
internal mir client code is really plain mir and miral has no part in it.

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2017-02-14: Posted in a previous version of this proposal

On 09/02/2017 14:23, Gerry Boland wrote:
> === modified file 'src/platforms/mirserver/mirserverintegration.cpp'
> -MirServerIntegration::MirServerIntegration(int &argc, char **argv)
> +MirServerIntegration::MirServerIntegration()
> Why?? This prevents Mir from processing any command line args. You really need to justify this reversal.

Yes, I meant to ask about it but ended up forgetting to do so due to the
sea of code and multitude of weeks that took to me make these branches.

The problem is, if I recall correctly, that Mir is getting in the way
when qtmir is used in uqmlscene (make tryFoo) and qmltestrunner (make
testFoo). That was the simplest solution. Please advise.

Revision history for this message

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

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

review: Approve (continuous-integration)

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2017-02-15: Posted in a previous version of this proposal

On 10/02/2017 11:53, Gerry Boland wrote:
> + for (int i = 0; i < m_windows.count(); ++i) {
> + MirWindow *window = m_windows[i];
> ranged for a little nicer! It's not an eyesore IMO

Done.

Revision history for this message

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

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

review: Approve (continuous-integration)

Revision history for this message

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

Rest of the code looks good. I need to rebuild everything to check the unity8 tests again

review: Approve (code)

Revision history for this message

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

On 16/02/2017 10:16, Gerry Boland wrote:
> Review: Approve code
>
> Only niggle is updating copyright years to 2017, for fakeapplicationinfo.{h,cpp}

Done.

Revision history for this message

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

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

review: Needs Fixing (continuous-integration)

Revision history for this message

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

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

review: Approve (continuous-integration)

Revision history for this message

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

Yep, still good

review: Approve

Revision history for this message

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

Looks unchanged

review: Approve

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/532/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

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

Text conflict in debian/control
Text conflict in src/platforms/mirserver/qteventfeeder.cpp
2 conflicts encountered.

Was already top approved.

review: Needs Fixing

Unmerged revisions

610. By Daniel d'Andrada on 2017-02-21: Fake applications using internal mir clients when UNITY_TESTING is defined

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