<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Sat, Jan 3, 2015 at 7:07 PM, Rajeesh K Nambiar <span dir="ltr"><<a href="mailto:rajeeshknambiar@gmail.com" target="_blank">rajeeshknambiar@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span class="">On Thu, Dec 11, 2014 at 4:30 PM, David Edmundson<br>
<<a href="mailto:david@davidedmundson.co.uk">david@davidedmundson.co.uk</a>> wrote:<br>
> In general this looks OK, there's some useful features and I can see myself<br>
> using this. I'd very much like it to become part of Plasma.<br>
><br>
> I gave it a review and made some notes below.<br>
<br>
</span>Thanks for the review. I cannot answer many of the comments, but some<br>
queries below:<br>
<span class=""><br>
><br>
> kded.cpp<br>
> The touchpad backend leaks?<br>
<br>
</span>TouchpadBackend::implementation has static variable, would this still<br>
cause leak? (especially if there are multiple backends...)<br>
<span class=""><br>
><br>
> There are blocking calls in the constructor using isServiceRegistered<br>
> combined with the dataengine querying this kded module in a blocking way in<br>
> init we have a strong potential to deadlock the two applications<br>
> For KDED modules we have to be a lot more strict to never block as others<br>
> will be querying us.<br>
<br>
</span>I couldn't find a way to work around this as there's no async<br>
alternative to isServiceRegistered. Could Alexander help?<br></blockquote><div><br></div><div>Ah, it's a bit obscure.</div><div><br></div><div>    QDBusPendingCall async = QDBusConnection::sessionBus().interface()->asyncCall(QLatin1String("ListNames"));</div><div>    QDBusPendingCallWatcher *callWatcher = new QDBusPendingCallWatcher(async, this);</div><div>    connect(callWatcher, SIGNAL(finished(QDBusPendingCallWatcher*)),</div><div>            this, SLOT(serviceNameFetchFinished(QDBusPendingCallWatcher*)));</div><div> </div><div><br></div><div><div>void serviceNameFetchFinished(QDBusPendingCallWatcher *callWatcher)</div><div>{</div><div>    QDBusPendingReply<QStringList> reply = *callWatcher;</div><div><div>    callWatcher->deleteLater();</div></div><div><br></div><div>    if (reply.isError()) {</div><div>        qDebug() << "omg wtf bbq";</div><div>        return;</div><div>    }</div><div><br></div><div>    QStringList allServices = reply.value();</div></div><div>    if (allService.contains("MYSERVICE")) {</div><div>      //do stuff</div><div>   }</div><div>}<br></div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span class=""><br>
><br>
> I don't understand why we're watching for plasmashell/kglobalaccel anyway.<br>
> Could you explain the rationale here.<br>
><br>
> applet:<br>
> The applet is completely not ported.<br>
><br>
> Applet translations need to go into a differnet .po file with a specific<br>
> name<br>
<br>
</span>Top level Messages.sh does put .{h,cpp} file translations to<br>
kcm_touchpad.pot and {qml,js} file translations to<br>
plasma_applet_touchpad.pot - could you be more specific if this needs<br>
to change?<br>
<span class=""><br>
> Copy a Messages.sh from one of the other plasmoids<br>
><br>
> KCM:<br>
> reverse scrolling doesn't seem to work<br>
> "disabled taps and scrolling only" -- The HIG says to avoid negated checkbox<br>
> descriptions.<br>
<br>
</span>But this makes sense to leave it as it is - as users would want to<br>
'disable' tap+scrolling alone.<br>
<div class=""><div class="h5"><br>
><br>
> The translation_domain doesn't seem to be set, so kded/kcm won't load<br>
> anything<br>
><br>
> touchpad backend.h:<br>
> This class shouldn't be instantiated directly, don't make the constructor<br>
> public.<br>
><br>
> The X backend:<br>
> This looked scary so I didn't review it at all.<br>
<br>
<br>
<br>
</div></div><span class=""><font color="#888888">--<br>
Rajeesh<br>
</font></span></blockquote></div><br></div></div>