Commit 98d56d91 authored by Andras Mantia's avatar Andras Mantia

Update the proxy to source and source to proxy mapping correctly

Summary:
The problem is visible when an item is moved to another screen and moved back.
When it is moved out, the proxy-source mapping changes (as the items
is filtered out from the underlining model), while when moved back, it is
changed again, but in a wrong way that breaks the mapping and results
in "losing" one mapping, thus one item disappears.
There is a unit test added for it.

Reviewers: #plasma, hein, mwolff

Reviewed By: #plasma, hein, mwolff

Subscribers: plasma-devel

Tags: #plasma

Differential Revision: https://phabricator.kde.org/D8915
parent cc9c3d32
......@@ -29,6 +29,7 @@
#include "foldermodel.h"
#include "positioner.h"
#include "screenmapper.h"
QTEST_MAIN(PositionerTest)
......@@ -58,6 +59,9 @@ void PositionerTest::cleanupTestCase()
void PositionerTest::init()
{
m_folderModel = new FolderModel(this);
m_folderModel->setScreen(0);
m_folderModel->setScreenMapper(ScreenMapper::instance());
m_folderModel->setUsedByContainment(true);
m_positioner = new Positioner(this);
m_positioner->setEnabled(true);
m_positioner->setFolderModel(m_folderModel);
......@@ -209,6 +213,98 @@ void PositionerTest::tst_changePerStripe()
QCOMPARE(s.count(), 2);
}
void PositionerTest::tst_proxyMapping()
{
auto *screenMapper = ScreenMapper::instance();
FolderModel secondFolderModel;
secondFolderModel.setUrl(m_folderDir->path() + QDir::separator() + desktop );
secondFolderModel.setUsedByContainment(true);
secondFolderModel.setScreenMapper(screenMapper);
secondFolderModel.setScreen(1);
Positioner secondPositioner;
secondPositioner.setEnabled(true);
secondPositioner.setFolderModel(&secondFolderModel);
secondPositioner.setPerStripe(3);
QSignalSpy s2(&secondFolderModel, &FolderModel::listingCompleted);
QVERIFY(s2.wait(1000));
QHash<int, int> expectedSource2ProxyScreen0;
QHash<int, int> expectedProxy2SourceScreen0;
QHash<int, int> expectedProxy2SourceScreen1;
QHash<int, int> expectedSource2ProxyScreen1;
for (int i = 0; i < m_folderModel->rowCount(); i++) {
expectedSource2ProxyScreen0[i] = i;
expectedProxy2SourceScreen0[i] = i;
}
// swap items 1 and 2 in the positioner
m_positioner->move({1, 2, 2, 1});
expectedSource2ProxyScreen0[1] = 2;
expectedSource2ProxyScreen0[2] = 1;
expectedProxy2SourceScreen0[1] = 2;
expectedProxy2SourceScreen0[2] = 1;
auto savedSource2ProxyScreen0 = expectedSource2ProxyScreen0;
auto savedProxy2SourceScreen0 = expectedProxy2SourceScreen0;
auto verifyMapping = [](const QHash<int, int> &actual, const QHash<int, int> &expected) {
auto ensureUnique = [](const QHash<int, int> mapping) {
auto values = mapping.values();
qSort(values);
auto uniqueValues = values.toSet().toList();
qSort(uniqueValues);
QVERIFY(uniqueValues == values);
};
ensureUnique(actual);
QCOMPARE(actual, expected);
};
verifyMapping(m_positioner->proxyToSourceMapping(), expectedProxy2SourceScreen0);
verifyMapping(m_positioner->sourceToProxyMapping(), expectedSource2ProxyScreen0);
verifyMapping(secondPositioner.proxyToSourceMapping(), expectedProxy2SourceScreen1);
verifyMapping(secondPositioner.sourceToProxyMapping(), expectedSource2ProxyScreen1);
const auto movedItem = m_folderModel->index(1, 0).data(FolderModel::UrlRole).toString();
// move the item 1 from source (now in position 2) to the second screen
screenMapper->addMapping(movedItem, 1);
expectedProxy2SourceScreen1[0] = 0;
expectedSource2ProxyScreen1[0] = 0;
expectedSource2ProxyScreen0.clear();
expectedProxy2SourceScreen0.clear();
for (int i = 0; i < m_folderModel->rowCount(); i++) {
// as item 1 disappeared, the mapping of all items after that are shifted
auto proxyIndex = (i <= 1) ? i : i + 1;
expectedProxy2SourceScreen0[proxyIndex] = i;
expectedSource2ProxyScreen0[i] = proxyIndex;
}
verifyMapping(m_positioner->proxyToSourceMapping(), expectedProxy2SourceScreen0);
verifyMapping(m_positioner->sourceToProxyMapping(), expectedSource2ProxyScreen0);
verifyMapping(secondPositioner.proxyToSourceMapping(), expectedProxy2SourceScreen1);
verifyMapping(secondPositioner.sourceToProxyMapping(), expectedSource2ProxyScreen1);
// move back the same item to the first screen
screenMapper->addMapping(movedItem, 0);
// nothing on the second screen
expectedSource2ProxyScreen1.clear();
expectedProxy2SourceScreen1.clear();
// first screen should look like in the beginning
expectedSource2ProxyScreen0 = savedSource2ProxyScreen0;
expectedProxy2SourceScreen0 = savedProxy2SourceScreen0;
verifyMapping(m_positioner->proxyToSourceMapping(), expectedProxy2SourceScreen0);
verifyMapping(m_positioner->sourceToProxyMapping(), expectedSource2ProxyScreen0);
verifyMapping(secondPositioner.proxyToSourceMapping(), expectedProxy2SourceScreen1);
verifyMapping(secondPositioner.sourceToProxyMapping(), expectedSource2ProxyScreen1);
}
void PositionerTest::checkPositions(int perStripe)
{
QSignalSpy s(m_positioner, &Positioner::positionsChanged);
......
......@@ -52,6 +52,7 @@ private Q_SLOTS:
void tst_defaultValues();
void tst_changeEnabledStatus();
void tst_changePerStripe();
void tst_proxyMapping();
private:
void checkPositions(int perStripe);
......
......@@ -425,6 +425,10 @@ void Positioner::move(const QVariantList &moves) {
const int newCount = rowCount();
if (newCount > oldCount) {
if (m_beginInsertRowsCalled) {
endInsertRows();
m_beginInsertRowsCalled = false;
}
beginInsertRows(QModelIndex(), oldCount, newCount - 1);
endInsertRows();
}
......@@ -510,7 +514,8 @@ void Positioner::sourceRowsAboutToBeInserted(const QModelIndex &parent, int star
if (m_enabled) {
if (m_proxyToSource.isEmpty()) {
if (!m_pendingPositions) {
emit beginInsertRows(parent, start, end);
beginInsertRows(parent, start, end);
m_beginInsertRowsCalled = true;
initMaps(end + 1);
}
......@@ -518,6 +523,19 @@ void Positioner::sourceRowsAboutToBeInserted(const QModelIndex &parent, int star
return;
}
// When new rows are inserted, they might go in the beginning or in the middle.
// In this case we must update first the existing proxy->source and source->proxy
// mapping, otherwise the proxy items will point to the wrong source item.
int count = end - start + 1;
m_sourceToProxy.clear();
for (auto it = m_proxyToSource.begin(); it != m_proxyToSource.end(); ++it) {
int sourceIdx = *it;
if (sourceIdx >= start) {
*it += count;
}
m_sourceToProxy[*it] = it.key();
}
int free = -1;
int rest = -1;
......@@ -538,6 +556,7 @@ void Positioner::sourceRowsAboutToBeInserted(const QModelIndex &parent, int star
int remainder = (end - rest);
beginInsertRows(parent, firstNew, firstNew + remainder);
m_beginInsertRowsCalled = true;
for (int i = 0; i <= remainder; ++i) {
updateMaps(firstNew + i, rest + i);
......@@ -547,6 +566,8 @@ void Positioner::sourceRowsAboutToBeInserted(const QModelIndex &parent, int star
}
} else {
emit beginInsertRows(parent, start, end);
beginInsertRows(parent, start, end);
m_beginInsertRowsCalled = true;
}
}
......@@ -617,7 +638,10 @@ void Positioner::sourceRowsInserted(const QModelIndex &parent, int first, int la
if (!m_ignoreNextTransaction) {
if (!m_pendingPositions) {
emit endInsertRows();
if (m_beginInsertRowsCalled) {
endInsertRows();
m_beginInsertRowsCalled = false;
}
} else {
applyPositions();
}
......
......@@ -76,6 +76,15 @@ class FOLDERPLUGIN_TESTS_EXPORT Positioner : public QAbstractItemModel
int rowCount(const QModelIndex &parent = QModelIndex()) const override;
int columnCount(const QModelIndex &parent = QModelIndex()) const override;
#ifdef BUILD_TESTING
QHash<int, int> proxyToSourceMapping() const {
return m_proxyToSource;
}
QHash<int, int> sourceToProxyMapping() const {
return m_sourceToProxy;
}
#endif
Q_SIGNALS:
void enabledChanged() const;
void folderModelChanged() const;
......@@ -128,6 +137,7 @@ class FOLDERPLUGIN_TESTS_EXPORT Positioner : public QAbstractItemModel
QHash<int, int> m_proxyToSource;
QHash<int, int> m_sourceToProxy;
bool m_beginInsertRowsCalled = false; // used to sync the amount of begin/endInsertRows calls
};
#endif
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment