Code review comment for lp://staging/~dandrader/qtmir/fake-apps

=== 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) {
foreach nicer

More coming