D22381: Add previous-/nextActivity methods
This revision was automatically updated to reflect the committed changes. muesli marked an inline comment as done. Closed by commit R161:e3a7cb2f1b18: Add previous-/nextActivity methods (authored by muesli). REPOSITORY R161 KActivity Manager Service CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D22381?vs=67276=67278 REVISION DETAIL https://phabricator.kde.org/D22381 AFFECTED FILES src/common/dbus/org.kde.ActivityManager.Activities.xml src/service/Activities.cpp src/service/Activities.h src/service/Activities_p.h To: muesli, ivan Cc: ivan, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D22381: Add previous-/nextActivity methods
muesli updated this revision to Diff 67276. muesli added a comment. Fix typo in 'nameBasedOrdering' REPOSITORY R161 KActivity Manager Service CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D22381?vs=61590=67276 BRANCH prevnext-activity (branched from master) REVISION DETAIL https://phabricator.kde.org/D22381 AFFECTED FILES src/common/dbus/org.kde.ActivityManager.Activities.xml src/service/Activities.cpp src/service/Activities.h src/service/Activities_p.h To: muesli, ivan Cc: ivan, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D22381: Add previous-/nextActivity methods
ivan added inline comments.
INLINE COMMENTS
> Activities.cpp:56
> +inline
> +bool nameBaseOrdering(const ActivityInfo , const ActivityInfo
> )
> +{
Nitpick Base -> Based
> Activities.cpp:251
> +
> +for (int i = 0; i < a.count(); ++i) {
> +if (a[i] == currentActivity) {
Can be done with `std::find_if` as well
> Activities.cpp:333
>
> +for (int i = 0; i < sortedActivities.count(); ++i) {
> +if (sortedActivities[i].id == activity) {
This can be done with `std::find_if` and then remove with iterator.
REPOSITORY
R161 KActivity Manager Service
REVISION DETAIL
https://phabricator.kde.org/D22381
To: muesli, ivan
Cc: ivan, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas,
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed,
jensreuterberg, abetts, sebas, apol, mart
D22381: Add previous-/nextActivity methods
muesli updated this revision to Diff 61590. muesli added a comment. Move nameBaseOrdering to an anonymous namespace and mark it as inline. REPOSITORY R161 KActivity Manager Service CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D22381?vs=61589=61590 BRANCH prevnext-activity (branched from master) REVISION DETAIL https://phabricator.kde.org/D22381 AFFECTED FILES src/common/dbus/org.kde.ActivityManager.Activities.xml src/service/Activities.cpp src/service/Activities.h src/service/Activities_p.h To: muesli, ivan Cc: ivan, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D22381: Add previous-/nextActivity methods
muesli updated this revision to Diff 61589. muesli added a comment. Addressed @ivan's change requests Summary: - Renamed sorting method to nameBaseOrdering - Remove activity from sorted cache without re-sorting the entire list - Switch to prev/next running activity, skipping other states REPOSITORY R161 KActivity Manager Service CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D22381?vs=61588=61589 BRANCH prevnext-activity (branched from master) REVISION DETAIL https://phabricator.kde.org/D22381 AFFECTED FILES src/common/dbus/org.kde.ActivityManager.Activities.xml src/service/Activities.cpp src/service/Activities.h src/service/Activities_p.h To: muesli, ivan Cc: ivan, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D22381: Add previous-/nextActivity methods
muesli updated this revision to Diff 61588. muesli added a comment. Use a QVector instead of a QList to store sorted activities. REPOSITORY R161 KActivity Manager Service CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D22381?vs=61579=61588 BRANCH prevnext-activity (branched from master) REVISION DETAIL https://phabricator.kde.org/D22381 AFFECTED FILES src/common/dbus/org.kde.ActivityManager.Activities.xml src/service/Activities.cpp src/service/Activities.h src/service/Activities_p.h To: muesli, ivan Cc: ivan, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D22381: Add previous-/nextActivity methods
ivan requested changes to this revision. ivan added a comment. This revision now requires changes to proceed. Inserting/Removing/Updating a sorted list does not need to resort every time - removing is easy, adding a new item is std::lower_bound (a binary search), and updating is a combination of the two. INLINE COMMENTS > Activities.cpp:54 > > +static > +bool infoLessThan(const ActivityInfo , const ActivityInfo ) 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. > Activities.cpp:349 > } > +updateSortedActivityList(); > You can just find the activity in the list, and remove it - the order for the rest will not change. > Activities_p.h:76 > QHash activities; > +QList sortedActivities; > QReadWriteLock activitiesLock; `QList` -> `QVector`. `QList` is an evil and slow class :) REPOSITORY R161 KActivity Manager Service REVISION DETAIL https://phabricator.kde.org/D22381 To: muesli, ivan Cc: ivan, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D22381: Add previous-/nextActivity methods
muesli updated this revision to Diff 61579. muesli added a comment. Keep an alphabetically sorted list of activities Summary: Instead of re-sorting the activity list every time previous-/nextActivity gets called, maintain a sorted list. REPOSITORY R161 KActivity Manager Service CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D22381?vs=61517=61579 BRANCH prevnext-activity (branched from master) REVISION DETAIL https://phabricator.kde.org/D22381 AFFECTED FILES src/common/dbus/org.kde.ActivityManager.Activities.xml src/service/Activities.cpp src/service/Activities.h src/service/Activities_p.h To: muesli, ivan Cc: ivan, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D22381: Add previous-/nextActivity methods
ivan added a comment. You are right, they are randomized. And, yes, that is what I meant on IRC - that redundancy due to keeping a sorted list will likely lead to future issues, but I see no other way if we want to be consistent regarding listing and switching. As it is required to have fast access by UUID, the hash needs to stay. So, I'd say we will need the redundancy. Maybe there is some multi-key sorted multi-map in boost - kamd already uses boost::flat_map, so this would be fine. :) REPOSITORY R161 KActivity Manager Service REVISION DETAIL https://phabricator.kde.org/D22381 To: muesli, ivan Cc: ivan, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D22381: Add previous-/nextActivity methods
muesli added inline comments. INLINE COMMENTS > ivan wrote in Activities.cpp:218 > 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. 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? REPOSITORY R161 KActivity Manager Service REVISION DETAIL https://phabricator.kde.org/D22381 To: muesli, ivan Cc: ivan, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D22381: Add previous-/nextActivity methods
ivan requested changes to this revision. ivan added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > Activities.cpp:218 > +auto a = q->ListActivitiesWithInformation(); > +std::sort(a.begin(), a.end(), ); > + 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. REPOSITORY R161 KActivity Manager Service REVISION DETAIL https://phabricator.kde.org/D22381 To: muesli, ivan Cc: ivan, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D22381: Add previous-/nextActivity methods
muesli created this revision. muesli added a reviewer: ivan. Herald added a project: Plasma. Herald added a subscriber: plasma-devel. muesli requested review of this revision. REVISION SUMMARY These two methods can be used to switch to the previous/next activity in alphabetical order. They are exposed via the DBus interface. This functionality can also be accessed by kactivities(-cli) and plasmashell, which currently both implement their own separate versions of it. @Ivan mentioned being a bit wary of keeping a sorted activity list in kactivitymanagerd, so I've opted to only retrieve and sort the list on demand here. REPOSITORY R161 KActivity Manager Service BRANCH prevnext-activity (branched from master) REVISION DETAIL https://phabricator.kde.org/D22381 AFFECTED FILES src/common/dbus/org.kde.ActivityManager.Activities.xml src/service/Activities.cpp src/service/Activities.h src/service/Activities_p.h To: muesli, ivan Cc: ivan, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
