Differential D9167

Improve string handling
AbandonedPublic
Actions

Authored by apol on Dec 4 2017, 1:20 PM.

Details

Reviewers

None

Group Reviewers

Frameworks

Summary

Instead of using implicit casts, use the ones we need as suggested for KF5.

Test Plan

Builds, tests pass

Diff Detail

Repository

R302 KIconThemes

Branch

master

Lint

No Linters Available

Unit

No Unit Test Coverage

apol created this revision.Dec 4 2017, 1:20 PM

Restricted Application added a project: Frameworks. · View Herald TranscriptDec 4 2017, 1:20 PM

apol requested review of this revision.Dec 4 2017, 1:20 PM

Looks already pretty just. I just stumbled over two issues which maybe should be clarified - see comments.

src/kiconeffect.cpp
163	This should be: if (effectGroupState == Colorize \|\| effectGroupState == ToMonochrome) { or do I miss something?
165	Previously, this was not an else if(), but just an if(). Are you sure this works as intended?

Thanks!

src/kiconeffect.cpp
165	Yes, `effectGroupState` is not going to change values out of the blue.

:( when I rebased to current master I realized that @mlaurent worked on a very similar patch without going through review (which btw is policy in KDE Frameworks).

*pouts*

Revision Contents
Changeset List

			Path	Packages
M			CMakeLists.txt (3 lines)
M			autotests/kiconengine_unittest.cpp (2 lines)
M			autotests/kiconloader_benchmark.cpp (2 lines)
M			autotests/kiconloader_rcctest.cpp (10 lines)
M			autotests/kiconloader_resourcethemetest.cpp (2 lines)
M			autotests/kiconloader_unittest.cpp (40 lines)
M			src/kicondialog.cpp (10 lines)
M			src/kiconeffect.cpp (35 lines)
M			src/kiconloader.cpp (48 lines)
M			src/kicontheme.cpp (24 lines)
M			src/tools/kiconfinder/kiconfinder.cpp (2 lines)