Wacomtablet moved to kdereview

[Aaron J. Seigo]

On Wednesday, September 29, 2010, Jörg Ehrichs wrote:
> I have moved my Wacomtablet configuration/set-up program into kdereview.

comments on the Plasma part:

if KDE is going to distribute this plugin, the plugin name in the .desktop 
file should look like:

X-KDE-PluginInfo-Name=org.kde.WacomTabletSettings

this call in the ctor:

    setMinimumSize(graphicsWidget()->minimumSize());

is almost certainly wrong. esp since there is a hardcoded minimum size on the 
graphicsWidget() in TabletApplet::buildDialog(). this should probably be done 
instead with size hints, based on the size hints of graphicsWidget. 
QGraphicsLayout can sometimes be a real pain, so if you run into size problems 
due to that, just ping us in #plasma

it would also be very nice if m_tabletInterface would be re-created when comes 
or goes org.kde.Wacom; this can be done with a QDBusServiceWatcher. this wy, 
the user will not need to remove/add the widget (or worse: think they need to 
restart plasma-desktop, or worse yet log out/in) if the service stops/starts. 
this also makes the advice in WacomTabletSettings::onTabletRemoved a bit less 
scary, and makes the whole "just work" much better.

you need to connect the applyClicked signal from the dialog to your 
configAccepted method.

    m_tabletInterface = new QDBusInterface("org.kde.Wacom", "/Tablet", 
"org.kde.Wacom");

is created in both TabletApplet and WacomTabbletSettings. might be nice to 
share that between them.

Tablet::updateProfile opens tabletprofilerc directly only to read one setting 
from it: the list of groups in m_deviceName->text(). it then queries the dbus 
service for the rest of the information. perhaps it would make sense to add 
that to the DBus service as well? (reading config files from within plasmoids 
is generally discouraged as it tends to create maintenance issues; plasmoids 
should be visualizations primarly, and only if possible; data fetching logic 
usually belongs elsewhere)

speaking of DBus services, instead of making multiple calls to the service for 
the profiles list, the current profile and the configuration ... it may make 
sense to have a "get all" method in the dbus service to prevent multiple dbus 
roundtrips. they still aren't the cheapest things in the world.

and i assume that none of the DBus methods in the services used has any 
realistic chance of blocking for any significant period of time? (sync calls 
in plasmoids are only Ok if they are reasonably guaranteed to return 
"immediately")

it would probably also make a lot of sense to add all of that dbus-ery into 
the hotplug dataengine. this is, after all, one more hot plug item, and that 
would allow any widget to easily get at the information (not to mention 
immediately make your config widget remotable :)

also looking at the DBus service, realize that this:

       foreach(const Solid::Device &device, Solid::Device::allDevices()) {

is probably very slow (many seconds) on most systems that will have a tablet 
connected to them. i would instead greatly recommend that you instead use:

	Solid::Device::listFromType(<type>)

if at all possible. since this is expected to go into kded, such things are 
fairly important as any stoppages cause everything to stop due to the single-
thread design.



p.s. it was moderately confusing at first to see that TabletApplet was not the 
Plasma::Applet at all, and that WacomTabletSettings was actully the 
Plasma::Applet. the settings are actually in TabletApplet, in fact. :)

-- 
Aaron J. Seigo
humru othro a kohnu se
GPG Fingerprint: 8B8B 2209 0C6F 7C47 B1EA  EE75 D6B7 2EB1 A7F1 DB43

KDE core developer sponsored by Qt Development Frameworks
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20100929/5557fd17/attachment.sig>