dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed.
So many hardcoded numbers! Much better indeed. INLINE COMMENTS > kcolorscheme.cpp:88 > > - _effects[0] = 0; > - _effects[1] = 0; > - _effects[2] = 0; > + for(auto &effect : _effects) effect = 0; > coding style: space after `for`, and add `{` ... `}` around the body, with newlines. > kcolorscheme.cpp:393 > { > - switch (role) { > - case KColorScheme::AlternateBackground: > - return _brushes.bg[1]; > - case KColorScheme::ActiveBackground: > - return _brushes.bg[2]; > - case KColorScheme::LinkBackground: > - return _brushes.bg[3]; > - case KColorScheme::VisitedBackground: > - return _brushes.bg[4]; > - case KColorScheme::NegativeBackground: > - return _brushes.bg[5]; > - case KColorScheme::NeutralBackground: > - return _brushes.bg[6]; > - case KColorScheme::PositiveBackground: > - return _brushes.bg[7]; > - default: > - return _brushes.bg[0]; > + if(role >= KColorScheme::NormalBackground && role < > KColorScheme::NBackgroundRoles) { > + return _brushes.bg[role]; coding style: `if (` with a space (repeats) > kcolorscheme.cpp:598 > > - for (int i = 0; i < 3; i++) { > - QPalette::ColorGroup state = states[i]; > + for (auto &state : states) { > KColorScheme schemeView(state, KColorScheme::View, config); 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. REPOSITORY R265 KConfigWidgets REVISION DETAIL https://phabricator.kde.org/D25677 To: ndavis, #frameworks, dfaure Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns