From 7e5b7d562672ecda1ec2db0cceb80a17407150f0 Mon Sep 17 00:00:00 2001 From: Frank Reininghaus Date: Wed, 14 Aug 2013 23:40:02 +0200 Subject: Fix crash when disabling "Show in groups" The problem was that items are removed from m_visibleGroups while a QMutableHashIterator iterates over this hash, such that the iterator can become invalid. The solution is to use a QHashIterator instead, which takes a copy of the hash. Therefore, it is not affected if m_visibleGroups is modified in any way. BUG: 323248 FIXED-IN: 4.11.1 REVIEW: 111919 --- src/kitemviews/kitemlistview.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'src') diff --git a/src/kitemviews/kitemlistview.cpp b/src/kitemviews/kitemlistview.cpp index 0c3183cd5..7f1526151 100644 --- a/src/kitemviews/kitemlistview.cpp +++ b/src/kitemviews/kitemlistview.cpp @@ -1237,8 +1237,10 @@ void KItemListView::slotGroupedSortingChanged(bool current) if (m_grouped) { updateGroupHeaderHeight(); } else { - // Clear all visible headers - QMutableHashIterator it (m_visibleGroups); + // Clear all visible headers. Note that the QHashIterator takes a copy of + // m_visibleGroups. Therefore, it remains valid even if items are removed + // from m_visibleGroups in recycleGroupHeaderForWidget(). + QHashIterator it(m_visibleGroups); while (it.hasNext()) { it.next(); recycleGroupHeaderForWidget(it.key()); -- cgit v1.3 From fd5ba3b4b22e923cff11b65654e9b6f931ce9a3c Mon Sep 17 00:00:00 2001 From: Frank Reininghaus Date: Wed, 14 Aug 2013 23:44:57 +0200 Subject: Fix slow scrolling when hidden files or symbolic links are shown The problem was that we drawed the overlays using KIconLoader, which can be very slow, every time an item appeared on the screen. This commit makes sure that not only the icon, but the icon including overlays is cached in QPixmapCache. Therefore, the overlay drawing is done just once for each icon+overlays combination. For previews, the overlay drawing is done in KFileItemModelRolesUpdater just after the preview is received. BUG: 310662 BUG: 314339 FIXED-IN: 4.11.1 REVIEW: 111956 --- src/kitemviews/kfileitemmodelrolesupdater.cpp | 16 ++++++++++++ src/kitemviews/kstandarditemlistwidget.cpp | 37 +++++++++++++-------------- src/kitemviews/kstandarditemlistwidget.h | 2 +- 3 files changed, 35 insertions(+), 20 deletions(-) (limited to 'src') diff --git a/src/kitemviews/kfileitemmodelrolesupdater.cpp b/src/kitemviews/kfileitemmodelrolesupdater.cpp index 698a6c5f0..e9b69566d 100644 --- a/src/kitemviews/kfileitemmodelrolesupdater.cpp +++ b/src/kitemviews/kfileitemmodelrolesupdater.cpp @@ -570,6 +570,22 @@ void KFileItemModelRolesUpdater::slotGotPreview(const KFileItem& item, const QPi } QHash data = rolesData(item); + + const QStringList overlays = data["iconOverlays"].toStringList(); + // Strangely KFileItem::overlays() returns empty string-values, so + // we need to check first whether an overlay must be drawn at all. + // It is more efficient to do it here, as KIconLoader::drawOverlays() + // assumes that an overlay will be drawn and has some additional + // setup time. + foreach (const QString& overlay, overlays) { + if (!overlay.isEmpty()) { + // There is at least one overlay, draw all overlays above m_pixmap + // and cancel the check + KIconLoader::global()->drawOverlays(overlays, scaledPixmap, KIconLoader::Desktop); + break; + } + } + data.insert("iconPixmap", scaledPixmap); disconnect(m_model, SIGNAL(itemsChanged(KItemRangeList,QSet)), diff --git a/src/kitemviews/kstandarditemlistwidget.cpp b/src/kitemviews/kstandarditemlistwidget.cpp index b429211b8..621cd81cc 100644 --- a/src/kitemviews/kstandarditemlistwidget.cpp +++ b/src/kitemviews/kstandarditemlistwidget.cpp @@ -839,29 +839,14 @@ void KStandardItemListWidget::updatePixmapCache() // use a generic icon as fallback iconName = QLatin1String("unknown"); } - m_pixmap = pixmapForIcon(iconName, maxIconHeight); + const QStringList overlays = values["iconOverlays"].toStringList(); + m_pixmap = pixmapForIcon(iconName, overlays, maxIconHeight); } else if (m_pixmap.width() != maxIconWidth || m_pixmap.height() != maxIconHeight) { // A custom pixmap has been applied. Assure that the pixmap // is scaled to the maximum available size. KPixmapModifier::scale(m_pixmap, QSize(maxIconWidth, maxIconHeight)); } - const QStringList overlays = values["iconOverlays"].toStringList(); - - // Strangely KFileItem::overlays() returns empty string-values, so - // we need to check first whether an overlay must be drawn at all. - // It is more efficient to do it here, as KIconLoader::drawOverlays() - // assumes that an overlay will be drawn and has some additional - // setup time. - foreach (const QString& overlay, overlays) { - if (!overlay.isEmpty()) { - // There is at least one overlay, draw all overlays above m_pixmap - // and cancel the check - KIconLoader::global()->drawOverlays(overlays, m_pixmap, KIconLoader::Desktop); - break; - } - } - if (m_isCut) { KIconEffect* effect = KIconLoader::global()->iconEffect(); m_pixmap = effect->apply(m_pixmap, KIconLoader::Desktop, KIconLoader::DisabledState); @@ -1323,9 +1308,9 @@ void KStandardItemListWidget::closeRoleEditor() m_roleEditor = 0; } -QPixmap KStandardItemListWidget::pixmapForIcon(const QString& name, int size) +QPixmap KStandardItemListWidget::pixmapForIcon(const QString& name, const QStringList& overlays, int size) { - const QString key = "KStandardItemListWidget:" % name % ":" % QString::number(size); + const QString key = "KStandardItemListWidget:" % name % ":" % overlays.join(":") % ":" % QString::number(size); QPixmap pixmap; if (!QPixmapCache::find(key, pixmap)) { @@ -1355,6 +1340,20 @@ QPixmap KStandardItemListWidget::pixmapForIcon(const QString& name, int size) KPixmapModifier::scale(pixmap, QSize(size, size)); } + // Strangely KFileItem::overlays() returns empty string-values, so + // we need to check first whether an overlay must be drawn at all. + // It is more efficient to do it here, as KIconLoader::drawOverlays() + // assumes that an overlay will be drawn and has some additional + // setup time. + foreach (const QString& overlay, overlays) { + if (!overlay.isEmpty()) { + // There is at least one overlay, draw all overlays above m_pixmap + // and cancel the check + KIconLoader::global()->drawOverlays(overlays, pixmap, KIconLoader::Desktop); + break; + } + } + QPixmapCache::insert(key, pixmap); } diff --git a/src/kitemviews/kstandarditemlistwidget.h b/src/kitemviews/kstandarditemlistwidget.h index ecf3a3b60..4bf6116fd 100644 --- a/src/kitemviews/kstandarditemlistwidget.h +++ b/src/kitemviews/kstandarditemlistwidget.h @@ -186,7 +186,7 @@ private: */ void closeRoleEditor(); - static QPixmap pixmapForIcon(const QString& name, int size); + static QPixmap pixmapForIcon(const QString& name, const QStringList& overlays, int size); /** * @return Preferred size of the rating-image based on the given -- cgit v1.3 From 59723fca41a44e4d1ee753e93589027cbdf8d20d Mon Sep 17 00:00:00 2001 From: Frank Reininghaus Date: Wed, 14 Aug 2013 23:57:51 +0200 Subject: Do not store default values in QHash Storing values which are equivalent to default-constructed QVariants does not make much sense because QHash::value returns the same value even if the corresponding key is not found in the hash. This commit reduces Dolphin's memory consumption in large folders by up to 7.3% (tested a folder with 100,000 files in Details View) and reduces the time required for loading a folder. BUG: 323517 FIXED-IN: 4.11.1 REVIEW: 111922 --- src/kitemviews/kfileitemmodel.cpp | 27 ++++++++++----------------- src/kitemviews/kstandarditemlistwidget.cpp | 1 - 2 files changed, 10 insertions(+), 18 deletions(-) (limited to 'src') diff --git a/src/kitemviews/kfileitemmodel.cpp b/src/kitemviews/kfileitemmodel.cpp index 1b4911dec..c0dedff39 100644 --- a/src/kitemviews/kfileitemmodel.cpp +++ b/src/kitemviews/kfileitemmodel.cpp @@ -1275,25 +1275,20 @@ QHash KFileItemModel::retrieveData(const KFileItem& item, data.insert(sharedValue("url"), item.url()); const bool isDir = item.isDir(); - if (m_requestRole[IsDirRole]) { - data.insert(sharedValue("isDir"), isDir); + if (m_requestRole[IsDirRole] && isDir) { + data.insert(sharedValue("isDir"), true); } - if (m_requestRole[IsLinkRole]) { - const bool isLink = item.isLink(); - data.insert(sharedValue("isLink"), isLink); + if (m_requestRole[IsLinkRole] && item.isLink()) { + data.insert(sharedValue("isLink"), true); } if (m_requestRole[NameRole]) { data.insert(sharedValue("text"), item.text()); } - if (m_requestRole[SizeRole]) { - if (isDir) { - data.insert(sharedValue("size"), QVariant()); - } else { - data.insert(sharedValue("size"), item.size()); - } + if (m_requestRole[SizeRole] && !isDir) { + data.insert(sharedValue("size"), item.size()); } if (m_requestRole[DateRole]) { @@ -1347,17 +1342,15 @@ QHash KFileItemModel::retrieveData(const KFileItem& item, data.insert(sharedValue("path"), path); } - if (m_requestRole[IsExpandableRole]) { - data.insert(sharedValue("isExpandable"), item.isDir()); + if (m_requestRole[IsExpandableRole] && isDir) { + data.insert(sharedValue("isExpandable"), true); } if (m_requestRole[ExpandedParentsCountRole]) { - int level = 0; if (parent) { - level = parent->values["expandedParentsCount"].toInt() + 1; + const int level = parent->values["expandedParentsCount"].toInt() + 1; + data.insert(sharedValue("expandedParentsCount"), level); } - - data.insert(sharedValue("expandedParentsCount"), level); } if (item.isMimeTypeKnown()) { diff --git a/src/kitemviews/kstandarditemlistwidget.cpp b/src/kitemviews/kstandarditemlistwidget.cpp index 621cd81cc..2a89004c6 100644 --- a/src/kitemviews/kstandarditemlistwidget.cpp +++ b/src/kitemviews/kstandarditemlistwidget.cpp @@ -791,7 +791,6 @@ void KStandardItemListWidget::updateExpansionArea() { if (m_supportsItemExpanding) { const QHash values = data(); - Q_ASSERT(values.contains("expandedParentsCount")); const int expandedParentsCount = values.value("expandedParentsCount", 0).toInt(); if (expandedParentsCount >= 0) { const qreal widgetHeight = size().height(); -- cgit v1.3 From 7c99a9c2ad4455c65a218c53dfa7f6376f389b66 Mon Sep 17 00:00:00 2001 From: Frank Reininghaus Date: Thu, 15 Aug 2013 00:07:43 +0200 Subject: Make sure that the sort order is correct after renaming KFileItemModel::setData() should not only cause a resorting when the sort role is changed. The name is always used as a fallback if the sort role of multiple files is equal, therefore, renaming a file can change the correct order of the files even if the files are not sorted by "name". Unit test included. BUG: 323518 FIXED-IN: 4.11.1 REVIEW: 111721 --- src/kitemviews/kfileitemmodel.cpp | 26 ++++++++++++++++++++++++-- src/tests/kfileitemmodeltest.cpp | 25 +++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 2 deletions(-) (limited to 'src') diff --git a/src/kitemviews/kfileitemmodel.cpp b/src/kitemviews/kfileitemmodel.cpp index c0dedff39..70e8834a6 100644 --- a/src/kitemviews/kfileitemmodel.cpp +++ b/src/kitemviews/kfileitemmodel.cpp @@ -192,8 +192,30 @@ bool KFileItemModel::setData(int index, const QHash& value emit itemsChanged(KItemRangeList() << KItemRange(index, 1), changedRoles); - if (changedRoles.contains(sortRole())) { - m_resortAllItemsTimer->start(); + // Trigger a resorting if the item's correct position has changed. Note + // that this can happen even if the sort role has not changed at all + // because the file name can be used as a fallback. + if (changedRoles.contains(sortRole()) || changedRoles.contains(roleForType(NameRole))) { + // Compare the changed item with its neighbors to see + // if an expensive resorting is needed at all. + const ItemData* changedItem = m_itemData.at(index); + const ItemData* previousItem = (index == 0) ? 0 : m_itemData.at(index - 1); + const ItemData* nextItem = (index == m_itemData.count() - 1) ? 0 : m_itemData.at(index + 1); + + if ((previousItem && lessThan(changedItem, previousItem)) + || (nextItem && lessThan(nextItem, changedItem))) { + m_resortAllItemsTimer->start(); + } else if (groupedSorting() && changedRoles.contains(sortRole())) { + // The position is still correct, but the groups might have changed + // if the changed item is either the first or the last item in a + // group. + // In principle, we could try to find out if the item really is the + // first or last one in its group and then update the groups + // (possibly with a delayed timer to make sure that we don't + // re-calculate the groups very often if items are updated one by + // one), but starting m_resortAllItemsTimer is easier. + m_resortAllItemsTimer->start(); + } } return true; diff --git a/src/tests/kfileitemmodeltest.cpp b/src/tests/kfileitemmodeltest.cpp index 513ecef5a..f8439789b 100644 --- a/src/tests/kfileitemmodeltest.cpp +++ b/src/tests/kfileitemmodeltest.cpp @@ -70,6 +70,7 @@ private slots: void testSetDataWithModifiedSortRole_data(); void testSetDataWithModifiedSortRole(); void testChangeSortRole(); + void testResortAfterChangingName(); void testModelConsistencyWhenInsertingItems(); void testItemRangeConsistencyWhenInsertingItems(); void testExpandItems(); @@ -368,6 +369,30 @@ void KFileItemModelTest::testChangeSortRole() QVERIFY(ok1 || ok2); } +void KFileItemModelTest::testResortAfterChangingName() +{ + // We sort by size in a directory where all files have the same size. + // Therefore, the files are sorted by their names. + m_model->setSortRole("size"); + + QStringList files; + files << "a.txt" << "b.txt" << "c.txt"; + m_testDir->createFiles(files); + + m_model->loadDirectory(m_testDir->url()); + QVERIFY(QTest::kWaitForSignal(m_model, SIGNAL(itemsInserted(KItemRangeList)), DefaultTimeout)); + QCOMPARE(itemsInModel(), QStringList() << "a.txt" << "b.txt" << "c.txt"); + + // We rename a.txt to d.txt. Even though the size has not changed at all, + // the model must re-sort the items. + QHash data; + data.insert("text", "d.txt"); + m_model->setData(0, data); + + QVERIFY(QTest::kWaitForSignal(m_model, SIGNAL(itemsMoved(KItemRange,QList)), DefaultTimeout)); + QCOMPARE(itemsInModel(), QStringList() << "b.txt" << "c.txt" << "d.txt"); +} + void KFileItemModelTest::testModelConsistencyWhenInsertingItems() { //QSKIP("Temporary disabled", SkipSingle); -- cgit v1.3