D10438: reserve space for checkable widgets in menu items

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  In D10438#205519 , @zzag wrote:
  
  > Maybe an option like "Reserve space for check boxes" should be added in 
fine tuning settings?
  
  
  Please no option. If we put an option to every feature we are not sure about, 
then we end up with unmaintainable and cluttered code, and only offer a 
cluttered style to the user.
  Either we like this feature and it stays, or we dont and it goes.
  
  ... another screenshot: 
  F5710404: Screenshot_20180213_205409.png 


REPOSITORY
  R31 Breeze

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

To: zzag, #breeze, #vdg, ngraham, hpereiradacosta
Cc: alake, colomar, januz, fabianr, mmustac, abetts, anemeth, plasma-devel, 
ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D10438: reserve space for checkable widgets in menu items

[Vlad Zagorodniy]

zzag added a comment.


  Alright here's a patch which would make possible to turn off reserving space. 
Yet, a checkbox should be added in "Fine tuning" tab.
  
From 45be224b2fb31966a3117aa1f226f8ea711f8109 Mon Sep 17 00:00:00 2001
From: Vlad Zagorodniy 
Date: Tue, 13 Feb 2018 17:44:43 +0200
Subject: [PATCH] add reserve space from checkboxes option

---
 kstyle/breezestyle.cpp | 20 +---
 kstyle/breezestyle.h   |  3 +++
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/kstyle/breezestyle.cpp b/kstyle/breezestyle.cpp
index 07f0be7d..02189575 100644
--- a/kstyle/breezestyle.cpp
+++ b/kstyle/breezestyle.cpp
@@ -2728,7 +2728,11 @@ namespace Breeze
 leftColumnWidth += Metrics::MenuItem_ItemSpacing;
 
 // add checkbox indicator width
-leftColumnWidth += Metrics::CheckBox_Size + 
Metrics::MenuItem_ItemSpacing;
+if( menuItemOption->menuHasCheckableItems || 
reserveSpaceForCheckboxesInMenus() )
+{
+leftColumnWidth += Metrics::CheckBox_Size
++  Metrics::MenuItem_ItemSpacing;
+}
 
 // add spacing for accelerator
 /*
@@ -4697,8 +4701,11 @@ namespace Breeze
 // define relevant rectangles
 // checkbox
 QRect checkBoxRect;
-checkBoxRect = QRect( contentsRect.left(), contentsRect.top() + 
(contentsRect.height()-Metrics::CheckBox_Size)/2, Metrics::CheckBox_Size, 
Metrics::CheckBox_Size );
-contentsRect.setLeft( checkBoxRect.right() + 
Metrics::MenuItem_ItemSpacing + 1 );
+if( menuItemOption->menuHasCheckableItems || 
reserveSpaceForCheckboxesInMenus() )
+{
+checkBoxRect = QRect( contentsRect.left(), contentsRect.top() 
+ (contentsRect.height()-Metrics::CheckBox_Size)/2, Metrics::CheckBox_Size, 
Metrics::CheckBox_Size );
+contentsRect.setLeft( checkBoxRect.right() + 
Metrics::MenuItem_ItemSpacing + 1 );
+}
 
 // render checkbox indicator
 if( menuItemOption->checkType == 
QStyleOptionMenuItem::NonExclusive )
@@ -7074,6 +7081,13 @@ namespace Breeze
 return g.readEntry("ShowIconsInMenuItems", true);
 }
 
+//
+bool Style::reserveSpaceForCheckboxesInMenus() const
+{
+const KConfigGroup g(KSharedConfig::openConfig(), "KDE");
+return g.readEntry("ReserveSpaceForCheckboxesInMenus", true);
+}
+
 //
 bool Style::showIconsOnPushButtons() const
 {
diff --git a/kstyle/breezestyle.h b/kstyle/breezestyle.h
index 12548c92..031a1225 100644
--- a/kstyle/breezestyle.h
+++ b/kstyle/breezestyle.h
@@ -468,6 +468,9 @@ namespace Breeze
 //* return true if icons should be shown in menus
 bool showIconsInMenuItems() const;
 
+//* return true if space is reserved for checkboxes in menus
+bool reserveSpaceForCheckboxesInMenus() const;
+
 //* return true if icons should be shown on buttons
 bool showIconsOnPushButtons() const;
 
-- 
2.16.1
  
  But, I would like to have D10480  in 
master so I could complete the patch above.

REPOSITORY
  R31 Breeze

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

To: zzag, #breeze, #vdg, ngraham, hpereiradacosta
Cc: alake, colomar, januz, fabianr, mmustac, abetts, anemeth, plasma-devel, 
ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D10438: reserve space for checkable widgets in menu items

[Marijo Mustac]

mmustac added a comment.


  I am a user :-D
  To be honest I liked Vlad`s proposal after my first comment a lot. { 
margin-left: 10px; }
  The current implementation is nothing I would really like to have and be 
definitly a reason to search for a solution or another theme (which would 
propably be the easier part for most of the poeple, specially new ones).

REPOSITORY
  R31 Breeze

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

To: zzag, #breeze, #vdg, ngraham, hpereiradacosta
Cc: alake, colomar, januz, fabianr, mmustac, abetts, anemeth, plasma-devel, 
ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D10438: reserve space for checkable widgets in menu items

[Vlad Zagorodniy]

zzag added a comment.


  Maybe an option like "Reserve space for check boxes" should be added in fine 
tuning settings?

REPOSITORY
  R31 Breeze

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

To: zzag, #breeze, #vdg, ngraham, hpereiradacosta
Cc: alake, colomar, januz, fabianr, mmustac, abetts, anemeth, plasma-devel, 
ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D10438: reserve space for checkable widgets in menu items

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  In D10438#205505 , @ngraham wrote:
  
  > FWIW I like it, but since this is mostly subjective, I wouldn't fight to 
the death to retain if if everyone else suddenly hated it.
  
  
  ok. Maybe it is just a question to get used to it. 
  also we'll see if we get user feedback.

REPOSITORY
  R31 Breeze

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

To: zzag, #breeze, #vdg, ngraham, hpereiradacosta
Cc: alake, colomar, januz, fabianr, mmustac, abetts, anemeth, plasma-devel, 
ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D10438: reserve space for checkable widgets in menu items

[Nathaniel Graham]

ngraham added a comment.


  FWIW I like it, but since this is mostly subjective, I wouldn't fight to the 
death to retain if if everyone else suddenly hated it.

REPOSITORY
  R31 Breeze

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

To: zzag, #breeze, #vdg, ngraham, hpereiradacosta
Cc: alake, colomar, januz, fabianr, mmustac, abetts, anemeth, plasma-devel, 
ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D10438: reserve space for checkable widgets in menu items

[Hugo Pereira Da Costa]

hpereiradacosta added subscribers: colomar, alake.
hpereiradacosta added a comment.


  So ... I applied the patch (which I approved), used it, and ... don't like it 
sorry.
  See attached screenshots
  I think the unnecessary space breaks alignment with the menu above. and is 
completely unnecessary. Is this _really_ what we want ? @ngraham ? @alake ? 
@colomar
  
  F5709899: Screenshot_20180213_155307.png 

  
  F5709901: Screenshot_20180213_155334.png 


REPOSITORY
  R31 Breeze

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

To: zzag, #breeze, #vdg, ngraham, hpereiradacosta
Cc: alake, colomar, januz, fabianr, mmustac, abetts, anemeth, plasma-devel, 
ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D10438: reserve space for checkable widgets in menu items

[Vlad Zagorodniy]

This revision was automatically updated to reflect the committed changes.
Closed by commit R31:8989f8fd9c2c: reserve space for checkable widgets in menu 
items (authored by zzag).

REPOSITORY
  R31 Breeze

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10438?vs=27019=27055

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

AFFECTED FILES
  kstyle/breezestyle.cpp

To: zzag, #breeze, #vdg, ngraham, hpereiradacosta
Cc: januz, fabianr, mmustac, abetts, anemeth, plasma-devel, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D10438: reserve space for checkable widgets in menu items

[Vlad Zagorodniy]

zzag added a comment.


  > Not that I know. Also: I cannot reproduce. Here, if I uncheck "Show Icons 
in Menus" in system settings->widget style->Fine tuning, icons dissapear and 
shortcut are still there.
  
  Seems like it's only relevant to Arch Linux and Arch Linux-based distros. KDE 
neon doesn't have such issues.
  Qt version: 5.10
  
  > It would be greate if you could add a screenshot of the new context menu to 
https://community.kde.org/KDE_Visual_Design_Group/HIG/ContextMenu#Examples . Of 
cause other improvements/corrections to the page are more then welcome, too.
  
  Sure, I'll add later.

REPOSITORY
  R31 Breeze

BRANCH
  menuitem-margins

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

To: zzag, #breeze, #vdg, ngraham, hpereiradacosta
Cc: januz, fabianr, mmustac, abetts, anemeth, plasma-devel, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D10438: reserve space for checkable widgets in menu items

[Fabian Riethmayer]

fabianr added a comment.


  @zzag It would be greate if you could add a screenshot of the new context 
menu to 
https://community.kde.org/KDE_Visual_Design_Group/HIG/ContextMenu#Examples . Of 
cause other improvements/corrections to the page are more then welcome, too. If 
you hagve question you can ping me on IRC/Telegram in the vdg channel.

REPOSITORY
  R31 Breeze

BRANCH
  menuitem-margins

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

To: zzag, #breeze, #vdg, ngraham, hpereiradacosta
Cc: januz, fabianr, mmustac, abetts, anemeth, plasma-devel, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D10438: reserve space for checkable widgets in menu items

[Hugo Pereira Da Costa]

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


  In D10438#205230 , @zzag wrote:
  
  >
  
  
  
  
  > I'm not sure whether these changes should come with this diff or maybe I 
should create another diff which "fixes" alignment of check boxes. @ngraham, 
@hpereiradacosta, @abetts what do you think?
  
  This should go in a different patch.
  
  >  ---
  > 
  > Also, is there a bug report about missing shortcut hints when icons in 
menus are disabled?
  
  Not that I know. Also: I cannot reproduce. Here, if I uncheck "Show Icons in 
Menus" in system settings->widget style->Fine tuning, icons dissapear and 
shortcut are still there.
  Or are we talking about a different settings ?
  
  In any case, this change is ready to land. Thanks !

REPOSITORY
  R31 Breeze

BRANCH
  menuitem-margins

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

To: zzag, #breeze, #vdg, ngraham, hpereiradacosta
Cc: januz, fabianr, mmustac, abetts, anemeth, plasma-devel, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D10438: reserve space for checkable widgets in menu items

[Vlad Zagorodniy]

zzag added a comment.


  @anemeth here are menu items with centered check boxes and radio buttons:
  
  F5708483: aligned-1.png 
  
  F5708505: aligned-2.png 
  
  F5708523: aligned-4.png 
  
  it's still not perfectly aligned because some icons have inner padding. So, 
that's the best what I can do.
  
  Also, I've increased `MenuItem_MarginWidth` to `4`(according to @fabianr's 
comment).
  
  F5708489: aligned-3.png 
  
  That's all.
  
  ---
  
  I'm not sure whether these changes should come with this diff or maybe I 
should create another diff which "fixes" alignment of check boxes. @ngraham, 
@hpereiradacosta, @abetts what do you think?
  
  ---
  
  Also, is there a bug report about missing shortcut hints when icons in menus 
are disabled?

REPOSITORY
  R31 Breeze

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

To: zzag, #breeze, #vdg, ngraham, hpereiradacosta
Cc: januz, fabianr, mmustac, abetts, anemeth, plasma-devel, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D10438: reserve space for checkable widgets in menu items

[Vlad Zagorodniy]

zzag added a comment.


  In D10438#205139 , @anemeth wrote:
  
  > Maybe you could align the checkbox better?
  >  The numbers are pixels
  >  F5708060: a.png 
  
  
  I don't really think this diff should mess with margins. I'd would like to 
leave it as it is right now. Maybe another diff should be created regarding 
checkbox spacing.
  
  ---
  
  Explanation behind numbers:
  
  - 4: it should be 3(marginwidth) + 1(measurement error?)
  - 9: 4(itemspacing) + 1 + 2[(20 - 16) / 2] + 2(icons have padding)
  - 8: 2[(20 - 16) / 2] + 4(itemspacing) + 1 + 1(measurement error?)

REPOSITORY
  R31 Breeze

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

To: zzag, #breeze, #vdg, ngraham, hpereiradacosta
Cc: januz, fabianr, mmustac, abetts, anemeth, plasma-devel, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D10438: reserve space for checkable widgets in menu items

[Alex Nemeth]

anemeth added a comment.


  Maybe you could align the checkbox better?
  The numbers are pixels
  F5708060: a.png 

REPOSITORY
  R31 Breeze

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

To: zzag, #breeze, #vdg, ngraham, hpereiradacosta
Cc: januz, fabianr, mmustac, abetts, anemeth, plasma-devel, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D10438: reserve space for checkable widgets in menu items

[Vlad Zagorodniy]

zzag edited the summary of this revision.

REPOSITORY
  R31 Breeze

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

To: zzag, #breeze, #vdg, ngraham, hpereiradacosta
Cc: januz, fabianr, mmustac, abetts, anemeth, plasma-devel, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D10438: reserve space for checkable widgets in menu items

[Nathaniel Graham]

ngraham added a comment.


  +1 on this new approach.

REPOSITORY
  R31 Breeze

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

To: zzag, #breeze, #vdg, ngraham, hpereiradacosta
Cc: januz, fabianr, mmustac, abetts, anemeth, plasma-devel, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D10438: reserve space for checkable widgets in menu items

[Vlad Zagorodniy]

zzag added a comment.


  In D10438#205125 , @anemeth wrote:
  
  > Whoops, I misunderstood your prev comment.
  
  
  I've deleted changes in breeze.h and breezestyle.h on purpose.

REPOSITORY
  R31 Breeze

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

To: zzag, #breeze, #vdg, ngraham, hpereiradacosta
Cc: januz, fabianr, mmustac, abetts, anemeth, plasma-devel, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D10438: reserve space for checkable widgets in menu items

[Alex Nemeth]

anemeth added a comment.


  In D10438#205119 , @zzag wrote:
  
  > In D10438#205118 , @anemeth 
wrote:
  >
  > > @zzag Did you accidentally removed the changes in breeze.h and 
breezestyle.h? :)
  >
  >
  > Yes. There is no point to keep them.
  
  
  Whoops, I misunderstood your prev comment.
  I personally like this approach better than the previous.

REPOSITORY
  R31 Breeze

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

To: zzag, #breeze, #vdg, ngraham, hpereiradacosta
Cc: januz, fabianr, mmustac, abetts, anemeth, plasma-devel, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D10438: reserve space for checkable widgets in menu items

[Vlad Zagorodniy]

zzag added a comment.


  In D10438#205118 , @anemeth wrote:
  
  > @zzag Did you accidentally removed the changes in breeze.h and 
breezestyle.h? :)
  
  
  Yes. There is no point to keep them.

REPOSITORY
  R31 Breeze

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

To: zzag, #breeze, #vdg, ngraham, hpereiradacosta
Cc: januz, fabianr, mmustac, abetts, anemeth, plasma-devel, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D10438: reserve space for checkable widgets in menu items

[Vlad Zagorodniy]

zzag retitled this revision from "increase left/right margin of menuitems" to 
"reserve space for checkable widgets in menu items".
zzag edited the summary of this revision.

REPOSITORY
  R31 Breeze

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

To: zzag, #breeze, #vdg, ngraham, hpereiradacosta
Cc: januz, fabianr, mmustac, abetts, anemeth, plasma-devel, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart