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

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_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