Rebased my old patch from reviewboard and tweaked it a bit.
Details
- Reviewers
hindenburg thsurrel tcanabrava - Group Reviewers
Konsole VDG - Commits
- R319:6fda6059bf04: Improve the display of search results
R319:5d55f916dc3c: Improve the display of search results
R319:77093eb82446: Improve the display of search results
R319:180fdaf3004f: Improve the display of search results
R319:7c3c8bedafb7: Improve the display of search results
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
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)
- 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)); } |
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.
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).
- 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).
I planned on doing that as well, but that needs a new class so I wanted to create a separate patch for that.
- 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.
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?
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.
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.
@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.