Differential D22144 Diff 60789 recentlyused/recentlyused.cpp

Changeset View

Standalone View

View Options

recentlyused/recentlyused.cpp

  • This file was added.
1​/*
2​ * Copyright (C) 2019 Méven Car <meven.car@kdemail.net>
3​ *
4​ * This library is free software; you can redistribute it and/or
5​ * modify it under the terms of the GNU Lesser General Public
6​ * License as published by the Free Software Foundation; either
7​ * version 2.1 of the License, or (at your option) version 3, or any
8​ * later version accepted by the membership of KDE e.V. (or its
9​ * successor approved by the membership of KDE e.V.), which shall
10​ * act as a proxy defined in Section 6 of version 3 of the license.
11​ *
12​ * This library is distributed in the hope that it will be useful,
13​ * but WITHOUT ANY WARRANTY; without even the implied warranty of
14​ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
15​ * Lesser General Public License for more details.
16​ *
17​ * You should have received a copy of the GNU Lesser General Public
18​ * License along with this library.
19​ * If not, see <http://www.gnu.org/licenses/>.
20​ */
21​
22​#include "recentlyused-logsettings.h"
23​#include "recentlyused.h"
24​
25​#include <QCoreApplication>
26​#include <QDBusInterface>
27​#include <QFileInfo>
28​#include <QDebug>
29​#include <QUrl>
ngrahamUnsubmitted
Done

Remove

ngraham: Remove
30​#include <QUrlQuery>
31​#include <QString>
32​
33​#include <KRecentDocument>
34​#include <KDirWatch>
35​#include <KDesktopFile>
36​#include <KIO/Job>
37​#include <KLocalizedString>
38​#include <KProtocolManager>
39​
40​
41​#include <stdio.h>
42​
43​#include <KActivities/Stats/Cleaning>
44​#include <KActivities/Stats/ResultModel>
45​#include <KActivities/Stats/Terms>
46​
47​namespace KAStats = KActivities::Stats;
48​
49​using namespace KAStats;
50​using namespace KAStats::Terms;
51​
52​extern "C" int Q_DECL_EXPORT kdemain(int argc, char **argv)
53​{
54​ // necessary to use other kio slaves
55​ QCoreApplication app(argc, argv);
56​ app.setApplicationName(QStringLiteral("kio_recentlyused"));
57​ if (argc != 4) {
58​ fprintf(stderr, "Usage: kio_recentlyused protocol domain-socket1 domain-socket2\n");
59​ exit(-1);
60​ }
61​ // start the slave
62​ RecentlyUsed slave(argv[2], argv[3]);
63​ slave.dispatchLoop();
64​ return 0;
65​}
66​
67​bool isRootUrl(const QUrl &url)
68​{
69​ const QString path = url.adjusted(QUrl::StripTrailingSlash).path();
70​ return (path.isEmpty() || path == QLatin1String("/"));
71​}
72​
73​
dfaureUnsubmitted
Done

prepend "static"

