Support setting text color for typewriter annotations
ClosedPublic

Authored by tobiasdeiminger on Sep 1 2018, 12:48 PM.

Details

Summary

Changing typewriter text color can be done in the typewriter properties dialog, or programmatically via new okular API methods TextAnnotation::textColor and TextAnnotation::setTextColor.

poppler >= 0.69 is required to store text color natively inside PDF documents. For other document types, text color is stored as metadata inside the document archive.

This work was done during GSoC 2018. See https://community.kde.org/GSoC/2018/StatusReports/DileepSankhla for details.

Test Plan
  • properties dialog of typewriter annotation has "Font Color" picker
  • saving to PDF results in <r> <g> <b> rg operation in /DA
  • saving to archive results in fontColor="rrggbb" attribute in metadata.xml

Diff Detail

Repository
R223 Okular
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
Restricted Application added a project: Okular. · View Herald TranscriptSep 1 2018, 12:48 PM
Restricted Application added a subscriber: okular-devel. · View Herald Transcript
tobiasdeiminger requested review of this revision.Sep 1 2018, 12:48 PM
sander added a subscriber: sander.Sep 4 2018, 1:35 PM
sander added inline comments.
generators/poppler/CMakeLists.txt
108

This will have to be at least 0.69...

Rebase on D15204. Check for poppler 0.69.

tobiasdeiminger marked an inline comment as done.Sep 5 2018, 7:52 AM

Remove reintroduced whitespace change.

No real changes, just follow rebase of parent revision D15204.

conf/editannottooldialog.cpp
274–275

engine [color] seems to be too ambiguous to store font color in it. Consider near future, when we want to implement font color support for inline notes. Then we have two different colors for one annotation, namely "background color" and "text color". One general color attribute (engine [color] / annotation [color] ) is not sufficient there. I think it's better to introduce a new annotation [textColor] attribute now.

Rebase on master. Some review fixes.

Restricted Application added a project: Documentation. · View Herald TranscriptFri, Sep 28, 5:49 AM
Restricted Application added a subscriber: kde-doc-english. · View Herald Transcript
tobiasdeiminger edited the summary of this revision. (Show Details)Fri, Sep 28, 6:48 AM
sander added a comment.Mon, Oct 1, 9:02 AM

Are you sure you rebased this on the current master? The diff still seems to contain D15204 , which has already been pushed.

tobiasdeiminger marked an inline comment as done.

Really rebase on master

sander added inline comments.Tue, Oct 2, 7:14 PM
ui/annotationwidgets.cpp
223

Why this whitespace change?

315

I think these whitespace changes should be in a separate patch (if they are desired at all).

321

I think this whitespace change should be in a separate patch (if it is desired at all).

tobiasdeiminger added inline comments.Tue, Oct 2, 8:05 PM
ui/annotationwidgets.cpp
315

Desired, because I did them wrong in the first typewriter commit. To be honest, I'd find an extra commit saying "3 whitespace changes" more annoying.

How about one single big style cleanup commit for the whole project (pro: helps to spare us periodic discussion about whitespaces for a while, con: remember to tell your tool to ignore whitespace changes when searching history for functional changes).

321

Same as above.

sander added a comment.Wed, Oct 3, 8:54 AM

I now actually tested and it works very nicely. I'll approve the diff, because I don't have a strong opinion on the whitespace issue.

I'd be nice if the color of the 'T' icon could show the current text color (like the Freehand Line icon does it). Currently it is always black. But that is a separate issue, right?

sander accepted this revision.Wed, Oct 3, 8:55 AM
This revision is now accepted and ready to land.Wed, Oct 3, 8:55 AM

I now actually tested and it works very nicely. I'll approve the diff, because I don't have a strong opinion on the whitespace issue.

Thanks for reviewing.

I'd be nice if the color of the 'T' icon could show the current text color (like the Freehand Line icon does it). Currently it is always black. But that is a separate issue, right?

Icon color already worked in Dileeps patch, so it's a regression introduced by me. I'll fix it before pushing.

More review fixes

  • Refine doxygen comments on TextAnnotation::textColor and TextAnnotation::setTextColor
  • Colorize toolbar icon in with text color instead of black
  • Remove accidential whitespace change
tobiasdeiminger marked 2 inline comments as done.Wed, Oct 3, 2:43 PM

Should be ready to land. I'll push tomorrow, time frame for last objections is open now ;-)

tobiasdeiminger retitled this revision from Support setting font color for typewriter annotation to Support setting text color for typewriter annotations.Thu, Oct 4, 6:00 PM
tobiasdeiminger edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.
ngraham added a subscriber: ngraham.Thu, Oct 4, 6:26 PM

I know it's a little late, but now that this has landed, I tried it out so I could add a picture of it to this week's Usability & Productivity post, but I couldn't get it to work. What am I doing wrong?

tobiasdeiminger added a comment.EditedThu, Oct 4, 6:35 PM

I know it's a little late, but now that this has landed, I tried it out so I could add a picture of it to this week's Usability & Productivity post, but I couldn't get it to work. What am I doing wrong?

Just a guess, you don't have latest poppler? You need to compile against 0.69.

Ah yeah, Neon only has Poppler 0.62.

So the color changing feature just doesn't work without Poppler 0.69? If that's the case, we should probably turn it off unless you have a good enough Poppler version.

tobiasdeiminger added a comment.EditedThu, Oct 4, 6:56 PM

Ah yeah, Neon only has Poppler 0.62.

So the color changing feature just doesn't work without Poppler 0.69? If that's the case, we should probably turn it off unless you have a good enough Poppler version.

It doesn't work for pdf in that case. But it still works for all other document types. So turning the feature off in the UI would need to depend on the currently loaded document type. If you think it's important I try to do it.

Turning it off at generator level is already implemented.

ngraham added a subscriber: simgunz.Thu, Oct 4, 7:11 PM

It's probably not that important right now since the color changing feature is totally buried in the UI so I doubt many people will ever find it, but it will probably become more important once @simgunz improves the accessibility of the tool options in D15580.

Also, since it's working for you, would you mind sending me a quick screenshot that shows both a colored typewriter annotation and also the colored toolbar icon? That's a really nice touch.

Also, since it's working for you, would you mind sending me a quick screenshot that shows both a colored typewriter annotation and also the colored toolbar icon? That's a really nice touch.

Sure, is F6304320 ok for you?

Absolutely perfect! Thanks so much.

Do you think it would be useful to have adjustable text color for inline notes, too? Then I'll prepare a similar patch.

sander added a comment.Sun, Oct 7, 8:02 PM

I think so.