New search result look, resurrected
AcceptedPublic

Authored by sandsmark on Dec 22 2018, 10:53 PM.

Details

Reviewers
hindenburg
thsurrel
tcanabrava
Group Reviewers
Konsole
VDG
Summary

Rebased my old patch from reviewboard and tweaked it a bit.

Test Plan

Searched with various colors schemes (white on black, linux colors, breeze, my own, etc.) and with different applications that do their own thing (mc).

Screenshots (because pictures are fun):

First my own color scheme:

Default breeze color scheme:

Black on white color scheme:

Linux colors color scheme with mc:

For reference, current search look with Linux colors in mc:

And with breeze:

Diff Detail

Repository
R319 Konsole
Lint
Lint Skipped
Unit
Unit Tests Skipped
sandsmark created this revision.Dec 22 2018, 10:53 PM
Restricted Application added a subscriber: konsole-devel. · View Herald TranscriptDec 22 2018, 10:53 PM
sandsmark requested review of this revision.Dec 22 2018, 10:53 PM
sandsmark edited the test plan for this revision. (Show Details)Dec 22 2018, 10:57 PM
tcanabrava accepted this revision.Dec 22 2018, 10:58 PM

Looks really nice and a really good improvement

This revision is now accepted and ready to land.Dec 22 2018, 10:58 PM
mglb added a subscriber: mglb.Dec 23 2018, 12:55 AM

Awesome! It was a thing I wanted to be made better. But there are some problems:

    • Invoke search, type text: all nice, I see current search line and remaining search results
    • Close search
    • Invoke search again: There is no current line (this is OK) and no dim (not OK) - search results are barely visible (without transparency they are not visible at all)
  1. Font size change (Ctrl+Scroll, Ctrl++, Ctrl+-) during search is not handled

Pedantic things:

  • Selecting search result removes the rounded rect's rounding

Ideas (maybe for future):

  • What do you think about making the lines 1px tall, drawn on inner edge of the rectangle? This would match lines in Breeze widget style.
  • Configurable colors, or colors taken from system theme/konsole theme
src/TerminalDisplay.cpp
1662

Lines are not drawn on a margin:


Also, the bottom line is 1px off. It needs to be translated down, like the bottom shaded area.

Fix:

@@ -1639,4 +1639,6 @@ void TerminalDisplay::paintEvent(QPaintEvent* pe)
         _searchResultRect.setTop(_searchResultRect.top() - _fontHeight / 3);
         _searchResultRect.setBottom(_searchResultRect.bottom() + _fontHeight / 3);
+        _searchResultRect.setLeft(0);
+        _searchResultRect.setRight(contentsRect().width());

         QColor shadeColor(0, 0, 0, 192);
@@ -1648,5 +1650,5 @@ void TerminalDisplay::paintEvent(QPaintEvent* pe)
         // Lightly shade line with results
         shadeColor.setAlpha(120);
-        paint.fillRect(0, _searchResultRect.top(), contentsRect().width(), _searchResultRect.height(), shadeColor);
+        paint.fillRect(_searchResultRect, shadeColor);
     } else {
         _searchResultRect = QRect();
@@ -1659,6 +1661,6 @@ void TerminalDisplay::paintEvent(QPaintEvent* pe)
     if (drawSearchResults) {
         paint.setPen(QPen(QColor(255, 255, 255, 64), 2));
-        paint.drawLine(QLineF(_searchResultRect.topLeft(), _searchResultRect.topRight()));
-        paint.drawLine(QLineF(_searchResultRect.bottomLeft(), _searchResultRect.bottomRight()));
+        paint.drawLine(QLine(_searchResultRect.topLeft(), _searchResultRect.topRight()));
+        paint.drawLine(QLine(_searchResultRect.bottomLeft(), _searchResultRect.bottomRight()).translated(0, 1));
     }
mglb added a reviewer: VDG.Dec 23 2018, 12:58 AM

Totally awesome! While you're at it, how about adding ticks to the scrollbar where there are search results? This would fix https://bugs.kde.org/show_bug.cgi?id=302284.

sandsmark updated this revision to Diff 48187.Dec 25 2018, 12:35 PM

fix the issues

In D17744#381034, @mglb wrote:
    • Invoke search, type text: all nice, I see current search line and remaining search results
    • Close search
    • Invoke search again: There is no current line (this is OK) and no dim (not OK) - search results are barely visible (without transparency they are not visible at all)

Fixed. But because we don't have any other way of indicating whether the search is active other than setting the current result line being valid I solved it by preserving the current search line (which makes more sense anyhow, imho).

  1. Font size change (Ctrl+Scroll, Ctrl++, Ctrl+-) during search is not handled

Not sure what you mean, it seems to work here.

Pedantic things:

  • Selecting search result removes the rounded rect's rounding

I explicitly didn't change this, to indicate clearly that it is selected, even when searching.

  • What do you think about making the lines 1px tall, drawn on inner edge of the rectangle? This would match lines in Breeze widget style.

Not entirely sure what you mean.

  • Configurable colors, or colors taken from system theme/konsole theme

Tested this a bit first, but it's hard to make it work with all kinds of different application (make menuconfig is what broke it for me, but mc etc. is also good at breaking this).

Totally awesome! While you're at it, how about adding ticks to the scrollbar where there are search results? This would fix https://bugs.kde.org/show_bug.cgi?id=302284.

I planned on doing that as well, but that needs a new class so I wanted to create a separate patch for that.

mglb added a comment.Dec 26 2018, 2:39 PM
  1. Font size change (Ctrl+Scroll, Ctrl++, Ctrl+-) during search is not handled

Not sure what you mean, it seems to work here.

Sorry, I've checked again and you are right. Not sure how it did happen before.

  • What do you think about making the lines 1px tall, drawn on inner edge of the rectangle? This would match lines in Breeze widget style.

Not entirely sure what you mean.

Code:

if (drawSearchResults) {
    paint.setPen(QPen(QColor(255, 255, 255, 64), 1));
    paint.drawLine(QLineF(_searchResultRect.topLeft(), _searchResultRect.topRight()).translated(0, 0.5));
    paint.drawLine(QLineF(_searchResultRect.bottomLeft(), _searchResultRect.bottomRight()).translated(0, 0.5));
}
  • Configurable colors, or colors taken from system theme/konsole theme

Tested this a bit first, but it's hard to make it work with all kinds of different application (make menuconfig is what broke it for me, but mc etc. is also good at breaking this).

What about something like this (uses Konsole color scheme)?

diff --git a/src/TerminalDisplay.cpp b/src/TerminalDisplay.cpp
index fb4ea1c4..b6bf1a4f 100644
--- a/src/TerminalDisplay.cpp
+++ b/src/TerminalDisplay.cpp
@@ -1639,5 +1639,6 @@ void TerminalDisplay::paintEvent(QPaintEvent* pe)


-        QColor shadeColor(0, 0, 0, 192);
+        QColor shadeColor(_colorTable[DEFAULT_BACK_COLOR]);
+        shadeColor.setAlpha(192);

         // Shade area above result
@@ -1646,5 +1647,5 @@ void TerminalDisplay::paintEvent(QPaintEvent* pe)
         paint.fillRect(0, _searchResultRect.bottom() + 1, contentsRect().width(), contentsRect().height(), shadeColor);
         // Lightly shade line with results
-        shadeColor.setAlpha(120);
+        shadeColor.setAlpha(128);
         paint.fillRect(_searchResultRect, shadeColor);
     } else {
@@ -1657,5 +1658,9 @@ void TerminalDisplay::paintEvent(QPaintEvent* pe)
     // from paintFilters because the lines potentially overlap some results
     if (drawSearchResults) {
-        paint.setPen(QPen(QColor(255, 255, 255, 64), 2));
+        CharacterColor ccFg(COLOR_SPACE_DEFAULT, DEFAULT_FORE_COLOR);
+        ccFg.setIntensive();
+        QColor fg = ccFg.color(_colorTable);
+        fg.setAlpha(96);
+        paint.setPen(QPen(fg, 2));
         paint.drawLine(QLine(_searchResultRect.topLeft(), _searchResultRect.topRight()));
         paint.drawLine(QLine(_searchResultRect.bottomLeft(), _searchResultRect.bottomRight()).translated(0, 1));
bug: first line is selected by default
  • Start Konsole
  • Ctrl+Shift+F

Result: first line is highlighted

bug: shade size does not reach bottom of a window
  • Start Konsole
  • Generate some text to have a few lines in history
  • Ctrl+Shift+F

Result: bottom dim rectangle is not drawn to the bottom of a window. When you scroll up, you'll see the first line is highlighted and the shade is under it. This also happen when you search for something and scroll down (so the result line is out of screen).

bug: margins area repaints wrong
  • set margins to make them visible
  • generate some text (to have something in history)
  • highlight any line
  • scroll a bit

Result: highlight on a margin is updated wrong.

Fix (horizontal lines changed to be drawn always on inner side of the rect):

diff --git a/src/TerminalDisplay.cpp b/src/TerminalDisplay.cpp
index fb4ea1c4..d548a62e 100644
--- a/src/TerminalDisplay.cpp
+++ b/src/TerminalDisplay.cpp
@@ -1386,2 +1386,12 @@ void TerminalDisplay::processFilters()

+inline QRect TerminalDisplay::calculateSearchResultRect() const
+{
+    const int searchLine = _screenWindow->currentResultLine() - _screenWindow->currentLine();
+    QRect resultRect = imageToWidget(QRect(0, searchLine, _columns, 1))
+                       .adjusted(0, -_fontHeight/3, 0, _fontHeight/3);
+    resultRect.setLeft(0);
+    resultRect.setRight(contentsRect().width());
+    return resultRect;
+}
+
 void TerminalDisplay::updateImage()
@@ -1559,4 +1569,3 @@ void TerminalDisplay::updateImage()
         // Highlight new result region
-        dirtyRegion |= QRect(0, _contentRect.top() + (_screenWindow->currentResultLine() - _screenWindow->currentLine()) * _fontHeight,
-                             _columns * _fontWidth, _fontHeight);
+        dirtyRegion |= calculateSearchResultRect();
     }
@@ -1632,9 +1641,3 @@ void TerminalDisplay::paintEvent(QPaintEvent* pe)
     if (drawSearchResults) {
-        const int searchLine = _screenWindow->currentResultLine() - _screenWindow->currentLine();
-        _searchResultRect = imageToWidget(QRect(0, searchLine, _columns, 1));
-        _searchResultRect.setTop(_searchResultRect.top() - _fontHeight / 3);
-        _searchResultRect.setBottom(_searchResultRect.bottom() + _fontHeight / 3);
-        _searchResultRect.setLeft(0);
-        _searchResultRect.setRight(contentsRect().width());
-
+        _searchResultRect = calculateSearchResultRect();

@@ -1659,4 +1662,5 @@ void TerminalDisplay::paintEvent(QPaintEvent* pe)
         paint.setPen(QPen(QColor(255, 255, 255, 64), 2));
-        paint.drawLine(QLine(_searchResultRect.topLeft(), _searchResultRect.topRight()));
-        paint.drawLine(QLine(_searchResultRect.bottomLeft(), _searchResultRect.bottomRight()).translated(0, 1));
+        const qreal lineOffset = paint.pen().width()/2.;
+        paint.drawLine(QLineF(_searchResultRect.topLeft(), _searchResultRect.topRight()).translated(0, lineOffset));
+        paint.drawLine(QLineF(_searchResultRect.bottomLeft(), _searchResultRect.bottomRight()).translated(0, 1 - lineOffset));
     }
diff --git a/src/TerminalDisplay.h b/src/TerminalDisplay.h
index 1b7dccc1..f22b8125 100644
--- a/src/TerminalDisplay.h
+++ b/src/TerminalDisplay.h
@@ -921,2 +921,4 @@ private:

+    QRect calculateSearchResultRect() const;
+
     // the window onto the terminal screen which this display
bug: highlight stays in same place when the screen content changes
  • start konsole
  • run mc
  • search for something; for best results highlight something between center and bottom of the window
  • click terminal to focus it
  • exit (f10)

Result: the highlight rectangle is still where it was.

sandsmark updated this revision to Diff 48449.Dec 31 2018, 3:52 PM

Apply patches from mglb, except the color one. Doesn't work well with my color scheme (way too low contrast), we can potentially fix that in a later patch.

Can't reproduce any of the issues now.

This looks rather bad on some of my color schemes. How do we handle where this looks bad w/ color schemes?

This looks rather bad on some of my color schemes. How do we handle where this looks bad w/ color schemes?

can you post the scheme or a screenshot? could make this configurable, though, if we can't find a nice universal solution.

How about just a checkbox to turn on/off the new "fancy" search look for now? I've tested with all the color schemes I have available, and I believe we handle it OK, but I could have missed some.

How about just a checkbox to turn on/off the new "fancy" search look for now? I've tested with all the color schemes I have available, and I believe we handle it OK, but I could have missed some.

I would support that. Apparently it looks really bad when Konsole is used in a GTK environment: https://bugs.kde.org/show_bug.cgi?id=393423. We might even want to go so far as to just automatically disable it in that case.

mglb added a comment.Mar 2 2019, 9:10 PM

@ngraham wrong review :)

@sandsmark I would like to modify colors/blending for it in the future (keeping all remaining code), so checkbox would be ok for now. Do we accept config options like "Use new $FEATURE (experimental)"? This would make it easier to remove the option when colors will work with everything.

Whoops, sorry!

In D17744#423497, @mglb wrote:

@ngraham wrong review :)

@sandsmark I would like to modify colors/blending for it in the future (keeping all remaining code), so checkbox would be ok for now. Do we accept config options like "Use new $FEATURE (experimental)"? This would make it easier to remove the option when colors will work with everything.

I don't think there's now, but could maybe add a separate "Experimental" tab to the config dialog? There's been a couple of issues now with semi-new stuff that could do with some wider testing before being enabled by default, I think that would be a good solution.