Changeset View
Standalone View
containments/desktop/plugins/folder/foldermodel.cpp
| Show First 20 Lines • Show All 143 Lines • ▼ Show 20 Line(s) | 143 | QObject::connect(dirLister, myCanceledSignal, this, [this] { | |||
|---|---|---|---|---|---|
| 144 | setStatus(Status::Canceled); | 144 | setStatus(Status::Canceled); | ||
| 145 | emit listingCanceled(); | 145 | emit listingCanceled(); | ||
| 146 | }); | 146 | }); | ||
| 147 | 147 | | |||
| 148 | m_dirModel = new KDirModel(this); | 148 | m_dirModel = new KDirModel(this); | ||
| 149 | m_dirModel->setDirLister(dirLister); | 149 | m_dirModel->setDirLister(dirLister); | ||
| 150 | m_dirModel->setDropsAllowed(KDirModel::DropOnDirectory | KDirModel::DropOnLocalExecutable); | 150 | m_dirModel->setDropsAllowed(KDirModel::DropOnDirectory | KDirModel::DropOnLocalExecutable); | ||
| 151 | 151 | | |||
| 152 | /* | 152 | /* | ||
fvogt: The comment refers to the first connect now, but is mostly aimed at the second. Maybe move it… | |||||
| 153 | * position dropped items at the desired target position | 153 | * position dropped items at the desired target position | ||
| 154 | * delay this via queued connection, such that the row is available and can be mapped | 154 | * delay this via queued connection, such that the row is available and can be mapped | ||
It doesn't use Qt::QueuedConnection explicitly - is the comment wrong or is it done implicitly? fvogt: It doesn't use Qt::QueuedConnection explicitly - is the comment wrong or is it done implicitly? | |||||
McPain: According to e94888701 and cc9c3d32a, the comment is wrong. | |||||
| 155 | * when we emit the move request | 155 | * when we emit the move request | ||
| 156 | */ | 156 | */ | ||
| 157 | connect(this, &QAbstractItemModel::rowsAboutToBeInserted, | ||||
| 158 | this, [this]() { | ||||
| 159 | if (!m_dropTargetPositions.isEmpty()) { | ||||
| 160 | setSortMode(-1); | ||||
| 161 | } | ||||
| 162 | }); | ||||
| 163 | | ||||
| 157 | connect(this, &QAbstractItemModel::rowsInserted, | 164 | connect(this, &QAbstractItemModel::rowsInserted, | ||
| 158 | this, [this](const QModelIndex &parent, int first, int last) { | 165 | this, [this](const QModelIndex &parent, int first, int last) { | ||
I'm a bit worried about queuing something with indexes. Indexes are only valid at that exact moment. If the source model does the following: When you process the delayed connection for the first insertion the old first/last could be pointing anywhere. We could maybe get round this by converting to QPeristentModelIndexes immediately, then queue up the operation that uses them. davidedmundson: I'm a bit worried about queuing something with indexes. Indexes are only valid at that exact… | |||||
Also I don't think I understand this:
at the point of rowsInserted() the row should be available, If not we have a source model bug. davidedmundson: Also I don't think I understand this:
* Delay this via queued connection, such that the… | |||||
like I said, in https://phabricator.kde.org/R119:cc9c3d32a381980e367aa893c6796d611c7a33a7 the Qt::QueuedConnection was removed from connection, but the comment was unchanged McPain: like I said, in https://phabricator.kde.org/R119:cc9c3d32a381980e367aa893c6796d611c7a33a7 the… | |||||
| 159 | for (int i = first; i <= last; ++i) { | 166 | for (int i = first; i <= last; ++i) { | ||
| 160 | const auto idx = index(i, 0, parent); | 167 | const auto idx = index(i, 0, parent); | ||
| 161 | const auto url = itemForIndex(idx).url(); | 168 | const auto url = itemForIndex(idx).url(); | ||
| 162 | auto it = m_dropTargetPositions.find(url.fileName()); | 169 | auto it = m_dropTargetPositions.find(url.fileName()); | ||
| 163 | if (it != m_dropTargetPositions.end()) { | 170 | if (it != m_dropTargetPositions.end()) { | ||
| 164 | const auto pos = it.value(); | 171 | const auto pos = it.value(); | ||
| 165 | m_dropTargetPositions.erase(it); | 172 | m_dropTargetPositions.erase(it); | ||
| 166 | setSortMode(-1); | 173 | setSortMode(-1); | ||
| ▲ Show 20 Lines • Show All 1810 Lines • Show Last 20 Lines | |||||
The comment refers to the first connect now, but is mostly aimed at the second. Maybe move it and add another one for the added connect call?