Code review comment for lp://qastaging/~unity-team/qtmir/qtmir.api

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