D10480: align checkable widgets in menu items

This revision was automatically updated to reflect the committed changes. Closed by commit R31:f697b07340f3: align checkable widgets in menu items (authored by zzag). REPOSITORY R31 Breeze CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10480?vs=27344&id=27359 REVISION DETAIL https

D10480: align checkable widgets in menu items

hpereiradacosta accepted this revision. hpereiradacosta added a comment. This revision is now accepted and ready to land. Ship it ! Thanks ! REPOSITORY R31 Breeze BRANCH center-checkbox REVISION DETAIL https://phabricator.kde.org/D10480 To: zzag, #breeze, #vdg, hpereiradacosta Cc:

D10480: align checkable widgets in menu items

zzag edited the summary of this revision. REPOSITORY R31 Breeze REVISION DETAIL https://phabricator.kde.org/D10480 To: zzag, #breeze, #vdg, hpereiradacosta Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D10480: align checkable widgets in menu items

zzag edited the summary of this revision. REPOSITORY R31 Breeze REVISION DETAIL https://phabricator.kde.org/D10480 To: zzag, #breeze, #vdg, hpereiradacosta Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D10480: align checkable widgets in menu items

zzag updated this revision to Diff 27344. zzag added a comment. delete double space bug fix REPOSITORY R31 Breeze CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10480?vs=27116&id=27344 BRANCH center-checkbox REVISION DETAIL https://phabricator.kde.org/D10480 AFFECTED FILES

D10480: align checkable widgets in menu items

zzag reclaimed this revision. zzag added inline comments. INLINE COMMENTS > hpereiradacosta wrote in breezestyle.cpp:4740 > If I understand right, this is the double spacing bug fix. > Correct ? Very nice. > In principle, it would be better to have it in a separate Review, and a > separate com

D10480: align checkable widgets in menu items

hpereiradacosta added a comment. In D10480#207511 , @zzag wrote: > > Here: if you look at the last checkbox (folders first). The distance to the left edge is larger to that of the bottom edge, while before they were identical. > > Oh, I have

D10480: align checkable widgets in menu items

zzag added a comment. > Here: if you look at the last checkbox (folders first). The distance to the left edge is larger to that of the bottom edge, while before they were identical. Oh, I haven't noticed that before. To me, that's still fine. > So this really depends on what we want

D10480: align checkable widgets in menu items

hpereiradacosta added a comment. In D10480#206782 , @zzag wrote: > Well, I could rework this patch to fix double spacing. > > Centering of check boxes would involve some changes in margins, which should be another diff, right? > > > posit

D10480: align checkable widgets in menu items

zzag added a comment. Well, I could rework this patch to fix double spacing. Centering of check boxes would involve some changes in margins, which should another diff, right? > positioning appears off with respect to the bottom of the menu, when checkboxes is present on the last ite

D10480: align checkable widgets in menu items

hpereiradacosta added a comment. Hello, I do not think this revision should be abbandonned just yet. The part that takes care of removing double spaces when icon is not present (when no icon is present in the menu), should go in. The part about increasing the side margins to make the ch

D10480: align checkable widgets in menu items

zzag abandoned this revision. REPOSITORY R31 Breeze REVISION DETAIL https://phabricator.kde.org/D10480 To: zzag, #breeze, #vdg, hpereiradacosta Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D10480: align checkable widgets in menu items

zzag added a comment. > So 5 does makes sense: there is 1 for the frame, and then 4, that matches the spacing. Right ? Yes, I guess so. REPOSITORY R31 Breeze REVISION DETAIL https://phabricator.kde.org/D10480 To: zzag, #breeze, #vdg, hpereiradacosta Cc: plasma-devel, ZrenBot, progw

D10480: align checkable widgets in menu items

