On Sat, Jan 3, 2015 at 7:07 PM, Rajeesh K Nambiar <rajeeshknamb...@gmail.com
> wrote:
> On Thu, Dec 11, 2014 at 4:30 PM, David Edmundson
> <da...@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
}
}
> >
> > 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
>
_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
