CCBUG:384001
Details
Details
- Reviewers
davidedmundson - Group Reviewers
Plasma - Commits
- R120:da91a5224e48: Add a signal notifying a change in the names of running activities.
Diff Detail
Diff Detail
- Repository
- R120 Plasma Workspace
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
Also, not sure adding a signal that has one user only to a shared library is the best approach, especially since .h says this:
37 * @NOTE: This is a placeholder, to be moved into KActivities (which it 38 * wraps) or the Task Manager applet backend.
But this is your call.
| libtaskmanager/activityinfo.cpp | ||
|---|---|---|
| 52 | This screams shared_ptr, for example to a structure holding a consumer and the model. | |
Comment Actions
I agree, but I already petitioned the upstream KActivities lib author for a less awkward API for this and failed, so I had to bite the bullet.
| libtaskmanager/activityinfo.cpp | ||
|---|---|---|
| 52 | I'm not going to refactor this code considerably and risk more breakage for a minor fix, especially as this pattern is used elsewhere in the lib and was reviewed as such already. | |
| libtaskmanager/activityinfo.cpp | ||
|---|---|---|
| 81–86 | They should emit also numberOfRunnigActivitiesChanges, no? | |
Comment Actions
Don't connect too rowsInserted/rowsRemoved.
This is already handled by connecting to KActivities::Consumer::runningActivitiesChanged.
| libtaskmanager/activityinfo.cpp | ||
|---|---|---|
| 81–86 | They don't need to. It's emitted in response to &KActivities::Consumer::runningActivitiesChanged. Actually, doing the reverse might make more sense. Hold on, I'll clean up a bit. | |
Comment Actions
ActivitiesCache has a perfectly good nameChanged signal which is exactly what we want.
I know it's not currently public API, but we should be fixing things properly, not bodging them.
Comment Actions
I inquired upstream about a signal/API a few times and wasn't told about this signal. I will look into it, unless Ivan wants to.
Comment Actions
Looking at the KActivities API more, perhaps not exposing this directly was deliberate of Ivan as otherwise Consumer would probably have all of the Cache signals. Ivan, can you opine on this?
Comment Actions
The activities cache is a private class meant to communicate directly to KAMD. The signals it contains are in the form (activity id, something) and are meant to be private. The KActivities::Info class was meant to wrap these signals into a nice OO API where an activity object (Info) sends signals that a particular property has changed.
I don't like the idea of exposing the same thing multiple times with different APIs. In the 4.x era, we had one function with the (id, something) format and it was removed by request (during API review) during KF5-ization of KActivities library.
One possible solution to all this (though I'm not sure I like it) is to add a class to KActivities API that keeps maintains a collection of Info objects (exposing the cache in a sense) - it could have activityAdded(Info*), activityRemoved(Info*) and activityChanged(Info*) or something similar.
Comment Actions
Such a class would imho be pretty redundant to the model class, so now it's back to whether David wants to accept this patch or not.