Better use of Qt APIs in Plasma::Theme
ClosedPublic

Authored by apol on Jun 28 2019, 6:19 PM.

Details

Summary

Try not to access hashes repeatedly.
QRegExp -> QRegularExpression.

Diff Detail

Repository
R242 Plasma Framework (Library)
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 13385
Build 13403: arc lint + arc unit
apol created this revision.Jun 28 2019, 6:19 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptJun 28 2019, 6:19 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
apol requested review of this revision.Jun 28 2019, 6:19 PM
broulik added inline comments.
src/plasma/svg.cpp
313

When you make it static it would only have to compile it once

src/plasma/theme.cpp
106

Can we clean up all of this custom refcounting by using a QSharedPointer?

336

So this code removes the item if already existing and then inserts it again? This thing is a QHash so insert will just override the existing one anyway, no need to remove it first.

apol marked 3 inline comments as done.Jun 29 2019, 3:59 PM
apol added inline comments.
src/plasma/theme.cpp
106

Implemented with QSharedData.

apol updated this revision to Diff 60840.Jun 29 2019, 3:59 PM
apol marked an inline comment as done.

Address kai's comments

apol updated this revision to Diff 60875.Jun 30 2019, 4:39 PM

address logic, no artifacts

fvogt requested changes to this revision.Jul 1 2019, 2:10 PM
fvogt added a subscriber: fvogt.
fvogt added inline comments.
src/plasma/svg.cpp
317

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)

This revision now requires changes to proceed.Jul 1 2019, 2:10 PM
tcanabrava added inline comments.
src/plasma/svg.cpp
339–364

What's this doing? it seems that it calculates the bestFit just to discard later.

tcanabrava added inline comments.Jul 1 2019, 2:13 PM
src/plasma/svg.cpp
342–349

looks a good example of code that could be written with an std::find_if

apol added inline comments.Jul 1 2019, 2:27 PM
src/plasma/svg.cpp
317

Please note this is only to make sure the regex was properly compiled. It isn't matching there yet.

342–349

I'm not sure, this loop goes through all the size hints.

fvogt added inline comments.Jul 1 2019, 2:32 PM
src/plasma/svg.cpp
317

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.

bruns added a subscriber: bruns.Jul 1 2019, 2:38 PM
bruns added inline comments.
src/plasma/svg.cpp
342–349

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.

346

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 <).

apol marked 3 inline comments as done.Jul 1 2019, 2:39 PM
apol added inline comments.
src/plasma/svg.cpp
317

You're right, I looked at the wrong code. Fixed now.

apol updated this revision to Diff 60932.Jul 1 2019, 2:39 PM
apol marked an inline comment as done.

address fvogt's comment

fvogt resigned from this revision.Jul 1 2019, 2:45 PM
mart accepted this revision.Jul 2 2019, 2:44 PM
This revision is now accepted and ready to land.Jul 2 2019, 2:44 PM
This revision was automatically updated to reflect the committed changes.
ndavis added a subscriber: ndavis.EditedJul 4 2019, 7:28 AM

@apol This patch seems to have caused a bug in the way icons are loaded for the system tray:

The largest icon available from the desktop theme is used instead of the correct size.

When I compile plasma-framework without this patch, the system tray looks how it should:

broulik added inline comments.Jul 4 2019, 7:30 AM
src/plasma/svg.cpp
313

Oh, I think the static might be the culprit, I didn't realize the status and ratio were changing. Sorry

ndavis added a comment.Jul 4 2019, 3:48 PM

I've compiled this with the latest commits, but the problem hasn't gone away.

apol added a comment.Jul 4 2019, 11:02 PM

I've compiled this with the latest commits, but the problem hasn't gone away.

Try removing .cache/plasma-svgelements-default_v5.60.0 and .cache/plasma_theme_default_v5.60.0.kcache.

In D22147#491119, @apol wrote:

I've compiled this with the latest commits, but the problem hasn't gone away.

Try removing .cache/plasma-svgelements-default_v5.60.0 and .cache/plasma_theme_default_v5.60.0.kcache.

Already tried deleting my .kcache files, but D22275 fixed it