Code review comment for lp:~dandrader/qtmir/missingTouchEnd-lp1295623

> Things I'd like clarified in a big comment in this code:
> 1. this is a workaround for a bug in Qt? If yes, please add the QTBUG number,
> plus a short textual description of that bug. It/when it gets fixed upstream,
> presumably we can remove this?
> 2. is this also processing slightly screwed up events from Mir so that they
> make sense to Qt? Other toolkits may also choke on this, so we should have a
> Mir bug on this topic - and reference it in this code too.
>
>
> Code review:
> +++ src/platforms/mirserver/qteventfeeder.h
> + class QtWindowSystem {
> This is actually a interface, please call it QtWindowSystemInterface -
> hopefully it doesn't clash with Qt's defined one. Else
> QtMirWindowSystemInterface maybe? Is it a Window really? :)

It's impossible to have a clash with anything coming with because it's defined *inside* QtEventFeeder class. So it's inside QtEventFeeder's namespace sort of. It's also prefixed with "Qt", whereas Qt itself uses just "Q" for prefixing its classes.

Changed it to adhere to the "FooInterface" convention.

>
> +++ src/platforms/mirserver/qteventfeeder.cpp
> + class RealQtWindowSystem
> if you do the above rename, then you can remove "Real" from the name - it
> confused me when I saw it first (though I understand now you mean not-Mock)

Done.

>
> Class is very single-monitor specific, please add a "FIXME - won't work with
> multimonitor" somewhere - I know that the QtEventFeeder has the same issue,
> but I just want to document it clearly for ourselves when we start thinking
> about desktop.

Done.

>
> + int mirMotionAction = event.action & MirEventActionMask;
> const?
>

Done.

> + // Qt needs a happy, sane stream of touch events. So let's make sure we're
> not forwarding
> + // any insanity.
> Add QTBUG here perhaps?

hmm... Not necessarily a bug as you cannot really assume that Qt should cope with a messed up platform abstraction. It would be great if it did, but not a necessity.

>
> + qCWarning(QTMIR_MIR_MESSAGES)
> I think a dedicated logging category for this would make sense.
> QTMIR_INPUT_PROCESSING -> qtmir.mir.input maybe?

Done.

>
> +void QtEventFeederTest::setIrrelevantMockWindowSystemExpectations()
> +{
> + EXPECT_CALL(*mockWindowSystem, hasTargetWindow())
> + .Times(AnyNumber())
> + .WillRepeatedly(Return(true));
> + EXPECT_CALL(*mockWindowSystem, targetWindowGeometry())
> + .Times(AnyNumber())
> + .WillRepeatedly(Return(QRect(0,0,100,100)));
> +}
> Nice method name :D They're not exactly irrelevant though. Since you call it
> before tests run, can't it be run as part of SetUp()?

They are irrelevant because the tests don't care about them. They are like uninteresting implementation details needed to get things working, from the tests point of view.

They can't be part of SetUp() because they have to be done more than once during a test. Everytime Mock::VerifyAndClearExpectations(mockWindowSystem) is called, all expectations are cleared/removed, as the method name says. So you have to set those expectations again after it.

>
> + .Times(1);
> I don't like it alone on it's own line. Almost looks like a statement of its
> own.

It's standard google mock coding style but ok, changed it.

>
> + setIrrelevantMockWindowSystemExpectations();
> why do you keep calling this? You use
> .Times(AnyNumber()).WillRepeatedly(Return(true)) so it should not need
> resetting

Everytime Mock::VerifyAndClearExpectations(mockWindowSystem) is called, all expectations are cleared/removed, as the method name says. So you have to set those expectations again after it. Think of Mock::VerifyAndClearExpectations like a checkpoint. So that you can break down a longer tests into several checkpoints, to reduce overall complexity of the expectations.

>
> +TEST_F(QtEventFeederTest, GenerateMissingTouchEnd)
> This test generates a Press event at (10,10). You check press event passed to
> Qt. Next generate Press event at (20, 20). You ensure this event also
> generates a Release event for the previous event.
> - would help to add this comment to the test. Sorry, but I'm considering
> somebody other than you or I reading this code!
>

Done.