kcm-touchpad in plasma 5.2?

[Rajeesh K Nambiar]

On Sat, Jan 3, 2015 at 7:56 PM, David Edmundson
<david at davidedmundson.co.uk> wrote:
>
>
> On Sat, Jan 3, 2015 at 7:07 PM, Rajeesh K Nambiar
> <rajeeshknambiar at gmail.com> wrote:
>>
>> On Thu, Dec 11, 2014 at 4:30 PM, David Edmundson
>> <david at davidedmundson.co.uk> wrote:
>> > In general this looks OK, there's some useful features and I can see
>> > myself
>> > using this. I'd very much like it to become part of Plasma.
>> >
>> > I gave it a review and made some notes below.
>>
>> Thanks for the review. I cannot answer many of the comments, but some
>> queries below:
>>
>> >
>> > kded.cpp
>> > The touchpad backend leaks?
>>
>> TouchpadBackend::implementation has static variable, would this still
>> cause leak? (especially if there are multiple backends...)
>>
>> >
>> > There are blocking calls in the constructor using isServiceRegistered
>> > combined with the dataengine querying this kded module in a blocking way
>> > in
>> > init we have a strong potential to deadlock the two applications
>> > For KDED modules we have to be a lot more strict to never block as
>> > others
>> > will be querying us.
>>
>> I couldn't find a way to work around this as there's no async
>> alternative to isServiceRegistered. Could Alexander help?
>
>
> Ah, it's a bit obscure.
>
>     QDBusPendingCall async =
> QDBusConnection::sessionBus().interface()->asyncCall(QLatin1String("ListNames"));
>     QDBusPendingCallWatcher *callWatcher = new
> QDBusPendingCallWatcher(async, this);
>     connect(callWatcher, SIGNAL(finished(QDBusPendingCallWatcher*)),
>             this, SLOT(serviceNameFetchFinished(QDBusPendingCallWatcher*)));
>
>
> void serviceNameFetchFinished(QDBusPendingCallWatcher *callWatcher)
> {
>     QDBusPendingReply<QStringList> reply = *callWatcher;
>     callWatcher->deleteLater();
>
>     if (reply.isError()) {
>         qDebug() << "omg wtf bbq";
>         return;
>     }
>
>     QStringList allServices = reply.value();
>     if (allService.contains("MYSERVICE")) {
>       //do stuff
>    }
> }
>

Thank you. I have made some changes and tested it - doesn't seem to
break things. Opened a review request here:
https://git.reviewboard.kde.org/r/121825/ .

>
>>
>> >
>> > I don't understand why we're watching for plasmashell/kglobalaccel
>> > anyway.
>> > Could you explain the rationale here.
>> >
>> > applet:
>> > The applet is completely not ported.
>> >
>> > Applet translations need to go into a differnet .po file with a specific
>> > name
>>
>> Top level Messages.sh does put .{h,cpp} file translations to
>> kcm_touchpad.pot and {qml,js} file translations to
>> plasma_applet_touchpad.pot - could you be more specific if this needs
>> to change?
>>
>> > Copy a Messages.sh from one of the other plasmoids
>> >
>> > KCM:
>> > reverse scrolling doesn't seem to work
>> > "disabled taps and scrolling only" -- The HIG says to avoid negated
>> > checkbox
>> > descriptions.
>>
>> But this makes sense to leave it as it is - as users would want to
>> 'disable' tap+scrolling alone.
>>
>> >
>> > The translation_domain doesn't seem to be set, so kded/kcm won't load
>> > anything
>> >
>> > touchpad backend.h:
>> > This class shouldn't be instantiated directly, don't make the
>> > constructor
>> > public.
>> >
>> > The X backend:
>> > This looked scary so I didn't review it at all.
>>
>>

-- 
Rajeesh