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

Changeset View

Standalone View

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 <KCoreDirLister>
42		43
43	#include <Solid/Device>	44		#include <Solid/Device>
44	#include <Solid/DeviceNotifier>	45		#include <Solid/DeviceNotifier>
Context not available.
78	m_bookmarkedItems(),	79		m_bookmarkedItems(),
79	m_hiddenItemToRemove(-1),	80		m_hiddenItemToRemove(-1),
80	m_updateBookmarksTimer(0),	81		m_updateBookmarksTimer(0),
81	m_storageSetupInProgress()	82		m_storageSetupInProgress(),
		83		m_tags()
82	{	84		{
			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
83	#ifdef HAVE_BALOO	85		#ifdef HAVE_BALOO
84	Baloo::IndexerConfig config;	86		Baloo::IndexerConfig config;
Context not available.
86	#endif	88		#endif
87	const QString file = QStandardPaths::writableLocation(QStandardPaths::GenericDataLocation) + "/user-places.xbel";	89		const QString file = QStandardPaths::writableLocation(QStandardPaths::GenericDataLocation) + "/user-places.xbel";
88	m_bookmarkManager = KBookmarkManager::managerForExternalFile(file);	90		m_bookmarkManager = KBookmarkManager::managerForExternalFile(file);
		91		m_dirLister = new KCoreDirLister();
			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…
89		92
90	createSystemBookmarks();	93		createSystemBookmarks();
91	initializeAvailableDevices();	94		initializeAvailableDevices();
		95
		96		if (KProtocolInfo::isKnownProtocol(QStringLiteral("tags"))) {
			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		connect(m_dirLister, &KCoreDirLister::itemsAdded, this, &PlacesItemModel::addTag);
			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.
		98		m_dirLister->openUrl(QUrl(m_tagsUrlBase));
		99		}
		100
92	loadBookmarks();	101		loadBookmarks();
93		102
94	const int syncBookmarksTimeout = 100;	103		const int syncBookmarksTimeout = 100;
Context not available.
106	{	115		{
107	qDeleteAll(m_bookmarkedItems);	116		qDeleteAll(m_bookmarkedItems);
108	m_bookmarkedItems.clear();	117		m_bookmarkedItems.clear();
		118		delete m_dirLister;
			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.
109	}	119		}
110		120
111	PlacesItem* PlacesItemModel::createPlacesItem(const QString& text,	121		PlacesItem* PlacesItemModel::createPlacesItem(const QString& text,
Context not available.
550		560
551	m_availableDevices << udi;	561		m_availableDevices << udi;
552	const KBookmark bookmark = PlacesItem::createDeviceBookmark(m_bookmarkManager, udi);	562		const KBookmark bookmark = PlacesItem::createDeviceBookmark(m_bookmarkManager, udi);
553	appendItem(new PlacesItem(bookmark));	563		appendItemToGroup(new PlacesItem(bookmark));
554	}	564		}
555		565
556	void PlacesItemModel::slotDeviceRemoved(const QString& udi)	566		void PlacesItemModel::slotDeviceRemoved(const QString& udi)
Context not available.
661	PlacesItem* item = new PlacesItem(newBookmark);	671		PlacesItem* item = new PlacesItem(newBookmark);
662	if (item->isHidden() && !m_hiddenItemsShown) {	672		if (item->isHidden() && !m_hiddenItemsShown) {
663	m_bookmarkedItems.append(item);	673		m_bookmarkedItems.append(item);
664	} else {	674		} else if (!(item->groupType() == PlacesItem::TagsType)) {
665	appendItemToGroup(item);	675		appendItemToGroup(item);
666	}	676		}
667	}	677		}
Context not available.
680	item = placesItem(modelIndex);	690		item = placesItem(modelIndex);
681	}	691		}
682		692
		693		if (item->groupType() == PlacesItem::TagsType){
			anthonyfieroniUnsubmitted Done a == b anthonyfieroni: a == b
		694		continue;
		695		}
		696
683	bool hasBeenRemoved = true;	697		bool hasBeenRemoved = true;
684	const KBookmark oldBookmark = item->bookmark();	698		const KBookmark oldBookmark = item->bookmark();
685	KBookmark newBookmark = root.first();	699		KBookmark newBookmark = root.first();
Context not available.
756	case PlacesItem::PlacesType: placesItems.append(item); break;	770		case PlacesItem::PlacesType: placesItems.append(item); break;
757	case PlacesItem::RecentlySavedType: recentlySavedItems.append(item); break;	771		case PlacesItem::RecentlySavedType: recentlySavedItems.append(item); break;
758	case PlacesItem::SearchForType: searchForItems.append(item); break;	772		case PlacesItem::SearchForType: searchForItems.append(item); break;
		773		case PlacesItem::TagsType: break;
759	case PlacesItem::DevicesType:	774		case PlacesItem::DevicesType:
760	default: Q_ASSERT(false); break;	775		default: Q_ASSERT(false); break;
761	}	776		}
Context not available.
775	case PlacesItem::PlacesType: placesItems.append(item); break;	790		case PlacesItem::PlacesType: placesItems.append(item); break;
776	case PlacesItem::RecentlySavedType: recentlySavedItems.append(item); break;	791		case PlacesItem::RecentlySavedType: recentlySavedItems.append(item); break;
777	case PlacesItem::SearchForType: searchForItems.append(item); break;	792		case PlacesItem::SearchForType: searchForItems.append(item); break;
		793		case PlacesItem::TagsType: break;
778	case PlacesItem::DevicesType:	794		case PlacesItem::DevicesType:
779	default: Q_ASSERT(false); break;	795		default: Q_ASSERT(false); break;
780	}	796		}
Context not available.
1167		1183
1168	return query.toSearchUrl();	1184		return query.toSearchUrl();
1169	}	1185		}
		1186
		1187		void PlacesItemModel::addTag(const QUrl& directoryUrl, const KFileItemList& items)
			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.
		1188		{
		1189		Q_UNUSED(directoryUrl);
		1190		const int maxTags = 8;
			anthonyfieroniUnsubmitted Done const anthonyfieroni: const
		1191
			anthonyfieroniUnsubmitted Done Remove it, use m_tags.size instead. anthonyfieroni: Remove it, use m_tags.size instead.
		1192		if (m_tags.isEmpty()) {
		1193		PlacesItem* allTagsItem = createSystemPlacesItem(SystemBookmarkData(QUrl(m_tagsUrlBase),
			anthonyfieroniUnsubmitted Done This is a copy, use const kFileItem &item instead anthonyfieroni: This is a copy, use const kFileItem &item instead
		1194		QStringLiteral("tag"),
			anthonyfieroniUnsubmitted Done const, make all unmodified variables const ones. anthonyfieroni: const, make all unmodified variables const ones.
		1195		I18N_NOOP2("KFile System Bookmarks", "All tags")));
			alexeyminUnsubmitted Done weird indentation alexeymin: weird indentation
		1196		appendItemToGroup(allTagsItem);
		1197		}
		1198
		1199		for (const KFileItem &item: items) {
			alexeyminUnsubmitted Done indentation alexeymin: indentation
		1200		const QString name = item.name();
		1201
			anthonyfieroniUnsubmitted Done Move out from loop, it's true only one time anthonyfieroni: Move out from loop, it's true only one time
		1202		if (!m_tags.contains(name) && m_tags.size() < maxTags) {
		1203		m_tags.append(name);
		1204		PlacesItem* placesItem = createSystemPlacesItem(SystemBookmarkData(QUrl(item.url()),
		1205		QStringLiteral("tag"),
		1206		name));
			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		appendItemToGroup(placesItem);
		1208		}
		1209		}
		1210		}
		1211
1170	#endif	1212		#endif
1171		1213
			alexeyminUnsubmitted Done extra space alexeymin: extra space
1172	#ifdef PLACESITEMMODEL_DEBUG	1214		#ifdef PLACESITEMMODEL_DEBUG
Context not available.
			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?