Code review comment for lp:~lukas-kde/qtmir/defaultKeymap

Revision history for this message
Gerry Boland (gerboland) wrote :

+ connect(Mir::instance(), &Mir::currentKeymapChanged, this, &MirInputDeviceObserver::setKeymap, Qt::DirectConnection);
Why specifying direct? Is it that the slot doesn't live in an object belonging to a QThread (which has no event loop, so queued signal/slot fails to work).

+MirInputDeviceObserver::~MirInputDeviceObserver()
+{
+ m_devices.clear();
+}
is this necessary? Surely the deletion of the QVector will achieve this too?

+ const QString layout = stringList.at(0);
could this be a reference and save a string copy? Similar for variant.

I'm a bit concerned about thread safety. The device_{add,chang,remov}ed
methods are called by Mir, and that is usually done on some Mir internal thread. The object itself belongs to that thread too. However the Mir::currentKeymapChanged signal fires on the QtGui thread, so it is possible for those 2 threads to call applyKeymap almost simultaneously.

I think a little locking is necessary for safety.

Also, could you please add comments on each method explaining the thread situation, as this stuff is super easy to forget.

review: Needs Fixing

« Back to merge proposal