Fix width and separator of ToolButtonComplexControl outline w/ dropdown menu
AbandonedPublic

Authored by ndavis on Aug 15 2019, 8:43 AM.

Details

Summary

Apparently, the code to adjust the rectangle when a menu is present is what was causing the problem. Also added an if statement to move the separator with the button when sunken.

Test Plan

Old:


New:


Diff Detail

Repository
R31 Breeze
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ndavis created this revision.Aug 15 2019, 8:43 AM
Restricted Application added a project: Plasma. · View Herald TranscriptAug 15 2019, 8:43 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
ndavis requested review of this revision.Aug 15 2019, 8:43 AM
ndavis edited the test plan for this revision. (Show Details)Aug 15 2019, 8:46 AM
ndavis retitled this revision from Fix width of ToolButtonComplexControl outline w/ dropdown menu to Fix width and separator of ToolButtonComplexControl outline w/ dropdown menu.
ndavis edited the summary of this revision. (Show Details)
ndavis edited the test plan for this revision. (Show Details)
ndavis edited the summary of this revision. (Show Details)Aug 15 2019, 8:54 AM
ngraham accepted this revision.Aug 15 2019, 3:52 PM
ngraham added a subscriber: ngraham.

Thanks for fixing this annoying issue.

This revision is now accepted and ready to land.Aug 15 2019, 3:52 PM
This revision was automatically updated to reflect the committed changes.

Hi Noah
Thanks for the patch, however, it is not the right fix to the issue. If you use a light color scheme (like the default breeze), you will see that the shadow below the part of the button that corresponds to the arrow is darker than below the rest of the button. This is because the frame is actually rendered twice.

Now, the bug you try to fix is real, and as I was 100% sure that it was not there in the past, I used git bisect to track it down to this commit:

32d8b02880a237e6de415861500a018a5cd09781

The corresponding diff contains
@@ -5988,7 +5988,6 @@ namespace Breeze

// frame
if( toolButtonOption->subControls & SC_ToolButton )
{
  • copy.rect = buttonRect; if( inTabBar ) drawTabBarPanelButtonToolPrimitive( &copy, painter, widget ); else drawPrimitive( PE_PanelButtonTool, &copy, painter, widget); }

Which is what causes the issue.
Could you revert this commit, and push instead the proper fix that I will post in another comment ?

Alternatively, I can do it myself.
Note that your changes on the separator position is legit, but should be a different patch

hpereiradacosta reopened this revision.Fri, Aug 16, 3:50 PM

This revision is now accepted and ready to land.Fri, Aug 16, 3:50 PM
hpereiradacosta requested changes to this revision.Fri, Aug 16, 3:50 PM
This revision now requires changes to proceed.Fri, Aug 16, 3:50 PM

Zoom showing the issue mentionned above with the "current" patch (or master branch of breeze)

How it should look:

This comment was removed by ndavis.

Hi Noah
Thanks for the patch, however, it is not the right fix to the issue. If you use a light color scheme (like the default breeze), you will see that the shadow below the part of the button that corresponds to the arrow is darker than below the rest of the button. This is because the frame is actually rendered twice.

Now, the bug you try to fix is real, and as I was 100% sure that it was not there in the past, I used git bisect to track it down to this commit:

32d8b02880a237e6de415861500a018a5cd09781

The corresponding diff contains
@@ -5988,7 +5988,6 @@ namespace Breeze

// frame
if( toolButtonOption->subControls & SC_ToolButton )
{
  • copy.rect = buttonRect; if( inTabBar ) drawTabBarPanelButtonToolPrimitive( &copy, painter, widget ); else drawPrimitive( PE_PanelButtonTool, &copy, painter, widget); }

    Which is what causes the issue. Could you revert this commit, and push instead the proper fix that I will post in another comment ?

    Alternatively, I can do it myself. Note that your changes on the separator position is legit, but should be a different patch

Sure, thanks for the help.

ndavis abandoned this revision.Fri, Aug 16, 5:37 PM

Alright, problem fixed.