QtMir

Code review comment for lp:~dandrader/qtmir/mousePointer

mousePointer
Merge into trunk

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2015-09-09:

On 09/09/15 10:36, Gerry Boland wrote:
> Review: Needs Fixing
>
> Ok ignore my relativeMovement thing. I'll not object to the usual terminology.
>
> However I fear I need to ask you to rebase on top of
> https://code.launchpad.net/~gerboland/qtmir/multimonitor/+merge/269906
> as there's a few conflicts, and the concept of multiple qwindows does make this more complex.

Done.

>
> + // We will draw our own cursor.
> + add_init_callback([this](){ the_cursor()->hide(); });
> This isn't great, as the mir cursor object is still being created. Can we replace Mir's implementation with our own one? -- not a blocker on this MR, can consider this later.

This will be the u-s-c cursor, and u-s-c will always have its own cursor
no matter what, I think. Don't think it's worth investigating this idea.

>
> ++ src/platforms/mirserver/mirsingleton.cpp
> +qtmir::Mir::~Mir()
> +{
> + m_instance = nullptr;
> +}
> You're not deleting what you potentially "new"ed. QScopedPointer helps prevent such accidents...

I am. The only place that does "new Mir" is Mir::instance() and it's
done only once. This is a simple, no-nonsense, singleton implementation
that does its job AFAICT. You want the global, static, Mir::m_instance
to be a QScopedPointer? I fail to see how would that work and how it
would be better than the current code. If you really want to see a
QScopedPointer there please give me a diff as I didn't get the point.

>
> +++ src/platforms/mirserver/mirsingleton.h
> +private:
> + Mir();
> maybe http://doc.qt.io/qt-5/qobject.html#Q_DISABLE_COPY

Done.

> Using this Mir singleton to save the cursorName will do fine for now, but I'm wary of it being a dumping ground for lots of little things.

Yes, Mir.cursorName is surely not the best solution but anything better
would require way more work. This is like a stop-gap measure. Also given
the uncertainty of the current cursor approach (as we will eventually
move to u-s-c cursor) I didn't want to invest a lot of effort on
something that could evaporate in the near term.

On 09/09/15 10:36, Gerry Boland wrote:
> Review: Needs Fixing
>
> Ok ignore my relativeMovement thing. I'll not object to the usual terminology.
>
> However I fear I need to ask you to rebase on top of 
> https://code.launchpad.net/~gerboland/qtmir/multimonitor/+merge/269906
> as there's a few conflicts, and the concept of multiple qwindows does make this more complex.

Done.

>
> +    // We will draw our own cursor.
> + add_init_callback([this](){ the_cursor()->hide(); });
> This isn't great, as the mir cursor object is still being created. Can we replace Mir's implementation with our own one? -- not a blocker on this MR, can consider this later.

This will be the u-s-c cursor, and u-s-c will always have its own cursor
no matter what, I think. Don't think it's worth investigating this idea.

>
> ++ src/platforms/mirserver/mirsingleton.cpp
> +qtmir::Mir::~Mir()
> +{
> +    m_instance = nullptr;
> +}
> You're not deleting what you potentially "new"ed. QScopedPointer helps prevent such accidents...

I am. The only place that does "new Mir" is Mir::instance() and it's
done only once. This is a simple,  no-nonsense, singleton implementation
that does its job AFAICT. You want the global, static, Mir::m_instance
to be a QScopedPointer? I fail to see how would that work and how it
would be better than the current code. If you really want to see a
QScopedPointer there please give me a diff as I didn't get the point.

>
> +++ src/platforms/mirserver/mirsingleton.h
> +private:
> + Mir();
> maybe http://doc.qt.io/qt-5/qobject.html#Q_DISABLE_COPY

Done.

> Using this Mir singleton to save the cursorName will do fine for now, but I'm wary of it being a dumping ground for lots of little things.

« Back to merge proposal