FEATURE: 395954
Details
- Reviewers
cullmann dhaumann - Group Reviewers
Kate KTextEditor - Commits
- R40:cdb3da47ccda: Support activities when opening files
I tested this on my environment (Plasma 15.13 on X11), but it should be tested also on Wayland (without activities) and on other DEs.
Diff Detail
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Thanks for the patch! Can you change Fixes #395954 to FEATURE: 395954? See https://community.kde.org/Infrastructure/Phabricator#Formatting_your_patch
Looks already good to me, but one more code iteration would be nice.
@cullmann Any comments from your side?
kate/kateapp.cpp | ||
---|---|---|
278 | Simply writing the following is enough: QStringList allActivities; | |
279 | Can we use range-based for loops here? Same below? | |
kate/kateapp.h | ||
194 | Could you add API documentation here as well? |
Think that is ok, if the given comments are implemented.
An alternative approach for the API would btw. to have just a function that tells you "I am on activity X", then one doesn't need to send stringlists around and check for contains.
Thanks for the comment. The problem is that it is possible that the window is on more than a single activity (e.g., there are three activities, A, B and C, and a single Kate instance is in both A and C at the same time), that's why I used a StringList. Indeed, I implemented that as you suggested at the beginning, but then I noticed this problem. Also, the check is done only when the user opens a new file, and the list of activities should not be that long. In general, I think that the overhead is acceptable, and it allows us to have a complete support for activities.
Anyways, I will implement all your comments ASAP and I will update the patch.
Thanks for the review.
Hmm, that I don't understand ;=)
You just do:
if (answer.size() == 1) { kateCurrentActivities = answer.at(0).toStringList(); for (int j = 0; j < kateCurrentActivities.size(); j++) { if (kateCurrentActivities.at(j) == currentActivity) { anyOnThisActivity = true; } } }
> That means if you would have a dbus function that tells you "haveWindowForActivity(currentActivity)" you would be fine, or?
OK, I got what you mean. I implemented your suggestion and the other suggested fixes. I also changed the logic in main.cpp, because I found a bug in the previous version. It should be also more understandable.
Someone will need to land this for Simone (whose email address is s.scalabrino9@gmail.com).