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? :)

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

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.

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

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

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

+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()?

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

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

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

Otherwise looks good.

Will do functional testing now