Review Request: Plasma + Nepomuk - libplasma patch - attempt 1

[Ivan Cukic]

> On 2009-09-23 21:28:50, Aaron Seigo wrote:
> > i'm going to concentrate on the GlobalContext class mostly here. as it stands, GlobalContext seems like an activity manager; where "activity" is defined as "a named object stored in nepomuk". is that correct?
> > 
> > if so, then it's not really a global Context object. perhaps an ActivityManager? or a ContextManager? it doesn't include location information either, which Plasma::Context should at some point here.
> > 
> > GlobalContext public API may only need to be: a signal noting when the active global context changes; a way to list available activities (mostly because the method in Plasma::Context isn't static) ...

I'd go for ContextManager if we are going to add location etc. to it.


> On 2009-09-23 21:28:50, Aaron Seigo wrote:
> > /trunk/KDE/kdelibs/plasma/containment.cpp, line 1846
> > <http://reviewboard.kde.org/r/1699/diff/1/?file=11890#file11890line1846>
> >
> >     kDebug? :)

I stopped using kdebug for some time now for some superstitious reasons :)


> On 2009-09-23 21:28:50, Aaron Seigo wrote:
> > /trunk/KDE/kdelibs/plasma/context.h, line 67
> > <http://reviewboard.kde.org/r/1699/diff/1/?file=11891#file11891line67>
> >
> >     ensureActivityExists?
> >     
> >     or maybe ensureActivity and createActivity could be merged? maybe something like:
> >     
> >     activityId(const QString &name, const QString &expectedId = QString());?
> >     
> >     or perhaps these can remain as they are but be private API? it should only really be needed by Context when setting the current activity in that Context?

It would be ok to rename to ensureActivityExists

activityId is a bit to vague - it doesn't say what it does at all (it looks like a getter for a field while it can create a new activity)

The idea was for an application/applet/whatever to be able to create a new activity - then, plasma (and other apps) would react to that, by creating a new plasma-activity.


> On 2009-09-23 21:28:50, Aaron Seigo wrote:
> > /trunk/KDE/kdelibs/plasma/context.h, line 83
> > <http://reviewboard.kde.org/r/1699/diff/1/?file=11891#file11891line83>
> >
> >     is this equivalent to currentContext()->name()?

hm... Context doesn't have a name. Containment has, and it can be different - if the activity name is empty, the Containment name is some generic string (Desktop/Panel...)


> On 2009-09-23 21:28:50, Aaron Seigo wrote:
> > /trunk/KDE/kdelibs/plasma/context.h, line 78
> > <http://reviewboard.kde.org/r/1699/diff/1/?file=11891#file11891line78>
> >
> >     or perhaps:
> >     
> >     forActivity(id)->setActiveContext(true);?
> >

This looks appealing. The problem is that the setActiveContext would have to trigger the context/activity changed signal in the manager.


> On 2009-09-23 21:28:50, Aaron Seigo wrote:
> > /trunk/KDE/kdelibs/plasma/context.h, lines 93-99
> > <http://reviewboard.kde.org/r/1699/diff/1/?file=11891#file11891line93>
> >
> >     i'd personally expect these as static methods in Plasma::Context, e.g.:
> >     
> >     Plasma::Context::contextForActivity(QString)
> >     Plasma::Context::activeContext()

Ok


> On 2009-09-23 21:28:50, Aaron Seigo wrote:
> > /trunk/KDE/kdelibs/plasma/context.h, line 105
> > <http://reviewboard.kde.org/r/1699/diff/1/?file=11891#file11891line105>
> >
> >     is this equivalent to:
> >     
> >     forActivity(id)->name(); ?

Again, the context doesn't have a name


> On 2009-09-23 21:28:50, Aaron Seigo wrote:
> > /trunk/KDE/kdelibs/plasma/context.h, line 137
> > <http://reviewboard.kde.org/r/1699/diff/1/?file=11891#file11891line137>
> >
> >     wouldn't this create an activity and associate it with this Context?

It could, although the name doesn't suggest it.


> On 2009-09-23 21:28:50, Aaron Seigo wrote:
> > /trunk/KDE/kdelibs/plasma/context.h, line 151
> > <http://reviewboard.kde.org/r/1699/diff/1/?file=11891#file11891line151>
> >
> >     setCurrentActivityId()? and then there could be a corresponding activityId().

the setCurrentActivity is an old function - so we can't really change the name. We could deprecate it as well, and introduce the one with the ID but (IMO) that would be too much...

I've mostly made decisions with a thought that I should change the api as little as possible.


- Ivan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/1699/#review2433
-----------------------------------------------------------


On 2009-09-23 20:19:36, Ivan Cukic wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1699/
> -----------------------------------------------------------
> 
> (Updated 2009-09-23 20:19:36)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> Nepomuk-based storage of plasma activities
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdelibs/plasma/CMakeLists.txt 1027137 
>   /trunk/KDE/kdelibs/plasma/applet.cpp 1027137 
>   /trunk/KDE/kdelibs/plasma/containment.h 1027137 
>   /trunk/KDE/kdelibs/plasma/containment.cpp 1027137 
>   /trunk/KDE/kdelibs/plasma/context.h 1027137 
>   /trunk/KDE/kdelibs/plasma/context.cpp 1027137 
>   /trunk/KDE/kdelibs/plasma/private/activitieshandler.h PRE-CREATION 
>   /trunk/KDE/kdelibs/plasma/private/activitieshandler.cpp PRE-CREATION 
>   /trunk/KDE/kdelibs/plasma/private/context_p.h PRE-CREATION 
>   /trunk/KDE/kdelibs/plasma/private/desktoptoolbox.cpp 1027137 
>   /trunk/KDE/kdelibs/plasma/private/fallbackactivitieshandler.h PRE-CREATION 
>   /trunk/KDE/kdelibs/plasma/private/fallbackactivitieshandler.cpp PRE-CREATION 
>   /trunk/KDE/kdelibs/plasma/private/nepomukactivitieshandler.h PRE-CREATION 
>   /trunk/KDE/kdelibs/plasma/private/nepomukactivitieshandler.cpp PRE-CREATION 
> 
> Diff: http://reviewboard.kde.org/r/1699/diff
> 
> 
> Testing
> -------
> 
> Needs more testing
> 
> 
> Thanks,
> 
> Ivan
> 
>