[Panel-devel] Plasma and Amarok

[Aaron J. Seigo]

On Monday 16 July 2007, Leo Franchi wrote:

first, this is pretty exciting for me (yes, i get easily excited ;) ... but to 
have another user of libplasma should really be interesting.

> The only issue with that patch that I see is that I have had to
> introduce a few static QStrings, not inside the private d-pointers
> (because it does not make sense to save a copy of the system-wide
> configuration data with each instance of an applet, for example).  If

you can put static members in Private classes as well. no members, other than 
the dptr, should be in the public classes... so they should be moved to 
Applet::Private and DataEngine::Private as the case may be.. in fact, i think 
we can do even better and only have one static string, see further below..

> Comments, whatever are of course very much appreciated.

comments/thoughts follow; it's a pretty big patch so i have a number of 
comments, hope you don't mind:

- sticking to the coding style is appreciated; it's already messy in a few 
places due to the many hands effect =) e.g. there's no spaces between ()s and 
the contents, so QString( "text/x-plasmoidservicename" ) should just be 
QString("text/x-plasmoidservicename") .. minor nitpick, really, and i won't 
shoot anyone if there a few extra spaces here and there, but it would be nice 
to follow things as closely as possible.

- instead of ThemePath perhaps ThemePrefix?

- why do you need the list of loaded applets from Corona (loadedApplets())? 
(not saying it's not valid, just wondering what the use case is)

- we'll need a QString Corona::appletMimeType so that we can get rid of 
hardcoded uses of the mimetype in applications where appropriate

- in Applet, i don't think it's necessary to change X-PlasmoidCategory since 
that is just used to mark categories in the .desktop files. why should that 
change?

- the other two static setters introduced in Applet are really just changing 
the same string (Plasma, or plasma, as the case may be). we have the same 
issue in DataEngineManager again. 

i wonder if it doesn't make sense to just use one setter for all of these. we 
could easily have a single setter/getter pair in plasma.cpp for a static 
QString in the Plasma namespace that sets/gets the prefix. this can then be 
used whereever we do KTrader lookups, etc..

so in plasma we'd use Plasma (making Plasma/Applet, Plasma/DataEngine, 
Plasma_foorc) and in amarok you might use Amarok (making Amarok/Applet and 
amarok_foorc). the two static strings can be kept (in the Private class) but 
having just one method for this would be nice. this would bring the number of 
calls to set up libplasma for use in other apps from 5 down to 2 (one for the 
theme prefix and one for the applet prefix). if we use this same approach for 
the drag-and-drop mimetype, we could even get rid of one more setter.

the downside to this approach is that, while simpler to use, it means that 
it's an all-or-nothing approach ... it wouldn't be possible to use 
DataEngines for plasma in amarok and vice versa as a side effect of setting 
up separate applets. 

hm ... for that matter, i'm really not sure why we need to namespace 
DataEngines at all? they are only requested by applets so the user never sees 
them; we just need to be sure to avoid name collissions which should be 
pretty easy to do. that removes the whole "namespacing applets affects 
dataengines" issue with the above approach.

we'd probably want to add some convenience methods to Plasma:: as well for 
the "final" products (applet service type, drag and drop mimetype, etc)

- in theme.cpp there is this:

-    KConfig config( "plasmarc" );
-    KConfigGroup group( &config, "Theme" );
+    KConfigGroup group = KGlobal::config()->group( "Theme" );

this will break krunner which shares the theming with plasma to create a 
consistent look, which is really the whole point of desktoptheme. =)

perhaps this should be re-thought a bit.... if we have the prefix setter, we 
could put the theme setting in its own group Theme-$Prefix? this would work 
for the time being until someone comes up with a good reason for why they 
need their own svg theme but not applets, or vice versa

- speaking of Plasma::Theme i don't know if changing from desktoptheme/ is 
really necessary. as long as the theme setting can be changed independantly, 
there's no reason other theme sets couldn't be stored in desktoptheme/ as 
well, perhaps prefixed if need be with amarok- (or whatever). another setter 
and qstring member killed! 

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

Full time KDE developer sponsored by Trolltech (http://www.trolltech.com)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://mail.kde.org/pipermail/amarok-devel/attachments/20070716/09334d68/attachment.pgp