Review Request: Add recentdocuments:/ kio slave to kde-runtime.

[Xuetian Weng]

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103849/

Review request for KDE Runtime. By Xuetian Weng. Description Add recentdocuments:/ kio slave to kde-runtime. Did some little rename "recentdocument -> recentdocuments", based on http://kde-apps.org/content/show.php?content=145878 Testing Works for me. Diffs kioslave/CMakeLists.txt (f3d5b00) kioslave/recentdocuments/CMakeLists.txt (PRE-CREATION) kioslave/recentdocuments/recentdocuments.h (PRE-CREATION) kioslave/recentdocuments/recentdocuments.cpp (PRE-CREATION) kioslave/recentdocuments/recentdocuments.protocol (PRE-CREATION) kioslave/recentdocuments/recentdocumentsnotifier.h (PRE-CREATION) kioslave/recentdocuments/recentdocumentsnotifier.cpp (PRE-CREATION) kioslave/recentdocuments/recentdocumentsnotifier.desktop (PRE-CREATION) View Diff

[Albert Astals Cid]

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103849/

You need a Messages.sh to extract your i18n calls so that they can be translated

kioslave/recentdocuments/recentdocuments.cpp (Diff revision 1)
		extern "C" int KDE_EXPORT kdemain(int argc, char **argv)
		22	KComponentData("kio_recentdocuments", "kdelibs4");

That kdelibs4 does not make sense, you need your own translation catalog, loading the kdelibs4 catalog only won't help

kioslave/recentdocuments/recentdocuments.protocol (Diff revision 1)
		13	maxInstances=4

Why 4 maxInstances?

- Albert

On February 2nd, 2012, 3:23 p.m., Xuetian Weng wrote:

Review request for KDE Runtime. By Xuetian Weng.

Updated Feb. 2, 2012, 3:23 p.m.

[Xuetian Weng]

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103849/

On February 2nd, 2012, 11:27 p.m., Albert Astals Cid wrote:

kioslave/recentdocuments/recentdocuments.cpp (Diff revision 1)
		extern "C" int KDE_EXPORT kdemain(int argc, char **argv)
		22	KComponentData("kio_recentdocuments", "kdelibs4");

That kdelibs4 does not make sense, you need your own translation catalog, loading the kdelibs4 catalog only won't help

Ok

On February 2nd, 2012, 11:27 p.m., Albert Astals Cid wrote:

kioslave/recentdocuments/recentdocuments.protocol (Diff revision 1)
		13	maxInstances=4

Why 4 maxInstances?

Not sure for this part... maybe only one?

- Xuetian

[Xuetian Weng]

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103849/

Review request for KDE Runtime. By Xuetian Weng.

Updated Feb. 5, 2012, 5:28 a.m. Changes remove space, modified by request.

Description Add recentdocuments:/ kio slave to kde-runtime. Did some little rename "recentdocument -> recentdocuments", based on http://kde-apps.org/content/show.php?content=145878 Testing Works for me.

Diffs (updated) kioslave/CMakeLists.txt (f3d5b00) kioslave/recentdocuments/CMakeLists.txt (PRE-CREATION) kioslave/recentdocuments/Messages.sh (PRE-CREATION)

kioslave/recentdocuments/recentdocuments.h (PRE-CREATION) kioslave/recentdocuments/recentdocuments.cpp (PRE-CREATION) kioslave/recentdocuments/recentdocuments.protocol (PRE-CREATION) kioslave/recentdocuments/recentdocumentsnotifier.h (PRE-CREATION) kioslave/recentdocuments/recentdocumentsnotifier.cpp (PRE-CREATION) kioslave/recentdocuments/recentdocumentsnotifier.desktop (PRE-CREATION)

View Diff

[Albert Astals Cid]

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103849/

kioslave/recentdocuments/recentdocumentsnotifier.cpp (Diff revision 2)
		void RecentDocumentsNotifier::dirty(const QString &path)
		31	Q_UNUSED(path)

it's not unused ;-)

- Albert

On February 5th, 2012, 5:28 a.m., Xuetian Weng wrote:

Review request for KDE Runtime. By Xuetian Weng. Updated Feb. 5, 2012, 5:28 a.m.

Description

Add recentdocuments:/ kio slave to kde-runtime. Did some little rename "recentdocument -> recentdocuments", based on http://kde-apps.org/content/show.php?content=145878 Testing Works for me. Diffs

[Xuetian Weng]

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103849/

Review request for KDE Runtime. By Xuetian Weng.

Updated Feb. 5, 2012, 7:24 p.m. Changes update

[David Faure]

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103849/

kioslave/recentdocuments/recentdocuments.cpp (Diff revision 3)
		void RecentDocuments::listDir(const KUrl& url)
		84	KUrl url;

This local variable hides the "url" argument to the method, making this code confusing to read.
Also, it's not needed here outside of the if, so I would recommend changing line 93 to

KUrl desktopFileUrll(entry);

But even then, it's only used at line 100, right? So this should really become

else
   urlToStat = KUrl(entry);

Not that I see much point in stat'ing the desktop file anyway... You'll replace the name and display name and url ... so basically a remote URL will appear to be a file (could be a bug, if it was a directory) of size "a few bytes" (the size of the .desktop file) ...
I guess you didn't want to stat remote files because this might ask for passwords, or trigger connecting to the internet at an unwanted time, or might be too slow?
I guess all of this is valid, and in that case the proper long-term solution would be to write out more information about recent urls in the file...
OK. But my point is, the size of the .desktop file is wrong, as "size of the remote file", so better not set any size (== unknown).
=> better not stat the desktop file, ever.

Just have two code paths:

  if urlInside.isLocalFile {
     stat(urlInside)
     and fix name, displayname, set localpath.
  } else {
     fill in a UDS entry from scratch with the little we know about the remote url
   }
    set UDS_TARGET_URL
    udslist << uds;

kioslave/recentdocuments/recentdocuments.cpp (Diff revision 3)
		void RecentDocuments::listDir(const KUrl& url)
		95	KUrl urlToState;

State? I guess you meant Stat.

