D26217: Add standard pen widths and replace hardcoded numbers

[Noah Davis]

This revision was automatically updated to reflect the committed changes.
Closed by commit R31:c16eb7a4614f: Add standard pen widths and replace 
hardcoded numbers (authored by ndavis).

REPOSITORY
  R31 Breeze

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26217?vs=72270=72271

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

AFFECTED FILES
  kstyle/breeze.h
  kstyle/breezehelper.cpp
  kstyle/breezehelper.h

To: ndavis, #breeze, #plasma, hpereiradacosta, ngraham
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26217: Add standard pen widths and replace hardcoded numbers

[Noah Davis]

ndavis updated this revision to Diff 72270.
ndavis marked 2 inline comments as done.
ndavis added a comment.


  Change symbol pen width back to 1.1

REPOSITORY
  R31 Breeze

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26217?vs=72223=72270

BRANCH
  replace-hardcoded (branched from master)

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

AFFECTED FILES
  kstyle/breeze.h
  kstyle/breezehelper.cpp
  kstyle/breezehelper.h

To: ndavis, #breeze, #plasma, hpereiradacosta, ngraham
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26217: Add standard pen widths and replace hardcoded numbers

[Noah Davis]

ndavis marked an inline comment as done.
ndavis added inline comments.

INLINE COMMENTS

> hpereiradacosta wrote in breeze.h:180
> For the record, //* (and /**) should also be caught by Doxygen (and hopefully 
> KDevelop). It is used more or less systematically in oxygen, so please use 
> this instead of ///

OK. KDevelop won't always detect comments that use other doxygen styles or 
display them correctly, but that's a KDevelop problem, not a Breeze code 
problem.

> hpereiradacosta wrote in breezehelper.cpp:1355
> Here the penwidth is actually changed (from 1.1 to 1.01) this could affect 
> the appearance of the actual buttons. Are you happy with the new appearance ? 
> Also, should doublecheck that this is consistent with button rendering in the 
> decoration.
> 
> In fact for the sake of changing only one thing at a time, I would set 
> penwidth::Symbol to 1.1

Yes, 1.01 works fine. 1.1 is slightly visible when scaling up the UI, so I 
changed it. I had planned to do the KDecoration after this patch. I will put 
the width change in a different commit though.

REPOSITORY
  R31 Breeze

BRANCH
  replace-hardcoded (branched from master)

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

To: ndavis, #breeze, #plasma, hpereiradacosta, ngraham
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26217: Add standard pen widths and replace hardcoded numbers

[Hugo Pereira Da Costa]

hpereiradacosta accepted this revision.
hpereiradacosta added a comment.


  looks good. See comment about the symbol pen width change and then ship it.
  
  In D26217#583358 , @ngraham wrote:
  
  > ...And after these patches land, let's do the doxygen-friendly comment 
formatting all at once in another patch.
  
  
  Most comments should already be doxygen friendly in breeze, except that //* 
and /** are used instead of ///

INLINE COMMENTS

> breeze.h:180
> + */
> +// The standard pen stroke width for symbols.
> +static constexpr qreal Symbol = 1.01;

For the record, //* (and /**) should also be caught by Doxygen (and hopefully 
KDevelop). It is used more or less systematically in oxygen, so please use this 
instead of ///

> breezehelper.cpp:1355
>  pen.setJoinStyle( Qt::MiterJoin );
> -pen.setWidthF( 1.1*qMax(1.0, 18.0/rect.width() ) );
> +pen.setWidthF( PenWidth::Symbol*qMax(1.0, 18.0/rect.width() ) );
>  painter->setPen( pen );

Here the penwidth is actually changed (from 1.1 to 1.01) this could affect the 
appearance of the actual buttons. Are you happy with the new appearance ? 
Also, should doublecheck that this is consistent with button rendering in the 
decoration.

In fact for the sake of changing only one thing at a time, I would set 
penwidth::Symbol to 1.1

REPOSITORY
  R31 Breeze

BRANCH
  replace-hardcoded (branched from master)

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

To: ndavis, #breeze, #plasma, hpereiradacosta, ngraham
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26217: Add standard pen widths and replace hardcoded numbers

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.
This revision is now accepted and ready to land.


  ...And after these patches land, let's do the doxygen-friendly comment 
formatting all at once in another patch.

REPOSITORY
  R31 Breeze

BRANCH
  replace-hardcoded (branched from master)

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

To: ndavis, #breeze, #plasma, hpereiradacosta, ngraham
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26217: Add standard pen widths and replace hardcoded numbers

[Noah Davis]

ndavis updated this revision to Diff 72223.
ndavis added a comment.


  Change comment formatting

REPOSITORY
  R31 Breeze

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26217?vs=72157=72223

BRANCH
  replace-hardcoded (branched from master)

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

AFFECTED FILES
  kstyle/breeze.h
  kstyle/breezehelper.cpp
  kstyle/breezehelper.h

To: ndavis, #breeze, #plasma, hpereiradacosta, ngraham
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26217: Add standard pen widths and replace hardcoded numbers

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> ndavis wrote in breeze.h:179
> It's a Doxygen style. If you do it, KDevelop shows the comments in the 
> tooltips for the commented functions, classes and variables.

Probably makes sense to do this universally all at once, rather than in little 
bits and pieces.

REPOSITORY
  R31 Breeze

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

To: ndavis, #breeze, #plasma, hpereiradacosta, ngraham
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26217: Add standard pen widths and replace hardcoded numbers

[Noah Davis]

ndavis added inline comments.

INLINE COMMENTS

> ngraham wrote in breeze.h:179
> Why three slashes for these comments?

It's a Doxygen style. If you do it, KDevelop shows the comments in the tooltips 
for the commented functions, classes and variables.

REPOSITORY
  R31 Breeze

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

To: ndavis, #breeze, #plasma, hpereiradacosta, ngraham
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26217: Add standard pen widths and replace hardcoded numbers

[Nathaniel Graham]

ngraham added a comment.


  +1, seems sensible to me, but probably wait for @hpereiradacosta's go-ahead.

INLINE COMMENTS

> breeze.h:179
> + */
> +/// The standard pen stroke width for symbols.
> +static constexpr qreal Symbol = 1.01;

Why three slashes for these comments?

REPOSITORY
  R31 Breeze

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

To: ndavis, #breeze, #plasma, hpereiradacosta, ngraham
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26217: Add standard pen widths and replace hardcoded numbers

[Noah Davis]

ndavis created this revision.
ndavis added reviewers: Breeze, Plasma.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
ndavis requested review of this revision.

REVISION SUMMARY
  The replaced numbers are directly related to the width of the pen.
  This patch likely doesn't replace all of the numbers that could be replaced. 
It just replaces the ones I'm currently certain of being related to pen width.
  The goal is to make the code show the intent of the designer.

REPOSITORY
  R31 Breeze

BRANCH
  replace-hardcoded (branched from master)

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

AFFECTED FILES
  kstyle/breeze.h
  kstyle/breezehelper.cpp
  kstyle/breezehelper.h

To: ndavis, #breeze, #plasma
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart