Differential D21121 Diff 57904 src/server/storage/itemretriever.cpp

Changeset View

Standalone View

View Options

src/server/storage/itemretriever.cpp

Show First 20 Lines • Show All 220 Lines • ▼ Show 20 Line(s)213{
221 QByteArrayList parts; 221​ QByteArrayList parts;
222 for (const QByteArray &part : qAsConst(mParts)) { 222​ for (const QByteArray &part : qAsConst(mParts)) {
223 if (part.startsWith(AKONADI_PARAM_PLD)) { 223​ if (part.startsWith(AKONADI_PARAM_PLD)) {
224 parts << part.mid(4); 224​ parts << part.mid(4);
225 } 225​ }
226 } 226​ }
227 227​
228 QHash<qint64, QString> resourceIdNameCache; 228​ QHash<qint64, QString> resourceIdNameCache;
229 QVector<ItemRetrievalRequest *> requests; 229​ std::vector<std::unique_ptr<ItemRetrievalRequest>> requests;
230 QHash<qint64 /* collection */, ItemRetrievalRequest *> colRequests; 230​ QHash<qint64 /* collection */, ItemRetrievalRequest *> colRequests;
231 QHash<qint64 /* item */, ItemRetrievalRequest *> itemRequests; 231​ QHash<qint64 /* item */, ItemRetrievalRequest *> itemRequests;
232 QVector<qint64> readyItems; 232​ QVector<qint64> readyItems;
233 qint64 prevPimItemId = -1; 233​ qint64 prevPimItemId = -1;
234 QSet<QByteArray> availableParts; 234​ QSet<QByteArray> availableParts;
235 ItemRetrievalRequest *lastRequest = nullptr; 235​ ItemRetrievalRequest *lastRequest = nullptr;
236 while (query.isValid()) { 236​ while (query.isValid()) {
237 const qint64 pimItemId = query.value(PimItemIdColumn).toLongLong(); 237​ const qint64 pimItemId = query.value(PimItemIdColumn).toLongLong();
Show All 28 Lines254 } else {
266 prevPimItemId = pimItemId; 266​ prevPimItemId = pimItemId;
267 } 267​ }
268 268​
269 if (itemIter != itemRequests.constEnd()) { 269​ if (itemIter != itemRequests.constEnd()) {
270 lastRequest = *itemIter; 270​ lastRequest = *itemIter;
271 } else { 271​ } else {
272 lastRequest = colRequests.value(collectionId); 272​ lastRequest = colRequests.value(collectionId);
273 if (!lastRequest || lastRequest->ids.size() > 100) { 273​ if (!lastRequest || lastRequest->ids.size() > 100) {
274 lastRequest = new ItemRetrievalRequest(); 274​ lastRequest = new ItemRetrievalRequest();
dfaureAuthorUnsubmitted
Not Done

I don't agree because the "new" happens here.

dfaure: I don't agree because the "new" happens here.
275 lastRequest->ids.push_back(pimItemId); 275​ lastRequest->ids.push_back(pimItemId);
276 auto resIter = resourceIdNameCache.find(resourceId); 276​ auto resIter = resourceIdNameCache.find(resourceId);
277 if (resIter == resourceIdNameCache.end()) { 277​ if (resIter == resourceIdNameCache.end()) {
278 resIter = resourceIdNameCache.insert(resourceId, Resource::retrieveById(resourceId).name()); 278​ resIter = resourceIdNameCache.insert(resourceId, Resource::retrieveById(resourceId).name());
279 } 279​ }
280 lastRequest->resourceId = *resIter; 280​ lastRequest->resourceId = *resIter;
281 lastRequest->parts = parts; 281​ lastRequest->parts = parts;
282 colRequests.insert(collectionId, lastRequest); 282​ colRequests.insert(collectionId, lastRequest);
283 itemRequests.insert(pimItemId, lastRequest); 283​ itemRequests.insert(pimItemId, lastRequest);
284 requests << lastRequest; 284​ requests.emplace_back(lastRequest);
anthonyfieroniUnsubmitted
Done

emplace_back(lastRequest)

anthonyfieroni: `emplace_back(lastRequest)`
dfaureAuthorUnsubmitted
Not Done

Good suggestion, thanks.

dfaure: Good suggestion, thanks.
285 } else { 285​ } else {
286 lastRequest->ids.push_back(pimItemId); 286​ lastRequest->ids.push_back(pimItemId);
287 itemRequests.insert(pimItemId, lastRequest); 287​ itemRequests.insert(pimItemId, lastRequest);
288 } 288​ }
289 } 289​ }
290 290​
291 if (query.value(PartTypeNameColumn).isNull()) { 291​ if (query.value(PartTypeNameColumn).isNull()) {
292 // LEFT JOIN did not find anything, retrieve all parts 292​ // LEFT JOIN did not find anything, retrieve all parts
Show All 31 Lines
324 324​
325 if (!readyItems.isEmpty()) { 325​ if (!readyItems.isEmpty()) {
326 Q_EMIT itemsRetrieved(readyItems | toQList); 326​ Q_EMIT itemsRetrieved(readyItems | toQList);
327 } 327​ }
328 328​
329 QEventLoop eventLoop; 329​ QEventLoop eventLoop;
330 connect(ItemRetrievalManager::instance(), &ItemRetrievalManager::requestFinished, 330​ connect(ItemRetrievalManager::instance(), &ItemRetrievalManager::requestFinished,
331 this, [&](ItemRetrievalRequest *finishedRequest) { 331​ this, [&](ItemRetrievalRequest *finishedRequest) {
332 if (requests.removeOne(finishedRequest)) { 332​ const auto it = std::find_if(requests.begin(), requests.end(), [finishedRequest](const auto &ptr) {
333​ return ptr.get() == finishedRequest;
334​ });
335​ if (it != requests.end()) {
333 if (mCanceled) { 336​ if (mCanceled) {
337​ requests.erase(it); // deletes finishedRequest
anthonyfieroniUnsubmitted
Not Done

for (auto it = requests.begin(); it != requests.end(); ++it)
Will be better.

anthonyfieroni: `for (auto it = requests.begin(); it != requests.end(); ++it)` Will be better.
dfaureAuthorUnsubmitted
Not Done

A for loop better than find_if? What makes you say that?
I want to find one element and process it.
With a for loop I'd need to insert an if(), and a break... and it would mean mixing finding and processing. And the reader wouldn't be immediately sure that I'm only looking for one item, until finding the break much further down.

It's general consensus that using an algorithm is always better than writing a for loop.

dfaure: A `for` loop better than `find_if`? What makes you say that? I want to find one element and…
anthonyfieroniUnsubmitted
Not Done

I made a wrong assumption, i mean well readable

for (auto it = requests.begin(); it != requests.end(); ++it) {
    if (it->get() == finishedRequest) {
anthonyfieroni: I made a wrong assumption, i mean well readable ``` for (auto it = requests.begin(); it !=…
334 eventLoop.exit(1); 338​ eventLoop.exit(1);
335 } else if (!finishedRequest->errorMsg.isEmpty()) { 339​ } else if (!finishedRequest->errorMsg.isEmpty()) {
336 mLastError = finishedRequest->errorMsg.toUtf8(); 340​ mLastError = finishedRequest->errorMsg.toUtf8();
341​ requests.erase(it); // deletes finishedRequest
337 eventLoop.exit(1); 342​ eventLoop.exit(1);
338 } else { 343​ } else {
339 Q_EMIT itemsRetrieved(finishedRequest->ids); 344​ Q_EMIT itemsRetrieved(finishedRequest->ids);
340 if (requests.isEmpty()) { 345​ requests.erase(it); // deletes finishedRequest
346​ if (requests.empty()) {
341 eventLoop.quit(); 347​ eventLoop.quit();
342 } 348​ }
343 } 349​ }
344 } 350​ }
345 }, Qt::UniqueConnection); 351​ }, Qt::UniqueConnection);
346 352​
347 if (mConnection) { 353​ if (mConnection) {
348 connect(mConnection, &Connection::connectionClosing, 354​ connect(mConnection, &Connection::connectionClosing,
349 &eventLoop, [&eventLoop]() { eventLoop.exit(1); }); 355​ &eventLoop, [&eventLoop]() { eventLoop.exit(1); });
350 } 356​ }
351 357​
352 auto it = requests.begin(); 358​ auto it = requests.begin();
353 while (it != requests.end()) { 359​ while (it != requests.end()) {
354 auto request = (*it); 360​ auto request = (*it).get();
355 if ((!mFullPayload && request->parts.isEmpty()) || request->ids.isEmpty()) { 361​ if ((!mFullPayload && request->parts.isEmpty()) || request->ids.isEmpty()) {
356 it = requests.erase(it); 362​ it = requests.erase(it);
357 delete request; ​
358 continue; 363​ continue;
dfaureAuthorUnsubmitted
Not Done

... and some requests get deleted here.

dfaure: ... and some requests get deleted here.
359 } 364​ }
360 // TODO: how should we handle retrieval errors here? so far they have been ignored, 365​ // TODO: how should we handle retrieval errors here? so far they have been ignored,
361 // which makes sense in some cases, do we need a command parameter for this? 366​ // which makes sense in some cases, do we need a command parameter for this?
362 try { 367​ try {
363 // Request is deleted inside ItemRetrievalManager, so we need to take 368​ // Request is deleted inside ItemRetrievalManager, so we need to take
364 // a copy here 369​ // a copy here
365 //const auto ids = request->ids; 370​ //const auto ids = request->ids;
366 ItemRetrievalManager::instance()->requestItemDelivery(request); 371​ ItemRetrievalManager::instance()->requestItemDelivery(request);
367 } catch (const ItemRetrieverException &e) { 372​ } catch (const ItemRetrieverException &e) {
368 qCCritical(AKONADISERVER_LOG) << e.type() << ": " << e.what(); 373​ qCCritical(AKONADISERVER_LOG) << e.type() << ": " << e.what();
369 mLastError = e.what(); 374​ mLastError = e.what();
370 return false; 375​ return false;
371 } 376​ }
372 ++it; 377​ ++it;
373 } 378​ }
374 if (!requests.isEmpty()) { 379​ if (!requests.empty()) {
375 if (eventLoop.exec()) { 380​ if (eventLoop.exec()) {
376 return false; 381​ return false;
377 } 382​ }
378 } 383​ }
379 384​
380 // retrieve items in child collections if requested 385​ // retrieve items in child collections if requested
381 bool result = true; 386​ bool result = true;
382 if (mRecursive && mCollection.isValid()) { 387​ if (mRecursive && mCollection.isValid()) {
▲ Show 20 Lines • Show All 48 Lines • Show Last 20 Lines