<div dir="ltr"><div><div><div>I've merged touchpad-kcm frameworks branch into master and I'll include it in the Plasma 5.2 Beta release.<br><br></div>I've also made Rajeesh the default assignee for new bugs on bugzilla.<br><br></div>The blocker to releasing it is that it overlaps with ktouchpadenabler somewhat but it doesn't set the default keyboard accelarator right, I've filed a bug and marked it release_blocker and I don't expect to make a stable release until it gets fixed<br><br><a href="https://bugs.kde.org/show_bug.cgi?id=342629">https://bugs.kde.org/show_bug.cgi?id=342629</a><br><br></div>Jonathan<br><br></div><div class="gmail_extra"><br><div class="gmail_quote">On 4 January 2015 at 12:52, 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Sat, Jan 3, 2015 at 7:56 PM, David Edmundson<br>
<div><div class="h5"><<a href="mailto:david@davidedmundson.co.uk">david@davidedmundson.co.uk</a>> wrote:<br>
><br>
><br>
> On Sat, Jan 3, 2015 at 7:07 PM, Rajeesh K Nambiar<br>
> <<a href="mailto:rajeeshknambiar@gmail.com">rajeeshknambiar@gmail.com</a>> wrote:<br>
>><br>
>> 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<br>
>> > 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>
>> Thanks for the review. I cannot answer many of the comments, but some<br>
>> queries below:<br>
>><br>
>> ><br>
>> > kded.cpp<br>
>> > The touchpad backend leaks?<br>
>><br>
>> TouchpadBackend::implementation has static variable, would this still<br>
>> cause leak? (especially if there are multiple backends...)<br>
>><br>
>> ><br>
>> > There are blocking calls in the constructor using isServiceRegistered<br>
>> > combined with the dataengine querying this kded module in a blocking way<br>
>> > 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<br>
>> > others<br>
>> > will be querying us.<br>
>><br>
>> I couldn't find a way to work around this as there's no async<br>
>> alternative to isServiceRegistered. Could Alexander help?<br>
><br>
><br>
> Ah, it's a bit obscure.<br>
><br>
>     QDBusPendingCall async =<br>
> QDBusConnection::sessionBus().interface()->asyncCall(QLatin1String("ListNames"));<br>
>     QDBusPendingCallWatcher *callWatcher = new<br>
> QDBusPendingCallWatcher(async, this);<br>
>     connect(callWatcher, SIGNAL(finished(QDBusPendingCallWatcher*)),<br>
>             this, SLOT(serviceNameFetchFinished(QDBusPendingCallWatcher*)));<br>
><br>
><br>
> void serviceNameFetchFinished(QDBusPendingCallWatcher *callWatcher)<br>
> {<br>
>     QDBusPendingReply<QStringList> reply = *callWatcher;<br>
>     callWatcher->deleteLater();<br>
><br>
>     if (reply.isError()) {<br>
>         qDebug() << "omg wtf bbq";<br>
>         return;<br>
>     }<br>
><br>
>     QStringList allServices = reply.value();<br>
>     if (allService.contains("MYSERVICE")) {<br>
>       //do stuff<br>
>    }<br>
> }<br>
><br>
<br>
</div></div>Thank you. I have made some changes and tested it - doesn't seem to<br>
break things. Opened a review request here:<br>
<a href="https://git.reviewboard.kde.org/r/121825/" target="_blank">https://git.reviewboard.kde.org/r/121825/</a> .<br>
<div class="HOEnZb"><div class="h5"><br>
><br>
>><br>
>> ><br>
>> > I don't understand why we're watching for plasmashell/kglobalaccel<br>
>> > 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>
>> 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>
>><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<br>
>> > checkbox<br>
>> > descriptions.<br>
>><br>
>> But this makes sense to leave it as it is - as users would want to<br>
>> 'disable' tap+scrolling alone.<br>
>><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<br>
>> > constructor<br>
>> > public.<br>
>> ><br>
>> > The X backend:<br>
>> > This looked scary so I didn't review it at all.<br>
>><br>
>><br>
<br>
--<br>
Rajeesh<br>
</div></div><div class="HOEnZb"><div class="h5">_______________________________________________<br>
Plasma-devel mailing list<br>
<a href="mailto:Plasma-devel@kde.org">Plasma-devel@kde.org</a><br>
<a href="https://mail.kde.org/mailman/listinfo/plasma-devel" target="_blank">https://mail.kde.org/mailman/listinfo/plasma-devel</a><br>
</div></div></blockquote></div><br></div>