Bug 138730

Summary: don't hardcode colors or play with the palette in ProcessWidget and derivatives
Product: [Applications] kdevelop Reporter: Matthew Woehlke <mwoehlke.floss>
Component: generalAssignee: kdevelop-bugs-null
Status: RESOLVED FIXED    
Severity: minor    
Priority: NOR    
Version: git master   
Target Milestone: ---   
Platform: Compiled Sources   
OS: Linux   
Latest Commit: Version Fixed In:
Sentry Crash Report:
Attachments: path to not play with palettes or use hard-coded (non-scheme) colors
patch to fix the patch

Description Matthew Woehlke 2006-12-13 00:50:16 UTC
Version:           3.3.9x (svn sync 2006-12-06) (using KDE KDE 3.5.5)
Installed from:    Compiled From Sources
Compiler:          gcc (GCC) 3.4.3 20050227 (Red Hat 3.4.3-22.1) 
OS:                Linux

ProcessWidget is tinkering with its palette, causing two undesirable effects:

1. The colors it chooses for highlighting are commonly indistinguishable from the window background (i.e. in any scheme where background() == mid(), e.g. '[Digital] CDE', 'Dark Blue', 'Solaris', 'High Contrast Black')

2. Styles that draw using the highlight color draw incorrectly.

Additionally, hard-coding of colors as done by several parts (grepview, to name one) is A Bad Thing because they can be less than legible (or even outright invisible) in some color schemes (for a REALLY nasty example, consider the effect of using any of the 'High Contrast *' schemes).
Comment 1 Matthew Woehlke 2006-12-13 00:58:55 UTC
Created attachment 18910 [details]
path to not play with palettes or use hard-coded (non-scheme) colors

Create a patch to do the following things:

- Don't play with the palette. Instead, use setCustomHighlighting(true); and
take over drawing the list item highlights so we don't confuse styles that use
the palette's highlight color.

- Don't use hard-coded colors (e.g. Qt::blue, Qt::darkGreen) for foreground
colors. Instead make use of QColorGroup::link[Visited]() and some color
blending to get more choices that will work well in the scheme. This changes
ProcessListBoxItem and GrepListBoxItem, which were the only two spots that I
could clearly identify as needing to be fixed.
Comment 2 Andreas Pakulat 2006-12-13 20:48:34 UTC
SVN commit 613289 by apaku:

Fix color highlighting so we don't confuse styles, do not use hardcoded colors.

Matthew, next time please use tabs instead of spaces if the original file does so ;)

BUG: 138730


 M  +56 -9     lib/widgets/processwidget.cpp  
 M  +4 -1      lib/widgets/processwidget.h  
 M  +28 -2     parts/grepview/grepviewwidget.cpp  


--- branches/kdevelop/3.4/lib/widgets/processwidget.cpp #613288:613289
@@ -31,7 +31,9 @@
 
 ProcessListBoxItem::ProcessListBoxItem(const QString &s, Type type)
     : QListBoxText(s), t(type)
-{}
+{
+    setCustomHighlighting(true);
+}
 
 
 bool ProcessListBoxItem::isCustomItem()
@@ -39,11 +41,58 @@
     return false;
 }
 
+static inline unsigned char normalize(int a)
+{
+    return (a < 0 ? 0 : a > 255 ? 255 : a);
+}
 
+static inline double blend1(double a, double b, double k)
+{
+    return a + (b - a) * k;
+}
+
+QColor ProcessListBoxItem::blend(const QColor &c1, const QColor &c2, double k) const
+{
+    if (k < 0.0) return c1;
+    if (k > 1.0) return c2;
+
+    int r = normalize((int)blend1((double)c1.red(),   (double)c2.red(),   k));
+    int g = normalize((int)blend1((double)c1.green(), (double)c2.green(), k));
+    int b = normalize((int)blend1((double)c1.blue(),  (double)c2.blue(),  k));
+
+    return QColor(qRgb(r, g, b));
+}
+
 void ProcessListBoxItem::paint(QPainter *p)
 {
-    p->setPen((t==Error)? Qt::darkRed :
-              (t==Diagnostic)? Qt::black : Qt::darkBlue);
+    QColor dim, warn, err, back;
+    if (listBox()) {
+        const QColorGroup& group = listBox()->palette().active();
+        if (isSelected()) {
+            back = group.button();
+            warn = group.buttonText();
+        }
+        else
+        {
+            back = group.base();
+            warn = group.text();
+        }
+        err = group.linkVisited();
+        dim = blend(warn, back);
+    }
+    else
+    {
+        warn = Qt::black;
+        dim = Qt::darkBlue;
+        err = Qt::darkRed;
+        if (isSelected())
+            back = Qt::lightGray;
+        else
+            back = Qt::white;
+    }
+    p->fillRect(p->window(), QBrush(back));
+    p->setPen((t==Error)? err :
+              (t==Diagnostic)? warn : dim);
     QListBoxText::paint(p);
 }
 
