Invert shade button by same logic as keep-above button
ClosedPublic

Authored by andykluger on Jul 26 2018, 5:04 AM.

Details

Summary

What do say we invert the shade button when it's in a "checked" state, as we do the keep-above button?

The window decoration theme already inverts the keep-above button's colors when its state is activated, as a "loud" visual cue. The shade button similarly has an active state to indicate, but lacks such a loud cue to help avoid confusion. Depending on a user's other settings, there may be little or no visual indication (other than the nice but modest flip of the button) that a shaded window decoration doesn't belong to the window seen below it.

I'm attaching two screenshots of this patch applied, with a konsole window in both keep-above and shade states, directly over another window's titlebar.

Diff Detail

Repository
R31 Breeze
Branch
feature/shade-louder
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 1275
Build 1289: arc lint + arc unit
andykluger created this revision.Jul 26 2018, 5:04 AM
Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptJul 26 2018, 5:04 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
andykluger requested review of this revision.Jul 26 2018, 5:04 AM
ngraham requested changes to this revision.Jul 26 2018, 11:17 PM
ngraham added a subscriber: ngraham.

Hmm, the issue here is that unlike the Keep Above button, the Shade button actually changes its icon when used. So it's not so much a "shade enabled/shade disabled" button, as it is a "shade/unshade" button. Do you see the difference? One tracks state ("is shade mode enabled?"), the other one expresses distinct actions ("shade the window!" "un-shade the window!"). I would want that harmonized with the Keep Above button so they both behave in the same way first before accepting this patch, because otherwise people will file bugs saying "The Keep Above button doesn't change its icon when it's turned on the way the Shade button does!"

This revision now requires changes to proceed.Jul 26 2018, 11:17 PM

I see how the shade button flips while the keep-above doesn't, but I don't see the difference between toggling the shade state and shade/unshade, or toggling the keep-above state and keep-above/don't-keep-above. But I can't argue against consistency here.

The keep-above button tooltip already changes to "Don't keep above" when it's "checked" so I think it would be better to also flip the keep-above icon rather than stop flipping the shade icon.

I've made the changes in my branch but am new to Phabricator and could use some guidance. I created a new patch set with

git format-patch master --stdout

I clicked "Update Diff" on this Differential page and pasted it. But on the review-the-diff screen it only shows the first change, not both, which are in the patch. I'd like to submit this properly, but for now I'll include it here:

From 3fc62c74a8feb6e336daef0cd35325808eb2aa7c Mon Sep 17 00:00:00 2001
From: Andy Kluger <andykluger@gmail.com>
Date: Thu, 26 Jul 2018 00:04:57 -0400
Subject: [PATCH 1/2] invert colors for shade button whenever already doing so
 for keep-above

---
 kdecoration/breezebutton.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kdecoration/breezebutton.cpp b/kdecoration/breezebutton.cpp
index e2e043bc..e6bf2615 100644
--- a/kdecoration/breezebutton.cpp
+++ b/kdecoration/breezebutton.cpp
@@ -368,7 +368,7 @@ namespace Breeze
 
             return d->titleBarColor();
 
