D25677: [KColorScheme/KStatefulBrush] Switch hardcoded numbers for enum items

2019-12-03 Thread Noah Davis 
This revision was automatically updated to reflect the committed changes.
Closed by commit R265:45b6460b0c09: [KColorScheme/KStatefulBrush] Switch 
hardcoded numbers for enum items (authored by ndavis).

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25677?vs=70812=70813

REVISION DETAIL
  https://phabricator.kde.org/D25677

AFFECTED FILES
  src/kcolorscheme.cpp

To: ndavis, #frameworks, dfaure
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D25677: [KColorScheme/KStatefulBrush] Switch hardcoded numbers for enum items

2019-12-03 Thread Noah Davis 
ndavis marked 2 inline comments as done.

REPOSITORY
  R265 KConfigWidgets

BRANCH
  arcpatch-D25677 (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D25677

To: ndavis, #frameworks, dfaure
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D25677: [KColorScheme/KStatefulBrush] Switch hardcoded numbers for enum items

2019-12-03 Thread Noah Davis 
ndavis updated this revision to Diff 70812.
ndavis added a comment.


  More code formatting

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25677?vs=70802=70812

BRANCH
  arcpatch-D25677 (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D25677

AFFECTED FILES
  src/kcolorscheme.cpp

To: ndavis, #frameworks, dfaure
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D25677: [KColorScheme/KStatefulBrush] Switch hardcoded numbers for enum items

2019-12-03 Thread David Faure 
dfaure accepted this revision.
dfaure added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

> kcolorscheme.cpp:289
>  struct {
> -QBrush fg[8], bg[8], deco[2];
> +QBrush fg[KColorScheme::NForegroundRoles], 
> bg[KColorScheme::NBackgroundRoles], deco[KColorScheme::NDecorationRoles];
>  } _brushes;

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

> kcolorscheme.cpp:742
> +
> +

extra empty lines added at the end?

REPOSITORY
  R265 KConfigWidgets

BRANCH
  arcpatch-D25677 (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D25677

To: ndavis, #frameworks, dfaure
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D25677: [KColorScheme/KStatefulBrush] Switch hardcoded numbers for enum items

2019-12-02 Thread Noah Davis 
ndavis marked 3 inline comments as done.

REPOSITORY
  R265 KConfigWidgets

REVISION DETAIL
  https://phabricator.kde.org/D25677

To: ndavis, #frameworks, dfaure
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D25677: [KColorScheme/KStatefulBrush] Switch hardcoded numbers for enum items

2019-12-02 Thread Noah Davis 
ndavis updated this revision to Diff 70802.
ndavis added a comment.


  - [KColorScheme/KStatefulBrush] Switch hardcoded numbers for enum items
  - fix code style

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25677?vs=70720=70802

BRANCH
  arcpatch-D25677 (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D25677

AFFECTED FILES
  src/kcolorscheme.cpp

To: ndavis, #frameworks, dfaure
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D25677: [KColorScheme/KStatefulBrush] Switch hardcoded numbers for enum items

2019-12-02 Thread David Faure 
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  : _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  : 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

D25677: [KColorScheme/KStatefulBrush] Switch hardcoded numbers for enum items

2019-12-02 Thread Noah Davis 
ndavis added a comment.


  In D25677#570896 , @ngraham wrote:
  
  > > 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.

REPOSITORY
  R265 KConfigWidgets

REVISION DETAIL
  https://phabricator.kde.org/D25677

To: ndavis, #frameworks, dfaure
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D25677: [KColorScheme/KStatefulBrush] Switch hardcoded numbers for enum items

2019-12-02 Thread Nathaniel Graham 
ngraham added a comment.


  All right.

REPOSITORY
  R265 KConfigWidgets

REVISION DETAIL
  https://phabricator.kde.org/D25677

To: ndavis, #frameworks, dfaure
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D25677: [KColorScheme/KStatefulBrush] Switch hardcoded numbers for enum items

2019-12-02 Thread Nathaniel Graham 
ngraham added a comment.


  > 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?

REPOSITORY
  R265 KConfigWidgets

REVISION DETAIL
  https://phabricator.kde.org/D25677

To: ndavis, #frameworks, dfaure
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D25677: [KColorScheme/KStatefulBrush] Switch hardcoded numbers for enum items

2019-12-02 Thread Noah Davis 
ndavis added a reviewer: dfaure.

REPOSITORY
  R265 KConfigWidgets

REVISION DETAIL
  https://phabricator.kde.org/D25677

To: ndavis, #frameworks, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25677: [KColorScheme/KStatefulBrush] Switch hardcoded numbers for enum items

2019-12-01 Thread Noah Davis 
ndavis added a reviewer: Frameworks.

REPOSITORY
  R265 KConfigWidgets

REVISION DETAIL
  https://phabricator.kde.org/D25677

To: ndavis, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25677: [KColorScheme/KStatefulBrush] Switch hardcoded numbers for enum items

2019-12-01 Thread Noah Davis 
ndavis added a dependency: D25676: [KColorScheme] Add items to ColorSet and 
Role enums for the total number of items.

REPOSITORY
  R265 KConfigWidgets

REVISION DETAIL
  https://phabricator.kde.org/D25677

To: ndavis
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25677: [KColorScheme/KStatefulBrush] Switch hardcoded numbers for enum items

2019-12-01 Thread Noah Davis 
ndavis created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
ndavis requested review of this revision.

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

REPOSITORY
  R265 KConfigWidgets

BRANCH
  remove-hardcoded-values (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D25677

AFFECTED FILES
  src/kcolorscheme.cpp

To: ndavis
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns