Bug 435309

Created attachment 137308 [details]
visible selection with 3b06b6f22 reverted

Hmm, this *might* dupe bug #433512, which was filed in late February after the commit in question on Feb 10, but that bug lists OpenSuSE Tumbleweed with plasma 5.21.80 and frameworks 5.80.0 but no konsole version and not specifying git, so was his version before or after that commit.  Additionally, that bug doesn't mention whether it's a regression or not.  Would Tumbleweed have that commit in the two weeks between it and the filing?  And the contrast in the screenshot there isn't anywhere /near/ as bad -- I can /see/ the selection there.  In my screenshot, it's all but impossible to see the selection, certainly at a glance.  So I'm not sure if it's just the pre-commit normal state, or if it's the same bug.

Created attachment 137311 [details]
attachment-14138-0.html

Please use search for markers
I understand and agree that this is a bug that needs to be fixed, but don’t
use text selection to mark the spot where you are.

Text search is better suited for that - or, we can create a way to actually
use text markers with different colors in the future.


On Sat, 3 Apr 2021 at 12:14 Duncan <bugzilla_noreply@kde.org> wrote:

> https://bugs.kde.org/show_bug.cgi?id=435309
>
>             Bug ID: 435309
>            Summary: 3b06b6f22 makes selection near invisible with some
>                     foreground/background color combinations
>            Product: konsole
>            Version: master
>           Platform: Gentoo Packages
>                 OS: Linux
>             Status: REPORTED
>           Severity: normal
>           Priority: NOR
>          Component: general
>           Assignee: konsole-devel@kde.org
>           Reporter: 1i5t5.duncan@cox.net
>   Target Milestone: ---
>
> Created attachment 137307 [details]
>   --> https://bugs.kde.org/attachment.cgi?id=137307&action=edit
> near-invisible selection
>
> See the attached screenshots.
>
> Unfortunately, after 3b06b6f22 (blend foreground/background instead of
> using
> reverse video for selection), with some foreground/background combinations
> the
> selection is now very close to invisible.
>
> Reasonably standard Linux color-scheme here, so normally black background
> with
> white text, which is fine.  But take a look at the attached gentoo portage
> package manager screenshots (the tail end of an emerge --ask --empty-tree
> konsole).  Without that commit reverted the color-coded white text on blue
> background of the package names makes the selection near invisible.
>
> The problem is that I tend to use selection as a way to mark my place, say
> when
> I have the kde live-git package update list displayed in one konsole
> window,
> while checking git logs for various packages in another konsole window.
> When
> there's well over a screen full of package updates, I'll
> double-click-select a
> package near where I am in the list so I can easily find it and pick out
> the
> next git log I want to run.
>
> Obviously, if I'm using selection as a marker and I can't see where that
> selection-marker is at a glance, it rather defeats the purpose.
>
> --
> You are receiving this mail because:
> You are the assignee for the bug.

(In reply to tcanabrava from comment #3)
> Please use search for markers
> I understand and agree that this is a bug that needs to be fixed, but don’t
> use text selection to mark the spot where you are.

No disrespect intended as I'm sure none was meant, but maybe a bit more context...

Dawn of the millennium.  Scroll-wheel-mice have been a thing for some years.  I'm sitting there on Windows98, one hand alternating between chips and a beverage of choice, the other on the mouse as I read yet another article on Linux, which I ultimately make my operating system of choice for the new century as the best alternative to installing what after doing my research I had come to call eXPrivacy due to the then new MS authorization techniques.

I'm absent-mindedly quick-double-click-select-marking/reading/scrolling/selecting/reading my way down the article, my mind on Linux...

That quick-double-click-select-mark usage has lasted since well before the turn of the century thru countless mice/pointing-devices, two major operating systems and multiple versions/distros of each, across browsers, news and email clients, desktop-environments, switching Windows->xfree86->xorg->wayland, down to the present, where I'm still using it in konsole to scroll thru thousands of lines of kernel git logs or (as above) hundreds of live-git-update packages.

The OSs/GUI-platforms/desktops/environments/apps and now hopefully temporarily necessary commit-revert patches to apps change, while the select-mark/scroll/read/select-mark usage, endures.  Why would I complicate it by adding repeatedly typing one-shot-find text into an otherwise read/select/scroll-only window in ordered to update otherwise-random-text one-shot markers, when a simple double-click and scroll serves the purpose?

Oh, and by the way (shaking my cane), get off my grass!  I'm feeling old-and-set-in-my-ways now.  Or is the term "experienced"? =:^)

*** Bug 436154 has been marked as a duplicate of this bug. ***

Should be fixed by https://invent.kde.org/utilities/konsole/-/merge_requests/380

(In reply to Ahmad Samir from comment #6)
> Should be fixed by
> https://invent.kde.org/utilities/konsole/-/merge_requests/380

Works for me! =:^)

*** Bug 433512 has been marked as a duplicate of this bug. ***

Git commit 9f7cb32dc23246783d5a72164a37e82f4268bc58 by Kurt Hindenburg, on behalf of Jan Blackquill.
Committed on 26/04/2021 at 13:33.
Pushed by hindenburg into branch 'master'.

Use inverted colours when calculated fancy BG has too low contrast

M  +17   -4    src/terminalDisplay/TerminalPainter.cpp

https://invent.kde.org/utilities/konsole/commit/9f7cb32dc23246783d5a72164a37e82f4268bc58

Thanks everyone participating in this bug hunt :)

FWIW, I do exactly the same as Duncan, and search is not an option for me because the text I select as a "marker" might be repeated in the output (and I like to know at a glance the exact place I was in, search would complicate that).

Git commit 2b441030a745a17f76d8f010a299a74532e3f211 by Kurt Hindenburg, on behalf of Jan Blackquill.
Committed on 27/04/2021 at 12:35.
Pushed by hindenburg into branch 'release/21.04'.

Use inverted colours when calculated fancy BG has too low contrast
(cherry picked from commit 9f7cb32dc23246783d5a72164a37e82f4268bc58)

M  +17   -4    src/terminalDisplay/TerminalPainter.cpp

https://invent.kde.org/utilities/konsole/commit/2b441030a745a17f76d8f010a299a74532e3f211

With some color schemes - such as Dark Pastels - this patch doesn't make any difference. The selection is still hard to see.

*** Bug 436320 has been marked as a duplicate of this bug. ***

*** Bug 436332 has been marked as a duplicate of this bug. ***

(In reply to Antonio Rojas from comment #12)
> With some color schemes - such as Dark Pastels - this patch doesn't make any
> difference. The selection is still hard to see.
+1

Instead of trying to fix the new behavior, can we get the old one? Reverse video for highlighted text was working and everyone was used to it.

(In reply to Duncan from comment #7)
> (In reply to Ahmad Samir from comment #6)
> > Should be fixed by
> > https://invent.kde.org/utilities/konsole/-/merge_requests/380
> Works for me! =:^)

Just upgraded to gcc-11.1.0 (from 10.3.0) and it no-longer works. =:^(  See below.

(In reply to Paulo from comment #15)
> (In reply to Antonio Rojas from comment #12)
> > With some color schemes - such as Dark Pastels - this patch doesn't make any
> > difference. The selection is still hard to see.
> +1
> 
> Instead of trying to fix the new behavior, can we get the old one? Reverse
> video for highlighted text was working and everyone was used to it.

While the new colors (if not the code) still works for me, given the facts that it's not working for everyone, that a revert to the old reversing behavior would be find by me as well, and that all the related bugs are now duped to this one, I'll go ahead and reopen this to report the new code error rather than opening a new bug as I normally would since the aspect I personally am experiencing arguably /is/ a new bug.

FWIW I'll update the invent thread with the new gcc-11.1.0 error as well.

Again, this error is what I'm getting on the new gcc-11.1.0 I just installed, while switching back to 10.3.0 to double-check verifies 10.3.0 is still building the code without error:

FAILED: src/CMakeFiles/konsoleprivate.dir/terminalDisplay/TerminalPainter.cpp.o
[snip the big long commandline]
../konsole-9999/src/terminalDisplay/TerminalPainter.cpp:381:10: error: 'optional' in namespace 'std' does not name a template type
381 |     std::optional<QColor> calculateBackgroundColor(const Character* style, const QColor *colorTable)
|          ^~~~~~~~
../konsole-9999/src/terminalDisplay/TerminalPainter.cpp:32:1: note: 'std::optional' is defined in header '<optional>'; did you forget to '#include <optional>'?
31 | #include <QtMath>
+++ |+#include <optional>
32 | 
../konsole-9999/src/terminalDisplay/TerminalPainter.cpp: In member function 'void Konsole::TerminalPainter::drawTextFragment(QPainter&, const QRect&, const QString&, const Konsole::Character*, const QColor*, Konsole::LineProperty)':
../konsole-9999/src/terminalDisplay/TerminalPainter.cpp:413:40: error: 'calculateBackgroundColor' was not declared in this scope
413 |         const QColor backgroundColor = calculateBackgroundColor(style, colorTable).value_or(foregroundColor);
|                                        ^~~~~~~~~~~~~~~~~~~~~~~~
ninja: build stopped: subcommand failed.


FWIW reverting to 7c5f08a2e, immediately before the 3b0 commit that killed reverse-select, builds (and runs) with gcc-11.1.0 too.  There's enough commits stacked on top of the target reversion commit now that it's getting a bit more complex to revert the individual commits (at least for a quick test), but reverting to that commit as head still works even if it's losing some other work by now as well.

Running this for now:

diff --git a/src/Screen.cpp b/src/Screen.cpp
index 54634d6..8d4c1b5 100644
--- a/src/Screen.cpp
+++ b/src/Screen.cpp
@@ -620,7 +620,7 @@ void Screen::copyFromHistory(Character* dest, int startLine, int count) const
         if (_selBegin != -1) {
             for (int column = 0; column < _columns; column++) {
                 if (isSelected(column, line)) {
-                    dest[destLineOffset + column].rendition |= RE_SELECTED;
+                    reverseRendition(dest[destLineOffset + column]);
                 }
             }
         }
@@ -643,7 +643,7 @@ void Screen::copyFromScreen(Character* dest , int startLine , int count) const
 
             // invert selected text
             if (_selBegin != -1 && isSelected(column, line + _history->getLines())) {
-                dest[destIndex].rendition |= RE_SELECTED;
+                reverseRendition(dest[destIndex]);
             }
         }
     }

(In reply to Duncan from comment #16)
> Just upgraded to gcc-11.1.0 (from 10.3.0) and it no-longer works. =:^(

The gcc-11 bug is fixed in live-git now, but I'm keeping the bug open because the original contrast bug remains for some people.

I just downloaded and built the konsole-master (output of 'konsole --version' is 21.07.70) with gcc 10.3.0, not 11, and konsole built ok.
Reverse video for selected text is working again as before for 'Linux colors' color scheme :D thank you very much.
But Dark Pastels still doesn't have reverse video for selected text.

I'm in favor of returning the old behavior.

It looks like we'll either have to revert the whole change or add (yet) another option to the settings dialog.

reversed colors always work, why change?

(In reply to Roberto from comment #22)
> reversed colors always work, why change?

Good question.

For those reading the bug who don't follow git closely enough to have seen the original git commit notes, they said:

>> Improve appearance of text selection
>>
>> By blending foreground & background instead of swapping them,
>> you can get a more visually appealing text selection.

But "more visually appealing" really isn't if it's unreadable, and as we're demonstrating, blending instead of swapping, by definition, is going to lower that contrast...

Please return to the original behavior. My background is #000000, my foreground is #ffffff and the new behavior forces me to squint all the time.

For now, my only recourse is to add "return {};" to the top of "std::optional<QColor> calculateBackgroundColor(const Character* style, const QColor *colorTable)" in src/terminalDisplay/TerminalPainter.cpp.

Since 21.04.1 (Gentoo hardened KDE) I'm faced with a situation that I cannot any longer copy+paste a command via mouse including the \n.

Said that I do now have to press "ENTER" for each commend line I copy+paste into the KOnsole :-/

(In reply to Toralf Förster from comment #25)
> Since 21.04.1 (Gentoo hardened KDE) I'm faced with a situation that I cannot
> any longer copy+paste a command via mouse including the \n.
> 
> Said that I do now have to press "ENTER" for each commend line I copy+paste
> into the KOnsole :-/

That's a different issue, not relates to konsole, see bug 431695.

So I just woke up to another CC notification on this bug (Patrick Silva), and being in the creative state of just waking up while thinking about this bug gave me an idea...

It seems to me that the new blended-color-selection behavior has visual characteristics both good and bad analogous to those of transparency/opacity, with arguably similar basic aesthetic appeal and ultimately similar practical readability problems, too.  Arguably, then, a possible solution could be similar as well.

The transparency preference is part of the color scheme, with the configuration being a 0-100% slider found in configuration/profiles/edit-profile dialog, appearance/color-scheme/edit-color-scheme dialog.

What about a similar selection-blend slider in the same dialog as it's arguably similarly part of the color-scheme, 100%-strict-reverse at one end of the slider, 50/50 (or whatever) foreground/background blend (thus making foreground/background the same and selected text unreadable, again conceptually similar to 100% transparent) at the other?

Or a bit more complex, add a selection-color static/dynamic checkbox option (analogous to the blur-background checkbox for transparency) to the reverse/blend slider.  Dynamic would be the blend with the slider behaving as above. Static would have color-pickers for selection foreground/background, with the slider blending between those and strict reverse.

Of course the question then would be where to set the defaults since it's well known that many users don't mess with them.  Personally I'd favor 100% strict reverse as the most usable default for accessibility reasons, and I suppose most users on this bug would agree or they'd not be on the bug.  OTOH, the author of the original color-blending selection commit would likely argue for a partially blended default.

Unquestionably that's a lot of work compared to just reverting to the old 100% reverse-video selection behavior, work that I can't do because I'm not a coder, but it seems to me to be a reasonable compromise that would let people on both sides have exactly the behavior they want/need, as long as they're willing to spend a bit of time configuring it.

And having it all per-profile as part of the appearance settings makes perfect sense too, just as it does with transparency.

https://invent.kde.org/utilities/konsole/-/merge_requests/403

Git commit 38208f94cd371ab2ebb66fe008441759b487f1e6 by Ahmad Samir.
Committed on 22/06/2021 at 09:56.
Pushed by hindenburg into branch 'master'.

Add option to always invert text selection colours

The recent change of blending the background/foreground colours for text
selection doesn't work everywhere, users with vision impairment or
colour-blindness would probably find it hard to distinguish text selection
colours; and the current code can't possibly cover all variations of colour
schemes out there. The same goes with certain displays and viewing angles,
some display types are notoriously awful when viewed at an angle.
FIXED-IN: 21.08

M  +19   -3    src/Screen.cpp
M  +7    -0    src/Screen.h
M  +1    -1    src/characters/Character.h
M  +2    -0    src/profile/Profile.cpp
M  +4    -0    src/profile/Profile.h
M  +1    -1    src/terminalDisplay/TerminalDisplay.cpp
M  +1    -1    src/terminalDisplay/TerminalDisplay.h
M  +1    -1    src/terminalDisplay/TerminalPainter.cpp
M  +7    -0    src/widgets/EditProfileAppearancePage.ui
M  +5    -0    src/widgets/EditProfileDialog.cpp

https://invent.kde.org/utilities/konsole/commit/38208f94cd371ab2ebb66fe008441759b487f1e6

Git commit 7326f9289fa356322f4534e0d1f23869edcad463 by Jan Blackquill.
Committed on 23/04/2021 at 22:12.
Pushed by aacid into branch 'mr/380'.

Use inverted colours when calculated fancy BG has too low contrast

M  +18   -4    src/terminalDisplay/TerminalPainter.cpp

https://invent.kde.org/utilities/konsole/commit/7326f9289fa356322f4534e0d1f23869edcad463