[Panel-devel] Plasma and Amarok

[Aaron J. Seigo]

On Tuesday 17 July 2007, Leo Franchi wrote:
> On 7/17/07, Aaron J. Seigo <aseigo at kde.org> wrote:
> > On Monday 16 July 2007, Leo Franchi wrote:
> > > On 7/16/07, Aaron J. Seigo <aseigo at kde.org> wrote:
> > > > 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
> >
> > it's mostly my fault. we started with the same style i used in kicker,
> > then
> > moved to the kdelibs style when it arrived, and then i realized that the
> > kdelibs style does not do spaces within ()s. so... yeah. we went through
> > one
> > major and one minor revolution already. mix in some deviations here and
> > there
> > from various committers that innevitably slip in and i can see how it
> > would
> > be easy to get mistaken here.
> >
> > > > - 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.
> >
> > ah! ok.. this makes things much clearer. i was just about to add a
> > clearApplets() method to Corona, but figured i might end up causing you
> > some
> > conflicts doing that.. so i'll add it after you commit your patch, unless
> > you
> > add it to your patch. that should take care of one of the use cases..
> >
> > the other one, switching layouts, is very interesting. Alexis has just
> > started
> > working on saving/restoring applet locations and one of the eventual
> > goals has always been to allow one to save/restore layouts at runtime by
> > passing in
> > a different configuration. so perhaps you may not need access to the
> > applets
> > themselves directly ... i'm ok with keeping the loadedApplets() method
> > there
> > (it might be useful for someone and can get you guys going in the
> > meantime)
> > but i'm glad i know the use case you have now so we can work towards
> > supporting that sooner...
>
> ah... thats interesting. i'll wait and see what comes out of the work
> on saving/restoring
> sessions.
>
>
> i also assume that context menus will become an issue.
>
> > Corona::contextMenuEvent will be problematic for amarok, i assume. in
> > fact,
> > it's already going to be an issue for plasma with panels. i'll look into
> > a solution for that here...
>
> yeah, i had kind of ignored that issue.
>
> > > 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 :)
> >
> > that's why we do peer review =)
> >
> > > tomorrow i'll fix up the patch and look to doing things how you have
> > > suggested.
> >
> > great =) i'm sure we'll get your patch in within the next day or so.
>
> ok, i attach the new patch. a few things: first of all, i did things
> the "first way" that you talked about, using a global prefix variable
> with getters/setters, instead of a global KTrader namespace with more
> constraints.

+// prefix of applets and data engines, defaults to Plasma => Plasma/Applet 
and Plasma/DataEngine 
+static QString prefix = "Plasma";

that should be in plasma.cpp not plasma.h. in fact, i think we may be able to 
do away with this altogether ......

as for this comment in theme.cpp:
+    // TODO find a good modular way of doing this

see the plasmaperaAppThemes.diff patch =)

> the reason is this: in order to add more constraints we 
> would need to add more getters
> and setters to each individual class that needs them,

either that or add more parameters to the methods in question... and then it's 
only in certain places. looking at it some more, it occurs to me that it's 
pretty meaningless to add this to ::loadApplet since we're already using the 
library name which needs to be unique anyways. so as long as you prefix your 
applets decently to ensure unique names, e.g. add amarok somewhere in there, 
you're all fine without any changes to KServiceTypeTrader calls in 
loadApplet.

and i don't think we need to touch DataEngine here as those are created on 
demand in response to applets. prefacing their name with "amarok" should be 
enough of a namespacing there. this is pretty much how things are handled for 
anything stored in ksycoca anyways, so we should be good.

this leaves us with modifying knownApplets() and knownCategories() since those 
are things that are shown to the user. which is easy to do. see attached 
patch.

this pretty much solves the entire issue, but in a much simpler way.

this leaves us with the context menu issue open, and your patch to set the 
mimetype is still valid. this would mean that setting up libplasma for use in 
amarok would be something like:

Corona* corona = new Corona(this);
corona->setAppletMimeType("x-amarokapplet");
Theme::self()->setApplicationName("amarok");

and then whenever you wanted to get a listing of applets or categories, it 
would look sth like this:

QStringList applets = Plasma::Applet::knownApplets("amarok");

and as long as the amarok specific applets have a X-KDE-ParentApp=amarok entry 
in their .desktop files, all is good. this should also prevent them from 
showing up in plasma. this also allows one to grab several sets of applets 
should they so choose.

how's that?

hm.. looking at it even more here, i think i'm going to add a category 
parameter to the knownApplets method so we can load applets from a given 
category (we need this for the desktop toolbox anyways). this would allow 
amarok to have applets that would be useful to both plasma and amarok in 
an "Amarok" category and to list those easily.

all together this provides for the following three scenarios:

- amarok can access plasma applets
- amarok can access applets specific to amarok
- plasma can access applets from amarok that are also useful on the desktop

i'm pretty happy with this now. and if you agree, please commit the mimetype 
part of your patch and i'll commit the two attached patches and we should be 
rockin'

-- 
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 Trolltech
-------------- next part --------------
A non-text attachment was scrubbed...
Name: plasma_appletParentApp.diff
Type: text/x-diff
Size: 3424 bytes
Desc: not available
Url : http://mail.kde.org/pipermail/amarok-devel/attachments/20070717/9bed9565/attachment-0002.bin 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: plasma_perAppThemes.diff
Type: text/x-diff
Size: 2520 bytes
Desc: not available
Url : http://mail.kde.org/pipermail/amarok-devel/attachments/20070717/9bed9565/attachment-0003.bin 
-------------- 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/20070717/9bed9565/attachment-0001.pgp