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