-        } else if( ( type() == DecorationButtonType::KeepBelow || type() == DecorationButtonType::KeepAbove ) && isChecked() ) {
+        } else if( ( type() == DecorationButtonType::KeepBelow || type() == DecorationButtonType::KeepAbove || type() == DecorationButtonType::Shade ) && isChecked() ) {
 
             return d->titleBarColor();
 
@@ -404,7 +404,7 @@ namespace Breeze
             if( type() == DecorationButtonType::Close ) return c->color( ColorGroup::Warning, ColorRole::Foreground );
             else return KColorUtils::mix( d->titleBarColor(), d->fontColor(), 0.3 );
 
-        } else if( ( type() == DecorationButtonType::KeepBelow || type() == DecorationButtonType::KeepAbove ) && isChecked() ) {
+        } else if( ( type() == DecorationButtonType::KeepBelow || type() == DecorationButtonType::KeepAbove || type() == DecorationButtonType::Shade ) && isChecked() ) {
 
             return d->fontColor();
 
-- 
2.18.0


From 008b5bff56036365e5c5be74dad481ced745571c Mon Sep 17 00:00:00 2001
From: Andy Kluger <andykluger@gmail.com>
Date: Fri, 27 Jul 2018 11:36:32 -0400
Subject: [PATCH 2/2] flip keep-above icon when checked

---
 kdecoration/breezebutton.cpp | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/kdecoration/breezebutton.cpp b/kdecoration/breezebutton.cpp
index e6bf2615..2932b8b7 100644
--- a/kdecoration/breezebutton.cpp
+++ b/kdecoration/breezebutton.cpp
@@ -310,15 +310,31 @@ namespace Breeze
 
                 case DecorationButtonType::KeepAbove:
                 {
-                    painter->drawPolyline( QPolygonF()
-                        << QPointF( 4, 9 )
-                        << QPointF( 9, 4 )
-                        << QPointF( 14, 9 ) );
+                    if (isChecked())
+                    {
 
-                    painter->drawPolyline( QPolygonF()
-                        << QPointF( 4, 13 )
-                        << QPointF( 9, 8 )
-                        << QPointF( 14, 13 ) );
+                        painter->drawPolyline( QPolygonF()
+                            << QPointF( 4, 4 )
+                            << QPointF( 9, 9 )
+                            << QPointF( 14, 4 ) );
+
+                        painter->drawPolyline( QPolygonF()
+                            << QPointF( 4, 8 )
+                            << QPointF( 9, 13 )
+                            << QPointF( 14, 8 ) );
+
+                    } else {
+
+                        painter->drawPolyline( QPolygonF()
+                            << QPointF( 4, 9 )
+                            << QPointF( 9, 4 )
+                            << QPointF( 14, 9 ) );
+
+                        painter->drawPolyline( QPolygonF()
+                            << QPointF( 4, 13 )
+                            << QPointF( 9, 8 )
+                            << QPointF( 14, 13 ) );
+                    }
                     break;
                 }
 
-- 
2.18.0
andykluger updated this revision to Diff 38617.Jul 27 2018, 4:30 PM

Flip keep-above icon when in checked state, for consistency with shade button

andykluger updated this revision to Diff 38618.Jul 27 2018, 4:38 PM

Invert shade button colors when in checked state; Vertically flip keep-above button in checked state

ngraham added a reviewer: VDG.Aug 5 2018, 4:51 AM

Sorry for the extended wait time!

Now the Keep Below button doesn't match the behavior of the Keep Above and Shade buttons, so we would need to apply it to that boo. But now that I see the latest change (that I requested) in action, I'm not sure I like the effect of reversing the icon for the Keep Above button. In its reversed state, it has the same icon as the Keep Below button, which I think could be confusing. I think I liked your original version better, sorry! If you revert to that, I'll give it a shot. I apologize for jerking you around like this when you've followed the right procedure and been very patient.

andykluger updated this revision to Diff 39228.Aug 6 2018, 7:41 PM
  • Revert "flip keep-above icon when checked"

Is it worth considering to stop vertically flipping the shade icon? I don't care either way, as the visual cue that provides is too subtle for me to notice anyway.

ngraham accepted this revision.Aug 20 2018, 8:27 PM

Thanks for your patience here. I'm fine with this, and I think it's a nice little improvement.

Any more VDG comments before I land this? If I don't hear any objections, I'll land it on 8/25/18, in 5 days.

This revision is now accepted and ready to land.Aug 20 2018, 8:27 PM
This revision was automatically updated to reflect the committed changes.

IMHO: you should either

  • keep the same icon, use the invert-color to set active state (meaning: remove the "unshade" icon
  • keep to different icons, and not invert-color (in which case the active state is driven by the drawn icon)

but not both.
Right now when I shade a window, I see an "unshade" icon, which appears as toggled. What am I suppose to expect when I click on it ? Will it "uncheck" the "unshade state" (meaning go to "shade") ? But then my window is shaded ... confusion.

This is a regression.

Note that I am fine with either solution (1, or 2). Not with the current which is the sum of the two.

Hugo
(former maintainer).

IMHO: you should either

  • keep the same icon, use the invert-color to set active state (meaning: remove the "unshade" icon
  • keep to different icons, and not invert-color (in which case the active state is driven by the drawn icon) but not both.

Yeah, that was my initial concern here too, but when @andykluger tried #2, I didn't like the result. Perhaps we should have tried #1 as well. The slight issue there is that the Shade button kind of looks similar to the Keep Below button if it doesn't change. I'd be fine with making the Shade button not change its appearance when toggled, but a clearer icon might help if we adopt that solution.

The icon is of lesser importance though. @andykluger, if you're still around, what do you think about submitting another patch to make the Shade button not change its icon when toggled?