hpereiradacosta added a comment. > F5710580: Screenshot_20180214_002713.png > > Why are there only 2 pixels? Because the darker pixel, around the menu is also part of the menu, not the shadow. (meaning, there is a one pixel frame that is

D10480: align checkable widgets in menu items

zzag added a comment. F5710626: with-check.png F5710628: no-check.png F5710630: no-icons.png REPOSITORY R31 Breeze REVISION DETAIL https://phabricator.kde.org/D10

D10480: align checkable widgets in menu items

zzag updated this revision to Diff 27116. zzag added a comment. - try to align check boxes by increasing MenuItem_MarginWidth - fix another double space problem REPOSITORY R31 Breeze CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10480?vs=27072&id=27116 BRANCH center-checkbox

D10480: align checkable widgets in menu items

zzag added a comment. F5710509: a.png //Side note: This patch tries to center every checkbox between the left border and blue rect(and it also ignores margins.)// F5710580: Screenshot_20180214_002713.png

D10480: align checkable widgets in menu items

hpereiradacosta added a comment. In D10480#205772 , @zzag wrote: > > In principle it should not be needed. Right ? > > Yes. > > > Ifcheckboxes have a fixed size, and if margins would be equal to spacing, and if the math are correct, havin

D10480: align checkable widgets in menu items

zzag added a comment. > In principle it should not be needed. Right ? Yes. > Ifcheckboxes have a fixed size, and if margins would be equal to spacing, and if the math are correct, having something laied out as: margin + checkbox + spacing + icon + spacing + text would be automatical

D10480: align checkable widgets in menu items

hpereiradacosta added a comment. Ok. So, the patch works, but ... there is something fishy here. In principle it should not be needed. Right ? Ifcheckboxes have a fixed size, and if margins would be equal to spacing, and if the math are correct, having something laied out as: margin + chec

D10480: align checkable widgets in menu items

zzag updated this revision to Diff 27072. zzag added a comment. fix checkbox aligning when there is no any icon REPOSITORY R31 Breeze CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10480?vs=27062&id=27072 BRANCH center-checkbox REVISION DETAIL https://phabricator.kde.org/D10

D10480: align checkable widgets in menu items

zzag added a comment. > this is because you have to rely on maxIconWidth, even if the current item has no icon, when computing the checkbox space. I've came up with something like this if( showIcon && menuItemOption->maxIconWidth > 0 ) { int dx = (iconRect.left() - Metr

D10480: align checkable widgets in menu items

zzag marked 2 inline comments as done. zzag added inline comments. INLINE COMMENTS > hpereiradacosta wrote in breezestyle.cpp:4701 > why was this chunk of code moved ? This is unrelated to the change. > Please try keep the diff to the minimum To keep it consistent: 1. compute required rectangle

D10480: align checkable widgets in menu items

zzag updated this revision to Diff 27062. zzag added a comment. delete changes not related to this diff REPOSITORY R31 Breeze CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10480?vs=27051&id=27062 BRANCH center-checkbox REVISION DETAIL https://phabricator.kde.org/D10480 AFF

D10480: align checkable widgets in menu items

hpereiradacosta added a comment. Also, code breaks in the following configurations: F5709787: Screenshot_20180213_142858.png (see the incorrect centering of the radio button) this is because you have to rely on maxIconWidth, even if the current

D10480: align checkable widgets in menu items

hpereiradacosta added a comment. the diff appears more complex than it actually is because of unrelated changes. Please keep the changes to the minimum, this will help reviewing. INLINE COMMENTS > breeze.h:70 > Menu_FrameWidth = 0, > -MenuItem_MarginWidth = 3, > +Men

D10480: align checkable widgets in menu items

zzag edited the summary of this revision. REPOSITORY R31 Breeze REVISION DETAIL https://phabricator.kde.org/D10480 To: zzag, #breeze, #vdg, hpereiradacosta Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D10480: align checkable widgets in menu items

zzag created this revision. zzag added reviewers: Breeze, VDG, hpereiradacosta. Restricted Application added a project: Plasma. Restricted Application added a subscriber: plasma-devel. zzag requested review of this revision. REVISION SUMMARY Some icons in menus have small internal padding. In ad