Differential D19542

[WIP] Make pop-up note text visible whatever background color is
AbandonedPublic
Actions

Authored by yurchor on Mar 5 2019, 3:08 PM.

Details

Reviewers
None
Group Reviewers
Okular
Summary

The user can choose the blue or some dark color for annotation background and the black text is hardly visible on such backgrounds.

Example:

This patch tries to use the luminance of background to determine what color (white or black) should be used for the text.

I know that it's a silly thing to use the dark background for the black text but it seems that someone's gotta do it. ;)

Currently, you should type something in the pop-up note to make the text visible. I could not find the right place for the color change yet. The help in determining the entry point to change the color from the beginning will be much appreciated.

BUG: 405105

Test Plan
  1. Create a pop-up note with the default (yellow) background (F6, click on the "Pop-up Note" button, click somewhere in the document).
  2. Type something. The text should be black (light background).
  3. Use "Settings -> Configure Okular..." in the main menu. Go to the "Annotations" page, choose "Pop-up Note", then press "Edit" and select the blue color.
  4. Create a pop-up note with the new (blue) background.
  5. Type something. The text should be white (dark background).

Screenshot:

Test file (you should type something in the "blue" pop-up note window to see the text):

test.pdf35 KBDownload

Diff Detail

Repository
R223 Okular
Lint
Lint Skipped
Unit
Unit Tests Skipped
yurchor created this revision.Mar 5 2019, 3:08 PM
Restricted Application added a project: Okular. · View Herald TranscriptMar 5 2019, 3:08 PM
Restricted Application added a subscriber: okular-devel. · View Herald Transcript
yurchor requested review of this revision.Mar 5 2019, 3:08 PM
aacid added a subscriber: aacid.Mar 6 2019, 12:02 AM

You don't need to change the color though settings, just right click on the annotation and change it's color.

I don't understand your "I could not find the right place for the color change yet", aren't you changing the color in there?

If we're going to go this route, shouldn't the header behave the same?

yurchor added a comment.Mar 6 2019, 9:41 AM
In D19542#425607, @aacid wrote:

You don't need to change the color though settings, just right click on the annotation and change it's color.

Oops... Thanks for the notice. :)

I don't understand your "I could not find the right place for the color change yet", aren't you changing the color in there?

Sure. But it is definitely not the right place because

  1. The color does not change at once (you need to press at least one key)

and

  1. The header does not behave the same way:

If we're going to go this route, shouldn't the header behave the same?

Yes. If nobody gives some advice, I will try to find the right place by myself.

davidhurka added a subscriber: davidhurka.Mar 6 2019, 10:35 AM

Erm, my Okular 1.6.2 already does this, just with another threshold. Below value() = 129, the text (and the header) become white, above value() = 128, they become black.

While searching for the code, I just found ui/drawingtoolactions.cpp:48, which does something similar:

 // draw check mark
const int lightness = ((color.red() * 299) + (color.green() * 587) + (color.blue() * 114)) / 1000;
p.setPen( lightness < 128 ? Qt::white : Qt::black );
p.drawText( QRect( QPoint( 0, 0 ), pmSel.size() ), Qt::AlignCenter, QStringLiteral("\u2713") );

Is it possible that Qt automatically sets the text color based on the background color, when the text color is not explicitely given?

yurchor added a comment.Mar 6 2019, 11:00 AM
In D19542#425698, @davidhurka wrote:

Erm, my Okular 1.6.2 already does this, just with another threshold. Below value() = 129, the text (and the header) become white, above value() = 128, they become black.

While searching for the code, I just found ui/drawingtoolactions.cpp:48, which does something similar:

 // draw check mark
const int lightness = ((color.red() * 299) + (color.green() * 587) + (color.blue() * 114)) / 1000;
p.setPen( lightness < 128 ? Qt::white : Qt::black );
p.drawText( QRect( QPoint( 0, 0 ), pmSel.size() ), Qt::AlignCenter, QStringLiteral("\u2713") );

Is it possible that Qt automatically sets the text color based on the background color, when the text color is not explicitely given?

Do you mean that the attached test case (text.pdf) works for you (the text in annotation window for the right blue pop-up note is visible)?

That's what I have for the git/master:

davidhurka added a comment.Mar 6 2019, 11:41 AM
In D19542#425756, @yurchor wrote:

Do you mean that the attached test case (text.pdf) works for you (the text in annotation window for the right blue pop-up note is visible)?

No, but the functionality is already there:

yurchor abandoned this revision.Mar 6 2019, 12:00 PM

Looks like this is expected Qt behavior. Nothing to tweak here. Should be corrected in Qt somehow (corner cases of the correct luminance determination).

Thanks to @davidhurka for testing.

davidhurka added a comment.Mar 6 2019, 2:35 PM

After reading some Wikipedia, I can say:

  • Checkmarks use luma (NTSC)
  • Your approach uses luma (sRGB)
  • QColor::lightness() uses lightness (HSL-bi-hexcone)

QColor::lightness() would not solve the problem, it would put white #ffffff text on yellow #00ffff background. Qt had to compute luma, but luma() would need to know the color space of the output device. I can imagine that Qt developers decided to use value() as a better tradeoff than assuming sRGB or using the primary display’s color space.

Revision Contents
Changeset List

PathPackages
Mui/annotwindow.cpp (2 lines)

Diff 53206

View Options

ui/annotwindow.cpp

Loading...