Differential D7700 Diff 19401 src/panels/places/placesitemmodel.cpp

Changeset View

Standalone View

View Options

src/panels/places/placesitemmodel.cpp

Context not available.
39#include <QMimeData> 39​#include <QMimeData>
40#include <QTimer> 40​#include <QTimer>
41#include <KUrlMimeData> 41​#include <KUrlMimeData>
42​#include <KIO/ListJob>
43​#include <KIO/Job>
42 44​
43#include <Solid/Device> 45​#include <Solid/Device>
44#include <Solid/DeviceNotifier> 46​#include <Solid/DeviceNotifier>
Context not available.
78 m_bookmarkedItems(), 80​ m_bookmarkedItems(),
79 m_hiddenItemToRemove(-1), 81​ m_hiddenItemToRemove(-1),
80 m_updateBookmarksTimer(0), 82​ m_updateBookmarksTimer(0),
81 m_storageSetupInProgress() 83​ m_storageSetupInProgress(),
84​ m_tags()
alexeyminUnsubmitted
Done

warning: ‘PlacesItemModel::m_storageSetupInProgress’ will be initialized after [-Wreorder]
warning: ‘QList<QString> PlacesItemModel::m_tags’ [-Wreorder]
swap m_tags() with m_storageSetupInProgress() ?

alexeymin: warning: ‘PlacesItemModel::m_storageSetupInProgress’ will be initialized after [-Wreorder]…
anthonyfieroniUnsubmitted
Done

Remove it, it's pointer type

anthonyfieroni: Remove it, it's pointer type
82{ 85​{
83#ifdef HAVE_BALOO 86​#ifdef HAVE_BALOO
84 Baloo::IndexerConfig config; 87​ Baloo::IndexerConfig config;
markgUnsubmitted
Not Done

Set a parent (you can use "this" i think), then you don't have to delete it later on (the parent -> child structure will take care of it).
+ move it up to the constructor initializers.

Also, rename this variable. It's not a "dirLister" even though you use a K(Core)DirLister object. You listen for tags. Call it something like m_tagsLister or so.

markg: Set a parent (you can use "this" i think), then you don't have to delete it later on (the…
Context not available.
89 92​
90 createSystemBookmarks(); 93​ createSystemBookmarks();
91 initializeAvailableDevices(); 94​ initializeAvailableDevices();
92 loadBookmarks(); 95​
96​ // if tags are supported load them before loading the bookmarks
emmanuelpUnsubmitted
Done

Why?

emmanuelp: Why?
nicolasfellaAuthorUnsubmitted
Done

Loading the Tags takes a while because of the async ListJob. I want to have all m_systemBookmarks items loaded when calling loadBookmarks() to use the same codepath for all items. This could be implemented otherwise if desired

nicolasfella: Loading the Tags takes a while because of the async ListJob. I want to have all…
97​ if (KProtocolInfo::isKnownProtocol(QStringLiteral("tags"))) {
98​ createTagBookmarks();
anthonyfieroniUnsubmitted
Not Done

Remove tag?

anthonyfieroni: Remove tag?
nicolasfellaAuthorUnsubmitted
Not Done

KCoreDirLister's itemsDeletedSignal is not emited when Tags are removed via Information Panel. In the tags IOSlave delete is not implemented.

nicolasfella: KCoreDirLister's itemsDeletedSignal is not emited when Tags are removed via Information Panel.
99​ } else {
100​ loadBookmarks();
101​ }
93 102​
94 const int syncBookmarksTimeout = 100; 103​ const int syncBookmarksTimeout = 100;
95 104​
anthonyfieroniUnsubmitted
Done

a == b

anthonyfieroni: a == b
markgUnsubmitted
Not Done

Remove this line if initializing m_dirLister in the constructor initialize section.

markg: Remove this line if initializing m_dirLister in the constructor initialize section.
Context not available.
731 QList<PlacesItem*> recentlySavedItems; 740​ QList<PlacesItem*> recentlySavedItems;
732 QList<PlacesItem*> searchForItems; 741​ QList<PlacesItem*> searchForItems;
733 QList<PlacesItem*> devicesItems; 742​ QList<PlacesItem*> devicesItems;
743​ QList<PlacesItem*> tagsItems;
734 744​
735 while (!bookmark.isNull()) { 745​ while (!bookmark.isNull()) {
736 if (acceptBookmark(bookmark, devices)) { 746​ if (acceptBookmark(bookmark, devices)) {
Context not available.
756 case PlacesItem::PlacesType: placesItems.append(item); break; 766​ case PlacesItem::PlacesType: placesItems.append(item); break;
757 case PlacesItem::RecentlySavedType: recentlySavedItems.append(item); break; 767​ case PlacesItem::RecentlySavedType: recentlySavedItems.append(item); break;
758 case PlacesItem::SearchForType: searchForItems.append(item); break; 768​ case PlacesItem::SearchForType: searchForItems.append(item); break;
769​ case PlacesItem::TagsType: tagsItems.append(item); break;
759 case PlacesItem::DevicesType: 770​ case PlacesItem::DevicesType:
760 default: Q_ASSERT(false); break; 771​ default: Q_ASSERT(false); break;
761 } 772​ }
Context not available.
775 case PlacesItem::PlacesType: placesItems.append(item); break; 786​ case PlacesItem::PlacesType: placesItems.append(item); break;
776 case PlacesItem::RecentlySavedType: recentlySavedItems.append(item); break; 787​ case PlacesItem::RecentlySavedType: recentlySavedItems.append(item); break;
777 case PlacesItem::SearchForType: searchForItems.append(item); break; 788​ case PlacesItem::SearchForType: searchForItems.append(item); break;
789​ case PlacesItem::TagsType: tagsItems.append(item); break;
778 case PlacesItem::DevicesType: 790​ case PlacesItem::DevicesType:
779 default: Q_ASSERT(false); break; 791​ default: Q_ASSERT(false); break;
780 } 792​ }
Context not available.
794 items.append(recentlySavedItems); 806​ items.append(recentlySavedItems);
795 items.append(searchForItems); 807​ items.append(searchForItems);
796 items.append(devicesItems); 808​ items.append(devicesItems);
809​ items.append(tagsItems);
797 810​
798 foreach (PlacesItem* item, items) { 811​ foreach (PlacesItem* item, items) {
799 if (!m_hiddenItemsShown && item->isHidden()) { 812​ if (!m_hiddenItemsShown && item->isHidden()) {
Context not available.
1167 1180​
1168 return query.toSearchUrl(); 1181​ return query.toSearchUrl();
1169} 1182​}
1183​
1184​void PlacesItemModel::createTagBookmarks()
1185​{
1186​ KIO::ListJob* job = KIO::listDir(QUrl(tagsUrlBase), KIO::HideProgressInfo, false);
1187​ connect(job, &KIO::ListJob::entries, this, &PlacesItemModel::addTag);
1188​ connect(job, &KJob::result, this, &PlacesItemModel::applyTags);
anthonyfieroniUnsubmitted
Done

Name should be addTag, because you add it one by one.

anthonyfieroni: Name should be addTag, because you add it one by one.
1189​ job->start();
1190​}
1191​
anthonyfieroniUnsubmitted
Done

const

anthonyfieroni: const
1192​void PlacesItemModel::applyTags()
anthonyfieroniUnsubmitted
Done

Remove it, use m_tags.size instead.

anthonyfieroni: Remove it, use m_tags.size instead.
1193​{
1194​ m_systemBookmarks.append(SystemBookmarkData(QUrl(tagsUrlBase),
anthonyfieroniUnsubmitted
Done

This is a copy, use const kFileItem &item instead

anthonyfieroni: This is a copy, use const kFileItem &item instead
1195​ QStringLiteral("tag"),
alexeyminUnsubmitted
Done

weird indentation

alexeymin: weird indentation
anthonyfieroniUnsubmitted
Done

const, make all unmodified variables const ones.

anthonyfieroni: const, make all unmodified variables const ones.
1196​ I18N_NOOP2("KFile System Bookmarks", "All tags")));
1197​
1198​ for (QString tag: m_tags) {
1199​ m_systemBookmarks.append(SystemBookmarkData(QUrl(tagsUrlBase+tag),
alexeyminUnsubmitted
Done

indentation

alexeymin: indentation
1200​ QStringLiteral("tag"),
1201​ tag));
1202​ }
anthonyfieroniUnsubmitted
Done

Move out from loop, it's true only one time

anthonyfieroni: Move out from loop, it's true only one time
1203​ loadBookmarks();
1204​}
1205​
1206​void PlacesItemModel::addTag(KIO::Job* job, const KIO::UDSEntryList& entries)
alexeyminUnsubmitted
Done

extra space

alexeymin: extra space
alexeyminUnsubmitted
Done

warning: unused parameter ‘job’ [-Wunused-parameter]
Q_UNUSED(job) ?

alexeymin: warning: unused parameter ‘job’ [-Wunused-parameter] Q_UNUSED(job) ?
1207​{
1208​ Q_UNUSED(job);
1209​ KIO::UDSEntryList::ConstIterator it = entries.begin();
1210​ const KIO::UDSEntryList::ConstIterator end = entries.end();
1211​
1212​ for (; it != end; ++it) {
1213​ const KIO::UDSEntry& entry = *it;
alexeyminUnsubmitted
Done

extra space

alexeymin: extra space
1214​ QString name = entry.stringValue( KIO::UDSEntry::UDS_NAME);
emmanuelpUnsubmitted
Done

Use the URL instead.

emmanuelp: Use the URL instead.
nicolasfellaAuthorUnsubmitted
Done

Do you mean UDS_URL instead of UDS_NAME? Why?

nicolasfella: Do you mean UDS_URL instead of UDS_NAME? Why?
emmanuelpUnsubmitted
Done

Ups mixed things up. UDSEntry doesn't have a url() method, only KFileItem has it.

Why?

The problem is that UDS_URL may be different from baseUrl + UDS_NAME, *depending* on the implementation of the ioslave. KFileItem handles this by checking if UDS_URL is set or not and handles it using two different code paths. But a user of KIO shouldn't depend on such implementation details, otherwise it may break in future if something changes in the ioslave.

I also think that we should use a KDirLister instead because:

  • It gives us KFileItems
  • We can react to itemsAdded and itemsDeleted signals -> shows new tags without restarting Dolphin
emmanuelp: Ups mixed things up. `UDSEntry` doesn't have a `url()` method, only `KFileItem` has it. > Why?
1215​
1216​ if (!m_tags.contains(name)) {
1217​ m_tags.append(name);
1218​ }
1219​ }
1220​}
1221​
1170#endif 1222​#endif
1171 1223​
1172#ifdef PLACESITEMMODEL_DEBUG 1224​#ifdef PLACESITEMMODEL_DEBUG
Context not available.