dfaure: prepend "static"
74​RecentlyUsed::RecentlyUsed(const QByteArray& pool, const QByteArray& app):
75​ ForwardingSlaveBase("recentlyused", pool, app)
76​{}
77​
78​RecentlyUsed::~RecentlyUsed()
79​{}
80​
81​
82​bool RecentlyUsed::rewriteUrl(const QUrl& url, QUrl& newUrl)
83​{
84​ Q_UNUSED(url);
85​ Q_UNUSED(newUrl);
86​ return false;
87​}
88​
89​ResultModel* runQuery(const QUrl& url) {
90​ qCDebug(KIO_RECENTLYUSED_LOG) << "runQuery for url" << url.toString();
91​
92​ auto query = UsedResources
dfaureUnsubmitted
Done

I may be missing something, but if you don't really reimplement rewriteUrl, then what's the point of using ForwardingSlaveBase in the first place, instead of just SlaveBase?

IIRC *all* of ForwardingSlaveBase's code is based on the rewriteUrl idea.

dfaure: I may be missing something, but if you don't really reimplement rewriteUrl, then what's the…
mevenAuthorUnsubmitted
Done

I started my work upon the recentdocument ioslave that used it and ForwardingSlaveBase documentation states "It has been designed to support only local filesystem like ioslaves.. If the resulting ioslave should be a simple proxy, you only need to implement the ForwardingSlaveBase::rewriteUrl() method." makes it sound like that is what I needed.
Using SlaveBase would need a lot more code added also that I don't particularly need to implement here.
But please correct me.

meven: I started my work upon the recentdocument ioslave that used it and ForwardingSlaveBase…
dfaureUnsubmitted
Done

I'm confused. Which code from ForwardingSlaveBase is this class benefiting from?
With internalRewriteUrl returning false all SlaveBase reimplementations in that class do nothing,

If you decide not to support listing/stat'ing subdirs then indeed you don't need ForwardingSlaveBase as base class.

dfaure: I'm confused. Which code from ForwardingSlaveBase is this class benefiting from? With…
mevenAuthorUnsubmitted
Done

After more testing, inheriting from ForwardSlaveBase was not needed.

meven: After more testing, inheriting from ForwardSlaveBase was not needed.
93​ | Limit(30);
94​
95​ // Parse url query parameter
96​ auto urlQuery = QUrlQuery(url);
97​
98​ // handles type aka mimetype
99​ if (urlQuery.hasQueryItem(QStringLiteral("type"))) {
100​ auto typeValue = urlQuery.queryItemValue(QStringLiteral("type"));
101​ if (typeValue.contains(QLatin1Char(','))) {
102​ // multiple mimetypes were passed
103​ query = query | Type(typeValue.split(QLatin1Char(',')));
104​ } else {
105​ query = query | Type(typeValue);
106​ }
107​ }
108​
109​ // limit parameter
110​ if (urlQuery.hasQueryItem(QStringLiteral("limit"))) {
dfaureUnsubmitted
Done

(hmm, someone should implement operator |= in kactivities-stat, to make such code simpler and possibly faster)

dfaure: (hmm, someone should implement operator |= in kactivities-stat, to make such code simpler and…
mevenAuthorUnsubmitted
Done

Tried implementing this in D23372 but @ivan was not found of it because of induced complexity it entails to work properly.

meven: Tried implementing this in D23372 but @ivan was not found of it because of induced complexity…
ivanUnsubmitted
Done

Also, there are set* and add* member functions which can be used for non-chained changes.

ivan: Also, there are `set*` and `add*` member functions which can be used for non-chained changes.
mevenAuthorUnsubmitted
Done

We don't have yet set function for Activity, Type, Agent, and Url only add.
Which means we would need to use query.addActivity(QStringList() << ":any")) sometimes for instance, that is not ideal since it makes the user class use internal details such as ":any".
I would be in favor to add setActivity(Terms::Activity) in KactivitiesStats same for Type, Agent and Url, to get arount that and maybe also addActivity(Terms::Activity) as well for consistency.

meven: We don't have yet set function for Activity, Type, Agent, and Url only add. Which means we…
ivanUnsubmitted
Done

You have Activity::any() - no need for the client to use the special values. You can do a addActivities({Activity::any()}).

I would not mind to add set* variants of add* functions - setActivities, setTypes, etc. to avoid the need of calling clear* and then add*.

I'm also thinking whether it would be useful to have a variadic template versions of these so that you can skip the QList<T> construction. That might be better to wait for Qt 6 and raised C++ version requirement. Not sure.

ivan: You have `Activity::any()` - no need for the client to use the special values. You can do a…
111​ auto limitValue = urlQuery.queryItemValue(QStringLiteral("limit"));
112​ bool parseOk;
113​ auto limitInt = limitValue.toInt(&parseOk);
114​ if (parseOk) {
115​ query = query | Limit(limitInt);
116​ }
117​ }
118​
119​ // acitivity parameter, filter using the uuid of the activity
120​ if (urlQuery.hasQueryItem(QStringLiteral("activity"))) {
121​ auto activityValue = urlQuery.queryItemValue(QStringLiteral("activity"));
122​ if (activityValue == QStringLiteral("any")) {
123​ query = query | Activity::any();
124​ } else {
125​ query = query | Activity(activityValue);
126​ }
127​ } else {
128​ query = query | Activity::current();
129​ }
130​
131​ // agent parameter, filter using the application name that used the resource
132​ if (urlQuery.hasQueryItem(QStringLiteral("agent"))) {
133​ auto agentValue = urlQuery.queryItemValue(QStringLiteral("agent"));
134​ if (agentValue == QStringLiteral("current")) {
135​ query = query | Agent::current();
136​ }else if (agentValue.contains(QLatin1Char(','))) {
137​ query = query | Agent(agentValue.split(QLatin1Char(',')));
138​ } else {
139​ query = query | Agent(agentValue);
140​ }
141​ } else {
142​ query = query | Agent::any();
143​ }
144​
ngrahamUnsubmitted
Done

Missing a space before else if

ngraham: Missing a space before `else if`
145​ // url parameter for exact path match or folders, supports wildcard pattern matching
ivanUnsubmitted
Done

Could this just be replaced with query = query | Agent(agentValue.split(QLatin1Char(',')));?

Obviously, the downside is always creating a list (usually with a single item in it), the upside is that it does not need to traverse the string twice (once to chech for commas, and the second time to split the string.

The trade-off depends on internals of Qt - if you don't have the time to benchmark what is faster, leave it as is.

ivan: Could this just be replaced with `query = query | Agent(agentValue.split(QLatin1Char(',')));`?
mevenAuthorUnsubmitted
Done

I have used count() as it allows not to keep the list in memory if not needed.
Same was done for the type parameter.

meven: I have used count() as it allows not to keep the list in memory if not needed. Same was done…
146​ if (urlQuery.hasQueryItem(QStringLiteral("url"))) {
147​ query = query | Url(urlQuery.queryItemValue(QStringLiteral("url")));
148​ } else {
149​ query = query | Url::file();
150​ }
151​
152​ // see KActivities::Stats::Terms::Order
153​ if (urlQuery.hasQueryItem(QStringLiteral("order"))) {
154​ auto orderValue = urlQuery.queryItemValue(QStringLiteral("order"));
155​
156​ if (orderValue == QStringLiteral("HighScoredFirst")) {
157​ query = query | Order::HighScoredFirst;
158​ } else if (orderValue == QStringLiteral("RecentlyCreatedFirst")) {
159​ query = query | Order::RecentlyCreatedFirst;
160​ } else if (orderValue == QStringLiteral("OrderByUrl")) {
161​ query = query | Order::OrderByUrl;
162​ } else if (orderValue == QStringLiteral("OrderByTitle")) {
163​ query = query | Order::OrderByTitle;
164​ } else {
165​ query = query | Order::RecentlyUsedFirst;
166​ }
167​ } else {
168​ query = query | Order::RecentlyUsedFirst;
169​ }
170​
171​ return new ResultModel(query);
172​}
173​
174​KIO::UDSEntry RecentlyUsed::udsEntryFromResource(const QString& resource){
175​
176​ // the query only returns files and folders
177​ QUrl resourceUrl = QUrl::fromLocalFile(resource);
178​
179​ KIO::UDSEntry uds;
180​ KIO::StatJob* job = KIO::stat(resourceUrl, KIO::HideProgressInfo);
181​
182​ // we do not want to wait for the event loop to delete the job
183​ QScopedPointer<KIO::StatJob> sp(job);
184​ job->setAutoDelete(false);
185​ if (job->exec()) {
186​ uds = job->statResult();
187​ }
dfaureUnsubmitted
Done

You don't need spaces between items, when using q[C]Debug

dfaure: You don't need spaces between items, when using q[C]Debug
188​ uds.fastInsert(KIO::UDSEntry::UDS_URL, resourceUrl.toString());
189​ return uds;
190​}
191​
192​void RecentlyUsed::listDir(const QUrl& url)
193​{
194​ if (!isRootUrl(url)) {
195​ error(KIO::ERR_DOES_NOT_EXIST, url.toDisplayString());
196​ return;
197​ }
198​
199​ auto model = runQuery(url);
200​
201​ KIO::UDSEntryList udslist;
202​ udslist.reserve(model->rowCount());
203​
204​ for(int r = 0; r < model->rowCount(); ++r) {
205​ QModelIndex index = model->index(r, 0);
206​ QString resource = model->data(index, ResultModel::ResourceRole).toString();
207​
208​ udslist << udsEntryFromResource(resource);
209​ }
210​
211​ listEntries(udslist);
212​ finished();
ngrahamUnsubmitted
Done

space after for

ngraham: space after `for`
213​}
214​
215​void RecentlyUsed::stat(const QUrl& url)
216​{
217​ qCDebug(KIO_RECENTLYUSED_LOG) << "stating" << " " << url;
218​
219​ if (isRootUrl(url)) {
220​ //
221​ // stat the root path
222​ //
223​
224​ QString dirName = i18n("Recent Documents");
225​ KIO::UDSEntry uds;
226​ uds.reserve(5);
227​ uds.fastInsert(KIO::UDSEntry::UDS_NAME, dirName);
228​ uds.fastInsert(KIO::UDSEntry::UDS_DISPLAY_NAME, dirName);
229​ uds.fastInsert(KIO::UDSEntry::UDS_DISPLAY_TYPE, dirName);
230​ uds.fastInsert(KIO::UDSEntry::UDS_ICON_NAME, QStringLiteral("document-open-recent"));
231​ uds.fastInsert(KIO::UDSEntry::UDS_FILE_TYPE, S_IFDIR);
232​ uds.fastInsert(KIO::UDSEntry::UDS_MIME_TYPE, QStringLiteral("inode/directory"));
233​
234​ statEntry(uds);
235​ finished();
236​ } else {
237​ auto model = runQuery(url);
238​ if (model->rowCount() != 1) {
239​ error(KIO::ERR_DOES_NOT_EXIST, url.toDisplayString());
240​ return ;
241​ }
242​
243​ QModelIndex index = model->index(0, 0);
244​ QString resource = model->data(index, ResultModel::ResourceRole).toString();
245​ statEntry(udsEntryFromResource(resource));
246​
dfaureUnsubmitted
Done

I count 6...

dfaure: I count 6...
247​ finished();
248​ }
249​}
250​
251​void RecentlyUsed::mimetype(const QUrl& url)
ivanUnsubmitted
Done

QStringLiteral?

ivan: `QStringLiteral`?
252​{
253​ // the root url is always a folder
254​ if (isRootUrl(url)) {
255​ mimeType(QString::fromLatin1("inode/directory"));
256​ finished();
257​ }
dfaureUnsubmitted
Done

Isn't that a problem? Any user of this ioslave might stat() a URL representing a file or directory, coming from this kioslave.

E.g. if you paste a URL of a subdir from one dolphin window to another, I suspect it will happen then. At least in konqueror it does, maybe dolphin assumes everything is a directory since it can't do much with a file URL....

dfaure: Isn't that a problem? Any user of this ioslave might stat() a URL representing a file or…
mevenAuthorUnsubmitted
Done

I implemented the kioslave to allow only stating and listing "/" path on purpose : I could not find a use case for subdirs.
I chose url parameters to pass pararmeters for simplicity albeit it makes it less appealing in UI as you cannot have subpath breaking down your parameters.
Any files or folders appearing in the ioslave keep their true urls.

meven: I implemented the kioslave to allow only stating and listing "/" path on purpose : I could not…
258​ // others urls are forwarded
259​ else {
260​ ForwardingSlaveBase::mimetype(url);
261​ }
262​}