@@ -52,13 +101,11 @@
     : KListBox(parent, name)
 {
     setFocusPolicy(QWidget::NoFocus);
-    QPalette pal = palette();
-    pal.setColor(QColorGroup::HighlightedText,
-                 pal.color(QPalette::Normal, QColorGroup::Text));
-    pal.setColor(QColorGroup::Highlight,
-                 pal.color(QPalette::Normal, QColorGroup::Mid));
-    setPalette(pal);
 
+    // Don't override the palette, as that can mess up styles. Instead, draw
+    // the background ourselves (see ProcessListBoxItem::paint).
+
+
     childproc = new KProcess();
     childproc->setUseShell(true);
 
--- branches/kdevelop/3.4/lib/widgets/processwidget.h #613288:613289
@@ -41,7 +41,10 @@
     ProcessListBoxItem(const QString &s, Type type);
 
     virtual bool isCustomItem();
-    
+ 
+protected:
+    QColor blend(const QColor &c1, const QColor &c2, double k = 0.5) const;
+
 private:
     virtual void paint(QPainter *p);
     Type t;
--- branches/kdevelop/3.4/parts/grepview/grepviewwidget.cpp #613288:613289
@@ -69,19 +69,45 @@
 
 void GrepListBoxItem::paint(QPainter *p)
 {
+	QColor base, dim, result, bkground;
+	if (listBox()) {
+		const QColorGroup& group = listBox()->palette().active();
+		if (isSelected()) {
+			bkground = group.button();
+			base = group.buttonText();
+		}
+		else
+		{
+			bkground = group.base();
+			base = group.text();
+		}
+		dim = blend(base, bkground);
+		result = group.link();
+	}
+	else
+	{
+		base = Qt::black;
+		dim = Qt::darkGreen;
+		result = Qt::blue;
+		if (isSelected())
+			bkground = Qt::lightGray;
+		else
+			bkground = Qt::white;
+	}
 	QFontMetrics fm = p->fontMetrics();
 	QString stx = lineNumber + ":  ";
 	int y = fm.ascent()+fm.leading()/2;
 	int x = 3;
+	p->fillRect(p->window(), QBrush(bkground));
 	if (show)
 	{
-		p->setPen(Qt::darkGreen);
+		p->setPen(dim);
 		p->drawText(x, y, fileName);
 		x += fm.width(fileName);
 	}
 	else
 	{
-		p->setPen(Qt::black);
+		p->setPen(dim);
 		QFont font1(p->font());
 		QFont font2(font1);
 		font2.setBold(true);
Comment 3 Matthew Woehlke 2006-12-13 21:05:08 UTC
Aw, nuts, that was KWrite's fault; I hand-edited the patch to pull two comments that are (well, were until a moment ago) still in my local sources (which *does* use tabs) and I have 'replace tabs' turned on. Sorry about that! (Maybe if someone would fix Bug 115175 I would notice when I do that ;-).)

Anyway, thanks Andreas!
Comment 4 Matthew Woehlke 2006-12-19 21:12:54 UTC
Created attachment 18982 [details]
patch to fix the patch

grepviewwidget.cpp did not get patched correctly: attaching a new patch to fix
it. (Andreas, is that because you had to apply it by hand? I'm sorry, again! I
made sure the tabs are right this time, at least as long as kwrite isn't lying
to me :-).)
Comment 5 Matthew Woehlke 2006-12-19 21:13:24 UTC
Re-open in light of previous comment/attachment
Comment 6 Andreas Pakulat 2006-12-19 21:36:48 UTC
SVN commit 614999 by apaku:

Really fix the color-problems. 

Patch from Matthew Woelke, applied cleanly this time ;)

BUG: 138730


 M  +2 -2      grepviewwidget.cpp  


--- branches/kdevelop/3.4/parts/grepview/grepviewwidget.cpp #614998:614999
@@ -107,7 +107,7 @@
 	}
 	else
 	{
-		p->setPen(dim);
+		p->setPen(base);
 		QFont font1(p->font());
 		QFont font2(font1);
 		font2.setBold(true);
@@ -116,7 +116,7 @@
 		p->setFont(font1);
 		x += fm.width(stx);
 
-		p->setPen(Qt::blue);
+		p->setPen(result);
 		p->drawText(x, y, text);
 	}
 }