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

I dislike that it is MirSurface & MirSurfaceItem's responsibility to subscribe themselves to this CoordinateTranslator. I would much prefer using the publish-subscribe model that Qt's signal/slots offer us, where MirSurface{,Item} would signal their creation, and CoordinateTranslator would operate on that.

I also dislike that CoordinateTranslator duplicates the list of MirSurfaces that SurfaceManager already has.

Why did you rule out this possibility: having the MirSurfaceItem position update the position of the corresponding mir::Surface in Mir's model? If Mir's model was roughly correct, we wouldn't need to replace the default CoordinateTranslator implementation. That option strikes me as simpler, with less code in qtmir, and less cross-thread synchronisation to deal with.

Also, I want to avoid us just replacing parts of Mir whenever we feel like it. I'd like you to try using Mir's CoordinateTransform implementation, and if it is unworkable, we should communicate why that is to the Mir team.

+ if (CoordinateTranslator::instance()) {
+ CoordinateTranslator::instance()->registerMirSurface(this, surface.get());
+ }
Why would there not be an instance returned? Having to check this every time is a pain. Singleton pattern requires instance() returns a valid object.

All the custom event stuff you've done is equivalent to a BlockingQueuedConnection afaics. Am curious why you avoided it.