kioslave/recentdocuments/recentdocuments.cpp (Diff revision 3)
		void RecentDocuments::listDir(const KUrl& url)
		102	if (!urlToState.isEmpty()) {

How about testing urlInside.isEmpty() together with the "protocol==recentdocuments" a few lines up, to use "continue" for both?

All this unnecessary nesting of if()s hurts readability a bit, creating multiple code paths.

kioslave/recentdocuments/recentdocuments.cpp (Diff revision 3)
		void RecentDocuments::listDir(const KUrl& url)
		103	if (KIO::StatJob* job = KIO::stat(urlToState, KIO::HideProgressInfo)) {

This if() is unnecessary, KIO::stat will never return a NULL job.

kioslave/recentdocuments/recentdocuments.cpp (Diff revision 3)
		void RecentDocuments::listDir(const KUrl& url)
		114	if (uds.count() && !urlSet.contains(prettyUrl)) {

if (uds.count()) is usually written as if (!uds.isEmpty()) in Qt code, more readable.

But it can only be empty if the job failed, so maybe it would be simpler to "continue" after failure, and thus remove if()s.

kioslave/recentdocuments/recentdocuments.cpp (Diff revision 3)
		void RecentDocuments::listDir(const KUrl& url)
		167	uds.insert(KIO::UDSEntry::UDS_NAME, i18n("Recent Document"));

This should be i18n("Recent Documents")  ('s' missing).
Use a local QString variable to avoid duplicating the string in the code.

kioslave/recentdocuments/recentdocumentsnotifier.cpp (Diff revision 3)
		RecentDocumentsNotifier::RecentDocumentsNotifier(QObject *parent, const QList<QVariant> &)
		26	connect(dirWatch, SIGNAL(dirty(QString)), SLOT(dirty(QString)));

Double connect, this was already done on line 24.

kioslave/recentdocuments/recentdocumentsnotifier.cpp (Diff revision 3)
		RecentDocumentsNotifier::RecentDocumentsNotifier(QObject *parent, const QList<QVariant> &)
		36	url.cleanPath();

Not sure what there is to clean in "recentdocuments:/filename" :-)

I think this line can be removed.

- David

On February 5th, 2012, 7:24 p.m., Xuetian Weng wrote:

Review request for KDE Runtime. By Xuetian Weng. Updated Feb. 5, 2012, 7:24 p.m.

Description

Add recentdocuments:/ kio slave to kde-runtime. Did some little rename "recentdocument -> recentdocuments", based on http://kde-apps.org/content/show.php?content=145878 Testing Works for me. Diffs

[David Faure]

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103849/

kioslave/recentdocuments/recentdocuments.cpp (Diff revision 3)
		bool RecentDocuments::rewriteUrl(const KUrl& url, KUrl& newUrl)
		58	KUrl desktopFileUrl;

Why use KUrl here? This is always a local path, and you turn it into a local path in order to use it.
It would be much simpler to just use a QString here, concatenate directory and filename, then use KDesktopFile with that.

- David

On February 5th, 2012, 7:24 p.m., Xuetian Weng wrote:

Review request for KDE Runtime. By Xuetian Weng. Updated Feb. 5, 2012, 7:24 p.m.

Description

Add recentdocuments:/ kio slave to kde-runtime. Did some little rename "recentdocument -> recentdocuments", based on http://kde-apps.org/content/show.php?content=145878 Testing Works for me. Diffs

[Xuetian Weng]

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103849/

Review request for KDE Runtime. By Xuetian Weng.

Updated Feb. 15, 2012, 3:11 p.m. Changes Update as required. Clean up unnecessary logic.

[Xuetian Weng]

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103849/

On February 13th, 2012, 11:26 a.m., David Faure wrote:

kioslave/recentdocuments/recentdocuments.cpp (Diff revision 3)
		bool RecentDocuments::rewriteUrl(const KUrl& url, KUrl& newUrl)
		58	KUrl desktopFileUrl;

Why use KUrl here? This is always a local path, and you turn it into a local path in order to use it.
It would be much simpler to just use a QString here, concatenate directory and filename, then use KDesktopFile with that.

Well.. any further comment on this review? :)

- Xuetian

On February 15th, 2012, 3:11 p.m., Xuetian Weng wrote:

Review request for KDE Runtime. By Xuetian Weng.

Updated Feb. 15, 2012, 3:11 p.m.

[David Faure]

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103849/

Ship it!

Looks good, please commit after adjusting the last minor issues I found below.

kioslave/recentdocuments/recentdocuments.cpp (Diff revision 4)
		void RecentDocuments::prepareUDSEntry(KIO::UDSEntry& entry, bool listing) const
		125	kDebug() << "prepareUDSEntry";

The method name is already printed by kDebug().

kioslave/recentdocuments/recentdocuments.cpp (Diff revision 4)
		void RecentDocuments::prepareUDSEntry(KIO::UDSEntry& entry, bool listing) const
		138	if (KDesktopFile::isDesktopFile(url.path()))

This should be url.toLocalFile() rather than url.path() [only makes a difference on Windows]

kioslave/recentdocuments/recentdocuments.cpp (Diff revision 4)
		void RecentDocuments::prepareUDSEntry(KIO::UDSEntry& entry, bool listing) const
		139	return url.path();

Same here.

- David

On February 15th, 2012, 3:11 p.m., Xuetian Weng wrote:

Review request for KDE Runtime. By Xuetian Weng. Updated Feb. 15, 2012, 3:11 p.m.

Description

Add recentdocuments:/ kio slave to kde-runtime. Did some little rename "recentdocument -> recentdocuments", based on http://kde-apps.org/content/show.php?content=145878 Testing Works for me. Diffs

[Commit Hook]

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103849/

This review has been submitted with commit feeadd5c4fed937864944e9d375b3441239c4e12 by Weng Xuetian to branch master.

- Commit

On February 15th, 2012, 3:11 p.m., Xuetian Weng wrote:

Review request for KDE Runtime. By Xuetian Weng. Updated Feb. 15, 2012, 3:11 p.m.

Description

Add recentdocuments:/ kio slave to kde-runtime. Did some little rename "recentdocument -> recentdocuments", based on http://kde-apps.org/content/show.php?content=145878 Testing Works for me. Diffs