Code review comment for lp:~unity-team/qtmir/touch_tracing

> === modified file 'debian/rules'
> -DNO_TESTS=1 - please undo, we want tests!
>
> === added file 'demos/qml-demo-client/main.cpp'
> I don't understand what this is useful for. Isn't the "qml" binary sufficient?
>
> === added file 'demos/qml-demo-shell/main.cpp'
> same here.
>

It's so we can get them to work with the benchmark util. The client needs to parse the command line parameters for the socket.

>
> === added file 'src/common/utils.h'
> file name "utils" as vague as possible:) Can you be more specific?

timestamp.h

>
> +++ src/common/utils.cpp
> +qint64 resetStartTime(qint64 timestamp) {
> +qint64 getStartTime_nsec(qint64 timestamp) {
> please stick into an anonymous namespace
>
> The indenting is inconsistent in the whole file.
>
> + QCoreApplication::instance()->setProperty("appStartTime", timestamp);
> I suspect a static variable would do the job just as well.

Used extern functions with a variable. Needs to be shared by the different modules.

>
> + qint64 startTime = getStartTime_nsec(timestamp.count())/1000000;
> std::chrono has nanosecond to milisecond conversion abilities built in, I
> think you can just cast and it works. It would be safer than dividing by
> 1000000. I suspect you can avoid all usages of qint64 as a result.
>
>
> ====
> This compression is a clever idea to work around the difference in timestamp
> units Qt & Mir uses. I'm still curious if instead of tying ourselves to the
> event APIs Qt has, e.g.
>
> void QWindowSystemInterface::handleFrameStrutMouseEvent(QWindow *w,
> ulong timestamp, const QPointF & local, const QPointF & global,
> Qt::MouseButtons b, Qt::KeyboardModifiers mods,
> Qt::MouseEventSource source)
>
> we could make our own handle*Event() methods. In the end, the above method
> simply does:
>
> QWindowSystemInterfacePrivate::MouseEvent * e =
> new QWindowSystemInterfacePrivate::MouseEvent(w, timestamp,
> QWindowSystemInterfacePrivate::FrameStrutMouse,
> QHighDpi::fromNativeLocalPosition(local, w),
> QHighDpi::fromNativePixels(global, w), b, mods, source);
>
> QWindowSystemInterfacePrivate::handleWindowSystemEvent(e);
>
> If we subclassed QWindowSystemInterfacePrivate::MouseEvent, and added the
> timestamp type we want, that event might pass through Qt safely. This would be
> especially usesful for Mir events which have more detail that Qt events have.
>
> To be investigated. For now, your approach is good.

I'll take a look into it when i get a minute.

> ====
>
>
> std::chrono::milliseconds compressTimestamp(std::chrono::nanoseconds
> timestamp);
> Do you need the milliseconds anywhere, as I only see you calling .count() on
> this everywhere it's used. Why not just return ulong?
>
> Also, would be good to document *why* you've done the timestamp
> compression/decompression, for future souls.

Done. in the header.

>
>
> === modified file 'src/platforms/mirserver/tracepoints.tp'
> Typos:
> touchEventDisptach_start
> touchEventDisptach_end
> and these propagate throughout the whole of mirserver.

Fixed.