[KColorScheme/KStatefulBrush] Switch hardcoded numbers for enum items
ClosedPublic

Authored by ndavis on Mon, Dec 2, 7:04 AM.

Details

Summary

This also replaces some for-loops with C++11 range based for-loops and switches for simpler if/else control blocks.

Test Plan

Open colorscheme editor in the Colors KCM to see if any colors or effects look broken.

Diff Detail

Repository
R265 KConfigWidgets
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ndavis created this revision.Mon, Dec 2, 7:04 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptMon, Dec 2, 7:04 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
ndavis requested review of this revision.Mon, Dec 2, 7:04 AM
ngraham added a subscriber: ngraham.Mon, Dec 2, 4:08 PM

This also replaces some for-loops with C++11 range based for-loops and switches for simpler if/else control blocks.

Seems like these changes are unrelated and should maybe be in a separate commit?

ndavis added a comment.Mon, Dec 2, 4:32 PM

This also replaces some for-loops with C++11 range based for-loops and switches for simpler if/else control blocks.

Seems like these changes are unrelated and should maybe be in a separate commit?

The intention of the patch is to reduce the number of hardcoded values, so they're related. It wouldn't make sense to switch the hardcoded for-loop ranges for the enum values introduced in the parent patch only to replace them with range based for-loops immediately afterward.

dfaure requested changes to this revision.Mon, Dec 2, 10:07 PM

So many hardcoded numbers! Much better indeed.

src/kcolorscheme.cpp
88–90

coding style: space after for, and add { ... } around the body, with newlines.

397–400

coding style: if ( with a space

(repeats)

602

The & seems overkill (and confused me because it looks non-const).
This is just an enum, "copying" by value is faster than following the indirection of a reference.

This revision now requires changes to proceed.Mon, Dec 2, 10:07 PM
ndavis updated this revision to Diff 70802.Mon, Dec 2, 11:53 PM
  • [KColorScheme/KStatefulBrush] Switch hardcoded numbers for enum items
  • fix code style
ndavis marked 3 inline comments as done.Mon, Dec 2, 11:53 PM
dfaure accepted this revision.Tue, Dec 3, 8:29 AM
dfaure added inline comments.
src/kcolorscheme.cpp
289–291

(I would split this into 3 lines for more readability)

743

extra empty lines added at the end?

This revision is now accepted and ready to land.Tue, Dec 3, 8:29 AM
ndavis updated this revision to Diff 70812.Tue, Dec 3, 9:24 AM

More code formatting

ndavis marked 2 inline comments as done.Tue, Dec 3, 9:25 AM
This revision was automatically updated to reflect the committed changes.