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

[Ivan Cukic]

> 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?
> 
>  wrote:
>     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.
> 
>  wrote:
>     well, the method fetches the id for a given activity name. in that sense, it seems like a getter. that it may need to create the activity in the nepomuk store is almost a detail?
>     
>     as for creating a new activity, it really seems more natural, at least to me, to create Context objects and manage those. the only complication is that activities have IDs and they have names. *thinks*
>     
>     you know, it seems to me that applications should ALWAYS be refering to activities by ID. i can't think of a single use case where application code should rely on the activity's name. that's something for the user, isn't it?
>     
>     in plasma-desktop, we need to keep a mapping of Containment ID to Activity ID (or, Context::id()), but that's all.

Yes, everything revolves around IDs. The reason this function needs the name as a parameter is so that it could provide a transition from the old system to the new one. (that's why it is ensure and not create or get)

The problem I see in naming it activityID instead of ensureActivityExists (or something as complex as that :) ) is that it could be easier to misunderstand - activityID looks like a getter, while for the same input, it doesn't really return the same value (at least not when the suggestedID is non existent, or when it doesn't refer to an activity with the specified name)

The mapping is currently implicit - Containment -> Context -> Activity


> 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);?
> >
> 
>  wrote:
>     This looks appealing. The problem is that the setActiveContext would have to trigger the context/activity changed signal in the manager.
> 
>  wrote:
>     that can be done behind the scenes quite easily. i'm thinking more about the public API here rather than the implementation; we often make some parts of the internal implementation a little "uglier" so that the public API is more consistent and pretty.

OK. The second question is what to do on setActiveContext(false) - it could make no activity active? I'd rather have setActive without parameters.

BTW, wouldn't it be a bit redundant to have Context::setActiveContext() instead of only Context::setActive()
(widgets have setFocus and not setFocusWidget)


> 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()?
> 
>  wrote:
>     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...)
> 
>  wrote:
>     the name of a Context should be the name associated with the Activity, if any, correct? i don't think we should think too much bout Containment is doing (other than we want to map Containment <-> Context)

Ok, so introducing the Context::name()


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