Add Sessions as jump list actions
ClosedPublic

Authored by broulik on Aug 8 2017, 10:50 AM.

Details

Summary

This adds Kate sessions as jump list actions that allow to launch Kate with a specific session from application launchers and task manager.

CCBUG: 383249

Test Plan
  • Verified that org.kde.kate.desktop is only copied when there are sessions
  • Verified that org.kde.kate.desktop is only modified if sessions changed
  • Verified that launching a specific session works
  • Verified that adding, renaming, removing sessions updates the jump list actions
  • Verified that custom jump list actions that might have been added to the desktop file by the user are preserved
  • Verified that special chars in session name don't break things (I chose to md5 them for the action group to avoid random special characters in there ven though KConfig seems to handle this gracefully)

This also conflicts with Kate Sessions runner, so you'll end up with duplicated "Application: Foo - Kate" and "Kate Sessions: Foo" results


Eike came up with a patch for Task Manager to make this work \o/ D7561 and D7562

Diff Detail

Repository
R40 Kate
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
broulik created this revision.Aug 8 2017, 10:50 AM
Restricted Application added a subscriber: kwrite-devel. · View Herald TranscriptAug 8 2017, 10:51 AM
dhaumann requested changes to this revision.Aug 8 2017, 6:41 PM
dhaumann added a subscriber: dhaumann.

Looks almost good, I would like to see one more revision with the raised issues :-)

kate/session/katesessionmanager.cpp
47

Really needed, see below?

95

Add a comment maybe ? E.g.:
// write jump list actions to disk in the kate.desktop file

522

Can't we put KDesktopFile on the stack? I don't see a reason for a pointer behind a QScopedPointer, or do I miss something?

539

I don't think this assert() is required, since this is how std::transform() works at its core.

548

Since locateLocal() returns a QString, please turn the const QString& into a simple QString.

561

I would prefert:

const int maxEntryCount = std::min(sessionActions.count(), 10);
for (int i = 0; i < maxEntryCount; ++i) {

This revision now requires changes to proceed.Aug 8 2017, 6:41 PM
broulik added inline comments.Aug 11 2017, 9:42 AM
kate/session/katesessionmanager.cpp
522

KDesktopFile::copyTo returns a new KDesktopFile which I need to dispose of and to avoid having two codepaths for "using the original desktop file" and the "copy" I went this route. If you have any better suggestions, I'm open to it.

hein edited edge metadata.Aug 12 2017, 12:55 AM

Looks good to me from the Task Manager end. The absolute path thing is true -- I've been pondering that for a while. There's arguments pro and con, this would be another con one.

@broulik If you address the other minor issues, you are good to go, so can you provide an updated patch?

Imho this is somewhat blocked on TM storing absolute paths (or rather it breaking/not updating Kate launchers when I mess with them in the way the patch does). Not sore how to proceed here. @hein Let's discuss this a bit on IRC tomorrow, shall we?

hein added a comment.Aug 23 2017, 11:42 AM

@broulik Yeah let's talk this week

hein added a comment.Aug 26 2017, 5:47 PM

See D7561 and D7562 for libtm and TM applet changes that will make this fly.

broulik updated this revision to Diff 18872.Aug 28 2017, 8:39 AM
broulik edited edge metadata.
broulik retitled this revision from RFC: Add Sessions as jump list actions to Add Sessions as jump list actions.
broulik edited the test plan for this revision. (Show Details)
dhaumann accepted this revision.Sep 18 2017, 3:43 PM

Looks good to me. Thanks for working on this! :)

kate/session/katesessionmanager.cpp
567

What is the effect of this TODO?

This revision is now accepted and ready to land.Sep 18 2017, 3:43 PM
This revision was automatically updated to reflect the committed changes.