Differential D22381 Diff 67278 src/service/Activities.cpp

Changeset View

Standalone View

View Options

src/service/Activities.cpp

Show First 20 Lines • Show All 45 Lines • ▼ Show 20 Line(s)
46#include "DebugActivities.h" 46​#include "DebugActivities.h"
47#include "activitiesadaptor.h" 47​#include "activitiesadaptor.h"
48#include "ksmserver/KSMServer.h" 48​#include "ksmserver/KSMServer.h"
49#include "common/dbus/common.h" 49​#include "common/dbus/common.h"
50 50​
51// Private 51​// Private
52#define ACTIVITY_MANAGER_CONFIG_FILE_NAME QStringLiteral("kactivitymanagerdrc") 52​#define ACTIVITY_MANAGER_CONFIG_FILE_NAME QStringLiteral("kactivitymanagerdrc")
53 53​
54​namespace {
ivanUnsubmitted
Done

You can use anonymous namespace for this (instead of static) or just make it a non-static function.

It can be marked as inline, although the compiler will probably do that regardless of you saying so.

You can rename it to something like nameBasedOrdering - better communicates what it does.

ivan: You can use anonymous namespace for this (instead of `static`) or just make it a non-static…
55​ inline
56​ bool nameBasedOrdering(const ActivityInfo &info, const ActivityInfo &other)
ivanUnsubmitted
Done

Nitpick Base -> Based

ivan: Nitpick Base -> Based
57​ {
58​ const auto comp =
59​ QString::compare(info.name, other.name, Qt::CaseInsensitive);
60​ return comp < 0 || (comp == 0 && info.id < other.id);
61​ }
62​}
63​
54Activities::Private::KDE4ConfigurationTransitionChecker::KDE4ConfigurationTransitionChecker() 64​Activities::Private::KDE4ConfigurationTransitionChecker::KDE4ConfigurationTransitionChecker()
55{ 65​{
56 // Checking whether we need to transfer the KActivities/KDE4 66​ // Checking whether we need to transfer the KActivities/KDE4
57 // configuration file to the new location. 67​ // configuration file to the new location.
58 const QString newConfigLocation 68​ const QString newConfigLocation
59 = QStandardPaths::writableLocation(QStandardPaths::ConfigLocation) 69​ = QStandardPaths::writableLocation(QStandardPaths::ConfigLocation)
60 + QLatin1Char('/') + ACTIVITY_MANAGER_CONFIG_FILE_NAME; 70​ + QLatin1Char('/') + ACTIVITY_MANAGER_CONFIG_FILE_NAME;
61 71​
▲ Show 20 Lines • Show All 76 Lines • ▼ Show 20 Line(s)145 } else if (!atLeastOneRunning) {
138 // configuration file is in a big problem and told us there 148​ // configuration file is in a big problem and told us there
139 // are no running activities, and enlists all of them as stopped. 149​ // are no running activities, and enlists all of them as stopped.
140 // In that case, we will pretend all of them are running 150​ // In that case, we will pretend all of them are running
141 qCWarning(KAMD_LOG_ACTIVITIES) << "The config file enlisted all activities as stopped"; 151​ qCWarning(KAMD_LOG_ACTIVITIES) << "The config file enlisted all activities as stopped";
142 for (const auto &keys: activities.keys()) { 152​ for (const auto &keys: activities.keys()) {
143 activities[keys] = Activities::Running; 153​ activities[keys] = Activities::Running;
144 } 154​ }
145 } 155​ }
156​
157​ QMetaObject::invokeMethod(
158​ this,
159​ "updateSortedActivityList",
160​ Qt::QueuedConnection);
161​}
162​
163​void Activities::Private::updateSortedActivityList() {
164​ QVector<ActivityInfo> a;
165​ for (const auto &activity : activities.keys()) {
166​ a.append(q->ActivityInformation(activity));
167​ }
168​
169​ std::sort(a.begin(), a.end(), &nameBasedOrdering);
170​
171​ QWriteLocker lock(&activitiesLock);
172​ sortedActivities = a;
146} 173​}
147 174​
148void Activities::Private::loadLastActivity() 175​void Activities::Private::loadLastActivity()
149{ 176​{
150 // This is called from constructor, no need for locking 177​ // This is called from constructor, no need for locking
151 178​
152 // If there are no public activities, try to load the last used activity 179​ // If there are no public activities, try to load the last used activity
153 const auto lastUsedActivity 180​ const auto lastUsedActivity
▲ Show 20 Lines • Show All 45 Lines • ▼ Show 20 Line(s)195{
199 226​
200 scheduleConfigSync(); 227​ scheduleConfigSync();
201 228​
202 emit q->CurrentActivityChanged(activity); 229​ emit q->CurrentActivityChanged(activity);
203 230​
204 return true; 231​ return true;
205} 232​}
206 233​
234​bool Activities::Private::previousActivity()
235​{
236​ const auto a = q->ListActivities(Activities::Running);
237​
238​ for (int i = 0; i < a.count(); ++i) {
239​ if (a[i] == currentActivity) {
240​ return setCurrentActivity(a[(i + a.size() - 1) % a.size()]);
241​ }
242​ }
243​
244​ return false;
245​}
ivanUnsubmitted
Done

I don't like the fact that it constantly resorts the activities.

The second problem is that ListActivities returns a list in one order, and this traverses activities in another order. If this will work by name, probably the List functions should as well.

ivan: I don't like the fact that it constantly resorts the activities. The second problem is that…
muesliAuthorUnsubmitted
Done

List probably returns a fairly randomized (per session) list, seeing how it's returning keys() from a QHash. Maybe I misunderstood you on IRC, I thought you'd rather not store a sorted list here. Instead of keeping a sorted cache though, maybe we should just keep the original list in a sorted order?

muesli: List probably returns a fairly randomized (per session) list, seeing how it's returning keys()…
246​
247​bool Activities::Private::nextActivity()
248​{
249​ const auto a = q->ListActivities(Activities::Running);
250​
251​ for (int i = 0; i < a.count(); ++i) {
ivanUnsubmitted
Not Done

Can be done with std::find_if as well

ivan: Can be done with `std::find_if` as well
252​ if (a[i] == currentActivity) {
253​ return setCurrentActivity(a[(i + 1) % a.size()]);
254​ }
255​ }
256​
257​ return false;
258​}
259​
207QString Activities::Private::addActivity(const QString &name) 260​QString Activities::Private::addActivity(const QString &name)
208{ 261​{
209 QString activity; 262​ QString activity;
210 263​
211 if (name.isEmpty()) { 264​ if (name.isEmpty()) {
212 Q_ASSERT(!name.isEmpty()); 265​ Q_ASSERT(!name.isEmpty());
213 return activity; 266​ return activity;
214 } 267​ }
Show All 14 Lines271 {
229 activities[activity] = Invalid; 282​ activities[activity] = Invalid;
230 activitiesCount = activities.size(); 283​ activitiesCount = activities.size();
231 } 284​ }
232 285​
233 setActivityState(activity, Running); 286​ setActivityState(activity, Running);
234 287​
235 q->SetActivityName(activity, name); 288​ q->SetActivityName(activity, name);
236 289​
290​ updateSortedActivityList();
291​
237 emit q->ActivityAdded(activity); 292​ emit q->ActivityAdded(activity);
238 293​
239 scheduleConfigSync(); 294​ scheduleConfigSync();
240 295​
241 if (activitiesCount == 1) { 296​ if (activitiesCount == 1) {
242 q->SetCurrentActivity(activity); 297​ q->SetCurrentActivity(activity);
243 } 298​ }
244 299​
Show All 25 Lines304{
270 325​
271 bool currentActivityDeleted = false; 326​ bool currentActivityDeleted = false;
272 327​
273 { 328​ {
274 QWriteLocker lock(&activitiesLock); 329​ QWriteLocker lock(&activitiesLock);
275 // Removing the activity 330​ // Removing the activity
276 activities.remove(activity); 331​ activities.remove(activity);
277 332​
333​ for (int i = 0; i < sortedActivities.count(); ++i) {
ivanUnsubmitted
Not Done

This can be done with std::find_if and then remove with iterator.

ivan: This can be done with `std::find_if` and then remove with iterator.
334​ if (sortedActivities[i].id == activity) {
335​ sortedActivities.remove(i);
336​ break;
337​ }
338​ }
339​
278 // If the removed activity was the current one, 340​ // If the removed activity was the current one,
279 // set another activity as current 341​ // set another activity as current
280 currentActivityDeleted = (currentActivity == activity); 342​ currentActivityDeleted = (currentActivity == activity);
281 } 343​ }
282 344​
ivanUnsubmitted
Done

You can just find the activity in the list, and remove it - the order for the rest will not change.

ivan: You can just find the activity in the list, and remove it - the order for the rest will not…
283 activityNameConfig().deleteEntry(activity); 345​ activityNameConfig().deleteEntry(activity);
284 activityDescriptionConfig().deleteEntry(activity); 346​ activityDescriptionConfig().deleteEntry(activity);
285 activityIconConfig().deleteEntry(activity); 347​ activityIconConfig().deleteEntry(activity);
286 348​
287 if (currentActivityDeleted) { 349​ if (currentActivityDeleted) {
288 ensureCurrentActivityIsRunning(); 350​ ensureCurrentActivityIsRunning();
289 } 351​ }
290 352​
▲ Show 20 Lines • Show All 74 Lines • ▼ Show 20 Line(s)424 if (configNeedsUpdating) {
365 mainConfig().writeEntry("runningActivities", 427​ mainConfig().writeEntry("runningActivities",
366 activities.keys(Activities::Running) 428​ activities.keys(Activities::Running)
367 + activities.keys(Activities::Starting)); 429​ + activities.keys(Activities::Starting));
368 mainConfig().writeEntry("stoppedActivities", 430​ mainConfig().writeEntry("stoppedActivities",
369 activities.keys(Activities::Stopped) 431​ activities.keys(Activities::Stopped)
370 + activities.keys(Activities::Stopping)); 432​ + activities.keys(Activities::Stopping));
371 scheduleConfigSync(); 433​ scheduleConfigSync();
372 } 434​ }
435​
436​ updateSortedActivityList();
373} 437​}
374 438​
375void Activities::Private::ensureCurrentActivityIsRunning() 439​void Activities::Private::ensureCurrentActivityIsRunning()
376{ 440​{
377 // If the current activity is not running, 441​ // If the current activity is not running,
378 // make some other activity current 442​ // make some other activity current
379 443​
380 const auto runningActivities = q->ListActivities(Activities::Running); 444​ const auto runningActivities = q->ListActivities(Activities::Running);
▲ Show 20 Lines • Show All 85 Lines • ▼ Show 20 Line(s)529{
466 // Public method can not put us in a limbo state 530​ // Public method can not put us in a limbo state
467 if (activity.isEmpty()) { 531​ if (activity.isEmpty()) {
468 return false; 532​ return false;
469 } 533​ }
470 534​
471 return d->setCurrentActivity(activity); 535​ return d->setCurrentActivity(activity);
472} 536​}
473 537​
538​bool Activities::PreviousActivity()
539​{
540​ return d->previousActivity();
541​}
542​
543​bool Activities::NextActivity()
544​{
545​ return d->nextActivity();
546​}
547​
474QString Activities::AddActivity(const QString &name) 548​QString Activities::AddActivity(const QString &name)
475{ 549​{
476 // We do not care about authorization if this is the first start 550​ // We do not care about authorization if this is the first start
477 if (!d->activities.isEmpty() && 551​ if (!d->activities.isEmpty() &&
478 !KAuthorized::authorize(QStringLiteral("plasma-desktop/add_activities"))) { 552​ !KAuthorized::authorize(QStringLiteral("plasma-desktop/add_activities"))) {
479 return QString(); 553​ return QString();
480 } 554​ }
481 555​
482 return d->addActivity(name); 556​ return d->addActivity(name);
483} 557​}
484 558​
485void Activities::RemoveActivity(const QString &activity) 559​void Activities::RemoveActivity(const QString &activity)
486{ 560​{
487 if (!KAuthorized::authorize(QStringLiteral("plasma-desktop/add_activities"))) { 561​ if (!KAuthorized::authorize(QStringLiteral("plasma-desktop/add_activities"))) {
488 return; 562​ return;
489 } 563​ }
490 564​
491 d->removeActivity(activity); 565​ d->removeActivity(activity);
492} 566​}
493 567​
494QStringList Activities::ListActivities() const 568​QStringList Activities::ListActivities() const
495{ 569​{
496 QReadLocker lock(&d->activitiesLock); 570​ QReadLocker lock(&d->activitiesLock);
497 return d->activities.keys(); 571​
572​ QStringList s;
573​ for (const auto &a : d->sortedActivities) {
574​ s << a.id;
575​ }
576​ return s;
498} 577​}
499 578​
500QStringList Activities::ListActivities(int state) const 579​QStringList Activities::ListActivities(int state) const
501{ 580​{
502 QReadLocker lock(&d->activitiesLock); 581​ QReadLocker lock(&d->activitiesLock);
503 return d->activities.keys((State)state); 582​
583​ QStringList s;
584​ for (const auto &a : d->sortedActivities) {
585​ if (a.state == (State)state) {
586​ s << a.id;
587​ }
588​ }
589​ return s;
504} 590​}
505 591​
506QList<ActivityInfo> Activities::ListActivitiesWithInformation() const 592​QList<ActivityInfo> Activities::ListActivitiesWithInformation() const
507{ 593​{
508 using namespace kamd::utils; 594​ using namespace kamd::utils;
509 595​
510 // Mapping activity ids to info 596​ // Mapping activity ids to info
511 597​
▲ Show 20 Lines • Show All 79 Lines • ▼ Show 20 Line(s)664{
591 d->ksmserver->stopActivitySession(activity); 677​ d->ksmserver->stopActivitySession(activity);
592} 678​}
593 679​
594int Activities::ActivityState(const QString &activity) const 680​int Activities::ActivityState(const QString &activity) const
595{ 681​{
596 QReadLocker lock(&d->activitiesLock); 682​ QReadLocker lock(&d->activitiesLock);
597 return d->activities.contains(activity) ? d->activities[activity] : Invalid; 683​ return d->activities.contains(activity) ? d->activities[activity] : Invalid;
598} 684​}
599 ​