Add a event Spy for GtkFileChooser recent files
ClosedPublic

Authored by meven on Aug 12 2019, 12:17 PM.

Details

Summary

GtkFileChooser/GtkRecentManager follows https://www.freedesktop.org/wiki/Specifications/desktop-bookmark-spec/ specification to store its recently accessed files.
The history is stored in $HOME/.local/share/recently-used.xbel

Adds an event spy for the xml file used by gtk following the example of the KRecentDocument EventSpy.

It allows to populate recently accessed files from apps using Gtk, including inkscape, gimp...

CCBUG: 311218

Test Plan

Compile
killall -9 kactivitymanage
run your localy built version of kactivitymanagerd, for instance:
~/kde/usr/lib/x86_64-linux-gnu/libexec/kactivitymanagerd
Run a gtk app and open a file
The file access can be seen in kickoff/kicker history

Diff Detail

Repository
R161 KActivity Manager Service
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
meven created this revision.Aug 12 2019, 12:17 PM
Restricted Application added a project: Plasma. · View Herald TranscriptAug 12 2019, 12:17 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
meven requested review of this revision.Aug 12 2019, 12:17 PM

Can you check out the KBookmarks framework which has classes for processing xbel files

broulik added inline comments.Aug 12 2019, 12:27 PM
src/service/plugins/gtk-eventspy/GtkEventSpy.cpp
126

It stores the full Exec line of the desktop file, including the %u.
What you probably want do to is use KServiceTypeTrader to find the application with said Exec line.

auto services = KServiceTypeTrader::self()->query(QStringLiteral("Application"), QStringLiteral("exist Exec and ('%1' == Exec)").arg(cmd));

and then operate on the KService desktop file name, e.g. org.gnome.evince instead of the executable.

meven added a comment.Aug 13 2019, 7:17 AM

Can you check out the KBookmarks framework which has classes for processing xbel files

Thanks for pointing it out.
I just did, and It does not seem to cover my use case here :
It is quite heavily oriented towards its main use : application bookmark.
For instance to get the attributes of the bookmarks elements I would need to use the internalElement() method to get a raw QDomElement which kind of defeat the purpose of using a library.
I would also need to parse myself the <application> elements using internalElement as well.
While user-places.xbel references https://www.freedesktop.org/wiki/Specifications/desktop-bookmark-spec/ as its specification, it does not abide by it.

meven updated this revision to Diff 63641.Aug 13 2019, 8:08 AM

Use KServiceTypeTrader to look for apps being used

meven updated this revision to Diff 63647.Aug 13 2019, 9:45 AM
meven marked an inline comment as done.

Remove unused include

meven updated this revision to Diff 63654.Aug 13 2019, 11:54 AM

Readd include QDebug used by qWarning

meven updated this revision to Diff 63669.Aug 13 2019, 3:54 PM

Add a logging category

ivan requested changes to this revision.Aug 13 2019, 4:59 PM

Thanks for geting involved this much!

src/service/plugins/gtk-eventspy/GtkEventSpy.cpp
130

It might be dangerous to assume that the command is wrapped with single quotes

134

const auto query

135

This should also be const, other local variables as well

144

Let's hope commands will never have spaces in them :)

This revision now requires changes to proceed.Aug 13 2019, 4:59 PM
broulik added inline comments.Aug 14 2019, 6:52 AM
src/service/plugins/gtk-eventspy/GtkEventSpy.cpp
136

Isn't that a bit coarse? Doesn't matching the Exec line exactly work?

In D23112#511354, @ivan wrote:

Thanks for geting involved this much!

:D

meven updated this revision to Diff 63713.Aug 14 2019, 10:28 AM

Add const where need be, simplify KTrader query

meven marked 5 inline comments as done.Aug 14 2019, 10:32 AM
meven added inline comments.
src/service/plugins/gtk-eventspy/GtkEventSpy.cpp
144

It is to just extract the executable name, we don't want to have an exploding number of initiatingAgent for every argument and parameter that might come through here.

meven marked 2 inline comments as done.Aug 14 2019, 10:33 AM

Cool stuff. Does it handle duplicates when a file is present in both GTKFileChooserRecent and KActivitiesStats?

src/service/plugins/gtk-eventspy/kactivitymanagerd-plugin-gtk-eventspy.json
42

Not sure you need to add translations of your own name here

47

that uses -> that use

meven updated this revision to Diff 63724.Aug 14 2019, 2:12 PM

grammar

meven marked 2 inline comments as done.Aug 14 2019, 2:13 PM
meven added inline comments.
src/service/plugins/gtk-eventspy/kactivitymanagerd-plugin-gtk-eventspy.json
42

This was done for Ivan so I guess the translation team fills this.

meven marked an inline comment as done.Aug 14 2019, 2:15 PM

Cool stuff. Does it handle duplicates when a file is present in both GTKFileChooserRecent and KActivitiesStats?

Yes, the file url is a sort of a key, so this is fine.

meven marked an inline comment as done.Aug 14 2019, 2:17 PM
ivan added inline comments.Aug 15 2019, 8:13 AM
src/service/plugins/gtk-eventspy/GtkEventSpy.cpp
69

No need for this to be a pointer to a list. Make it just QList<Application> applications.

71

The constructor will not be needed once applications stops being a pointer.

77

{ which starts a function should be on a new line (I don't care much about this, but let's follow the KF5 style)

78–79

When you make applications not to be a pointer qAsConst will not be needed as this is a const member function.

90

Replace with:

BookmarkHandler()
    : current(nullptr)
{
}
92

No need for this - marks are already default-constructed.

114

No need for dynamic allocation. Make it a normal variable instead of a pointer.

144

I meant it will be a problem if someone decides to have a space in the executable like my\ aweomse\ binary. But this should not be an issue at the moment.

186

No need for dynamic allocation. Make it a normal variable instead of a pointer.

meven updated this revision to Diff 63786.Aug 15 2019, 9:12 AM
meven marked 9 inline comments as done.

Remove unnecessary pointer use, code formatting

meven updated this revision to Diff 63830.Aug 15 2019, 5:56 PM

run uncrustify-kf5

ivan requested changes to this revision.Aug 16 2019, 9:45 PM

Another tiny change, and I think it is ready to land. Unless someone else sees other issues.

src/service/plugins/gtk-eventspy/GtkEventSpy.cpp
127

No need to compare chars with strings:

if (!exec.isEmpty() && exec[0] == '\'' && exec.back() == '\'')
This revision now requires changes to proceed.Aug 16 2019, 9:45 PM
bruns added a subscriber: bruns.Aug 17 2019, 1:22 AM
bruns added inline comments.
src/service/plugins/gtk-eventspy/GtkEventSpy.cpp
127

missing QChar('\''), otherwise it breaks with QT_NO_CAST_FROM_ASCII

-> if (exec.startsWith(QChar('\'') && exec.endsWith(QChar('\'')) {...

145

spaceI_n_dex

meven updated this revision to Diff 63890.Aug 17 2019, 6:28 AM

Ensure the log category is exported, compare only chars, variable naming fix

meven marked 3 inline comments as done.Aug 17 2019, 6:31 AM
ivan accepted this revision.Aug 17 2019, 10:26 AM
This revision is now accepted and ready to land.Aug 17 2019, 10:26 AM
meven updated this revision to Diff 63913.Aug 17 2019, 11:18 AM

Reduce the amount of included code and linked libraries

This revision was automatically updated to reflect the committed changes.
GB_2 awarded a token.Aug 17 2019, 11:41 AM
meven edited the summary of this revision. (Show Details)Aug 17 2019, 5:01 PM
meven edited the summary of this revision. (Show Details)