Changeset View
Standalone View
src/plasma/svg.cpp
Show First 20 Lines • Show All 304 Lines • ▼ Show 20 Line(s) | |||||
305 | QPixmap SvgPrivate::findInCache(const QString &elementId, qreal ratio, const QSizeF &s) | 305 | QPixmap SvgPrivate::findInCache(const QString &elementId, qreal ratio, const QSizeF &s) | ||
306 | { | 306 | { | ||
307 | QSize size; | 307 | QSize size; | ||
308 | QString actualElementId; | 308 | QString actualElementId; | ||
309 | 309 | | |||
310 | if (elementsWithSizeHints.isEmpty()) { | 310 | if (elementsWithSizeHints.isEmpty()) { | ||
311 | // Fetch all size hinted element ids from the theme's rect cache | 311 | // Fetch all size hinted element ids from the theme's rect cache | ||
312 | // and store them locally. | 312 | // and store them locally. | ||
313 | QRegExp sizeHintedKeyExpr(CACHE_ID_NATURAL_SIZE(QStringLiteral("(\\d+)-(\\d+)-(.+)"), status, ratio)); | 313 | static const QRegularExpression sizeHintedKeyExpr(CACHE_ID_NATURAL_SIZE(QStringLiteral("$(\\d+)-(\\d+)-(.+)^"), status, ratio)); | ||
broulik: When you make it `static` it would only have to compile it once | |||||
Oh, I think the static might be the culprit, I didn't realize the status and ratio were changing. Sorry broulik: Oh, I think the `static` might be the culprit, I didn't realize the `status` and `ratio` were… | |||||
apol: Fixed https://commits.kde.org/plasma-framework/e3c6c2731eb272ca4f66e7836281992fb1f90e04 | |||||
314 | 314 | | |||
315 | foreach (const QString &key, cacheAndColorsTheme()->listCachedRectKeys(path)) { | 315 | foreach (const QString &key, cacheAndColorsTheme()->listCachedRectKeys(path)) { | ||
316 | if (sizeHintedKeyExpr.exactMatch(key)) { | 316 | const auto match = sizeHintedKeyExpr.match(key); | ||
317 | QString baseElementId = sizeHintedKeyExpr.cap(3); | 317 | if (match.isValid()) { | ||
isValid is always true, you probably want to use hasMatch instead. This is not obvious, I only noticed this because I debugged this error before (https://phabricator.kde.org/D17359) fvogt: `isValid` is always true, you probably want to use `hasMatch` instead.
This is not obvious, I… | |||||
Please note this is only to make sure the regex was properly compiled. It isn't matching there yet. apol: Please note this is only to make sure the regex was properly compiled. It isn't matching there… | |||||
It really does not look that way as you're immediately using captures after that. If that's really what you want (which I doubt), it should be sizeHintedKeyExpr.isValid() instead and be done before the foreach. Currently it would just add QSize(0, 0) to elementsWithSizeHints even for "öoiawze9pv5z2p3v4" as key. fvogt: It really does not look that way as you're immediately using captures after that.
If that's… | |||||
apol: You're right, I looked at the wrong code. Fixed now. | |||||
318 | QSize sizeHint(sizeHintedKeyExpr.cap(1).toInt(), | 318 | QString baseElementId = match.captured(3); | ||
319 | sizeHintedKeyExpr.cap(2).toInt()); | 319 | QSize sizeHint(match.capturedRef(1).toInt(), | ||
320 | match.capturedRef(2).toInt()); | ||||
320 | 321 | | |||
321 | if (sizeHint.isValid()) { | 322 | if (sizeHint.isValid()) { | ||
322 | elementsWithSizeHints.insertMulti(baseElementId, sizeHint); | 323 | elementsWithSizeHints.insertMulti(baseElementId, sizeHint); | ||
323 | } | 324 | } | ||
324 | } | 325 | } | ||
325 | } | 326 | } | ||
326 | 327 | | |||
327 | if (elementsWithSizeHints.isEmpty()) { | 328 | if (elementsWithSizeHints.isEmpty()) { | ||
328 | // Make sure we won't query the theme unnecessarily. | 329 | // Make sure we won't query the theme unnecessarily. | ||
329 | elementsWithSizeHints.insert(QString(), QSize()); | 330 | elementsWithSizeHints.insert(QString(), QSize()); | ||
330 | } | 331 | } | ||
331 | } | 332 | } | ||
332 | 333 | | |||
333 | // Look at the size hinted elements and try to find the smallest one with an | 334 | // Look at the size hinted elements and try to find the smallest one with an | ||
334 | // identical aspect ratio. | 335 | // identical aspect ratio. | ||
335 | if (s.isValid() && !elementId.isEmpty()) { | 336 | if (s.isValid() && !elementId.isEmpty()) { | ||
336 | QList<QSize> elementSizeHints = elementsWithSizeHints.values(elementId); | 337 | const QList<QSize> elementSizeHints = elementsWithSizeHints.values(elementId); | ||
337 | 338 | | |||
338 | if (!elementSizeHints.isEmpty()) { | 339 | if (!elementSizeHints.isEmpty()) { | ||
339 | QSize bestFit(-1, -1); | 340 | QSize bestFit(-1, -1); | ||
340 | 341 | | |||
341 | Q_FOREACH (QSize hint, elementSizeHints) { | 342 | for (const QSize &hint : elementSizeHints) { | ||
342 | 343 | | |||
343 | if (hint.width() >= s.width() * ratio && hint.height() >= s.height() * ratio && | 344 | if (hint.width() >= s.width() * ratio && hint.height() >= s.height() * ratio && | ||
344 | (!bestFit.isValid() || | 345 | (!bestFit.isValid() || | ||
345 | (bestFit.width() * bestFit.height()) > (hint.width() * hint.height()))) { | 346 | (bestFit.width() * bestFit.height()) > (hint.width() * hint.height()))) { | ||
can you swap bestFit and hint (and reverse the >) here, makes it easier to read IMHO (i.e. the comment says "smallest one", so the check should be <). bruns: can you swap bestFit and hint (and reverse the `>`) here, makes it easier to read IMHO (i.e. | |||||
346 | bestFit = hint; | 347 | bestFit = hint; | ||
347 | } | 348 | } | ||
348 | } | 349 | } | ||
looks a good example of code that could be written with an std::find_if tcanabrava: looks a good example of code that could be written with an std::find_if | |||||
You either have to sort on bestFit.area() first, or do the find_if in a nested loop, to find a better match (smaller one) after the first one. bruns: You either have to sort on `bestFit.area()` first, or do the find_if in a nested loop, to find… | |||||
apol: I'm not sure, this loop goes through all the size hints. | |||||
349 | 350 | | |||
350 | if (bestFit.isValid()) { | 351 | if (bestFit.isValid()) { | ||
351 | actualElementId = QString::number(bestFit.width()) % QLatin1Char('-') % | 352 | actualElementId = QString::number(bestFit.width()) % QLatin1Char('-') % | ||
352 | QString::number(bestFit.height()) % QLatin1Char('-') % elementId; | 353 | QString::number(bestFit.height()) % QLatin1Char('-') % elementId; | ||
353 | } | 354 | } | ||
354 | } | 355 | } | ||
355 | } | 356 | } | ||
356 | 357 | | |||
357 | if (elementId.isEmpty() || !q->hasElement(actualElementId)) { | 358 | if (elementId.isEmpty() || !q->hasElement(actualElementId)) { | ||
358 | actualElementId = elementId; | 359 | actualElementId = elementId; | ||
359 | } | 360 | } | ||
360 | 361 | | |||
361 | if (elementId.isEmpty() || (multipleImages && s.isValid())) { | 362 | if (elementId.isEmpty() || (multipleImages && s.isValid())) { | ||
362 | size = s.toSize() * ratio; | 363 | size = s.toSize() * ratio; | ||
363 | } else { | 364 | } else { | ||
What's this doing? it seems that it calculates the bestFit just to discard later. tcanabrava: What's this doing? it seems that it calculates the bestFit just to discard later. | |||||
364 | size = elementRect(actualElementId).size().toSize() * ratio; | 365 | size = elementRect(actualElementId).size().toSize() * ratio; | ||
365 | } | 366 | } | ||
366 | 367 | | |||
367 | if (size.isEmpty()) { | 368 | if (size.isEmpty()) { | ||
368 | return QPixmap(); | 369 | return QPixmap(); | ||
369 | } | 370 | } | ||
370 | 371 | | |||
371 | QString id = cachePath(path, size); | 372 | const QString id = cachePath(path, size) + actualElementId; | ||
372 | | ||||
373 | if (!actualElementId.isEmpty()) { | | |||
374 | id.append(actualElementId); | | |||
375 | } | | |||
376 | 373 | | |||
377 | //qCDebug(LOG_PLASMA) << "id is " << id; | 374 | //qCDebug(LOG_PLASMA) << "id is " << id; | ||
378 | 375 | | |||
379 | QPixmap p; | 376 | QPixmap p; | ||
380 | if (cacheRendering && cacheAndColorsTheme()->findInCache(id, p, lastModified)) { | 377 | if (cacheRendering && cacheAndColorsTheme()->findInCache(id, p, lastModified)) { | ||
381 | p.setDevicePixelRatio(ratio); | 378 | p.setDevicePixelRatio(ratio); | ||
382 | //qCDebug(LOG_PLASMA) << "found cached version of " << id << p.size(); | 379 | //qCDebug(LOG_PLASMA) << "found cached version of " << id << p.size(); | ||
383 | return p; | 380 | return p; | ||
▲ Show 20 Lines • Show All 624 Lines • Show Last 20 Lines |
When you make it static it would only have to compile it once