kcm-touchpad in plasma 5.2?

[Rajeesh K Nambiar]

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?

>
> 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