[Panel-devel] Plasma and Amarok

[Leo Franchi]

On 7/16/07, Aaron J. Seigo <aseigo at kde.org> wrote:
>
> On Monday 16 July 2007, Leo Franchi wrote:
>
> 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.


sorry about that. i tried to figure out if there was a consistent
coding convention (especially for spaces inside parenthesis) but it
seemed like it was pretty even between
spaces-between-parens and no spaces. as Amarok code uses spaces, that
was my default. anyway, i will
fix up these typographical things :)


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


this is so there is a way to clear the corona. as applets are started
by the user, currently from the controlbox, the contextscene (corona
subclass) needs to know what is on
the corona itself so it can a) save the state b) clear it c) set next
state. as the user plays a track, for example, the corona should clear
the "home" state and display context-
relative information. this is all done in the subclassed ContextScene, but
as the applet list is in the dptr, it needs access to it.



> 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


ok, i don't really know much about krunner so i had no idea of the
repercussions of that change :)

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


also a good idea :)

tomorrow i'll fix up the patch and look to doing things how you have suggested.

leo

--
> 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)
>
> _______________________________________________
> Amarok-devel mailing list
> Amarok-devel at kde.org
> https://mail.kde.org/mailman/listinfo/amarok-devel
>
>
>


-- 
______________________________________________________
Leo Franchi                    angel666 at myrealbox.com
4305 Charlemagne Ct         lfranchi at gmail.com
Austin                                 cell: (650) 704 3680
TX, USA                              home: (650) 329 0125
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/amarok-devel/attachments/20070717/de16256d/attachment.html