Add Typewriter annotation tool in Okular
AbandonedPublic

Authored by dileepsankhla on May 29 2018, 4:49 PM.

Details

Summary

This patch adds the typewriter annotation tool in Okular with a new typewriter icon and the settings dialogue to configure annotation's font and the text input UI is same as that of inline note. It sets the intent to TypeWriter and background color transparent.

Test Plan

Test for forwards and backwards compatibility of #AARRGGBB and #RRGGBB color formats.

1.1. Test for backwards compatibility:

  1. open a test.txt file in older Okular (i.e. compiled from master branch)
  2. add some annotations (inline note, eclipse, ...)
  3. save the document to test.okular document archive
  4. unpack the okular archive unzip test.okular
  5. open metadata.xml and check if all <annotation> ...color=... </annotation> attributes are in #RRGGBB format
  6. pack the archive again zip test_rrggbb.okular content.xml test.txt metadata.xml
  7. open test_rrggbb.okular in Okular with D13203 applied
  8. check if annotations are painted with colors as expected. It should work!

1.2. Test for forwards compatibility:

  1. open a test.txt file in Okular with D13203 applied
  2. add some annotations (inline note, eclipse, ...)
  3. save the document to test.okular document archive
  4. unpack the okular archive unzip test.okular
  5. open metadata.xml and check if all <annotation>...color=...</annotation> attributes are in #AARRGGBB format
  6. pack the archive again zip test_aarrggbb.okular content.xml test.txt metadata.xml
  7. open test_aarrggbb.okular in older Okular (i.e. compiled from master branch)
  8. check if annotations are painted with colors as expected. It should work!

Diff Detail

Repository
R223 Okular
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 992
Build 1005: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application added a project: Okular. Β· View Herald TranscriptMay 29 2018, 4:49 PM
Restricted Application added a subscriber: okular-devel. Β· View Herald Transcript
dileepsankhla requested review of this revision.May 29 2018, 4:49 PM
This revision was not accepted when it landed; it landed in state Needs Review.May 29 2018, 4:59 PM
This revision was automatically updated to reflect the committed changes.

Hmm, what can this do that the regular text annotation tool can't?

Also, why land it immediately without waiting for review?

@ngraham This is the part of my GSoC project: https://summerofcode.withgoogle.com/projects/#6053164340477952

I have committed the same to my separate branch: https://cgit.kde.org/okular.git/log/?h=gsoc2018_typewriter but don't know how it got landed! It needs to be reviewed first. Have I done something wrong?

ltoscano reopened this revision.May 29 2018, 5:06 PM
ltoscano added a subscriber: ltoscano.

Pushed to the gsoc branch, but reopening.

Ah, I see, you committed it to a branch, not to master.

Still, by committing anywhere, Phabricator figured that it had been landed.

I have committed the same to my separate branch: https://cgit.kde.org/okular.git/log/?h=gsoc2018_typewriter but don't know how it got landed! It needs to be reviewed first. Have I done something wrong?

The gsoc* branches are meant to share and log small junks of GSoC progress frequently (even daily), prior to publishing a ready-to-review patch on phabricator. But there haven't been any intermediate GSoC commits during the past two weeks on gsoc2018_typewriter. D13203 on phabricator is just the same as c344c5e31b6a in the branch. So both diffs have the same hash and therefore got automatically linked.

If you don't push your work-in-progress frequently to gsoc2018_typewriter, I doubt there's a point in having that extra branch branch at all. You could equally work on a local branch only and submit a patch after few weeks to phabricator.

If I use the new tool to typewrite onto a plain text file, I get opaque white background instead of transparent background. Transparency does work for PDF documents (i.e. rendered by poppler). In the former case the typewriter annotation is drawn in ui/pagepainter.cpp L.680. Can you checkout what's wrong there?

We should add something like "Transparent background working on any supported document type" to the testplan (or only test .pdf and .txt for a start, it's enough to test both generator and pagepainter drawing paths).

Hmm, what can this do that the regular text annotation tool can't?

The typewriter feature should ultimately get an UI similar to the Adobe Reader typewriter tool, but we decided to start with a less breaking and more working patch πŸ˜„ Therefore Dileep just reuses the inline note tool. The resulting annotation differs from inline note in making background always transparent and setting PDF field "intent" to FreeTextTypeWriter to ensure interoperability with other PDF tools. This fulfills the minimal requirements for a typewriter tool.

In upcoming steps we will try to make the UI more like the Adobe Reader typewriter (no popup window to enter text anymore, but type directly onto the document). That's really not an easy task. Dileep already requested for comments, but there was no response so far.
@ngraham Any ideas how to motivate people joining? Every opinion is appreciated. People with deep knowledge about pagepainter, FormWidgets, annotation subsystem and generators could especially help us finding a promising approach.

The gsoc* branches are meant to share and log small junks of GSoC progress frequently (even daily), prior to publishing a ready-to-review patch on phabricator. But there haven't been any intermediate GSoC commits during the past two weeks on gsoc2018_typewriter. D13203 on phabricator is just the same as c344c5e31b6a in the branch. So both diffs have the same hash and therefore got automatically linked.

If you don't push your work-in-progress frequently to gsoc2018_typewriter, I doubt there's a point in having that extra branch branch at all. You could equally work on a local branch only and submit a patch after few weeks to phabricator.

Thank you so much for the info. From now on, I will frequently push my work to gsoc2018_typewriter.

If I use the new tool to typewrite onto a plain text file, I get opaque white background instead of transparent background. Transparency does work for PDF documents (i.e. rendered by poppler). In the former case the typewriter annotation is drawn in ui/pagepainter.cpp L.680. Can you checkout what's wrong there?

We should add something like "Transparent background working on any supported document type" to the testplan (or only test .pdf and .txt for a start, it's enough to test both generator and pagepainter drawing paths).

In ui/pagepainter.cpp L 668, we can comment out acolor.setAlpha( opacity ) as here the value of opacity is 255 but changing the L 659 or 668 should be consistent in the case of other annotations too. Is commenting out L 668 accepted?

In ui/pagepainter.cpp L 668, we can comment out acolor.setAlpha( opacity ) as here the value of opacity is 255 but changing the L 659 or 668 should be consistent in the case of other annotations too. Is commenting out L 668 accepted?

Hm, if you just comment L.668, how would you control transparency of ordinary inline notes and popup notes? Preferably without changing the look of existing annotations created in earlier Okular versions?

Let's reconsider. We have two transparency parameters for each annotation:

  • Alpha channel of QColor Style::color
  • double Style::opacity

Sadly it seems like PagePainter and Poppler currently have different interpretations of those two values:

  • Poppler: Use alpha channel of Style::color to control transparency of background color. Use Style::opacity to control transparency of the whole appearance=border+background+font.
  • PagePainter: Ignore alpha channel of Style::color. Use Style::opacity to control transparency of background color. No way to control transparency of the whole appearance=border+background+font.

This is a inconsistent. I think we could either fix this inconsistency in Poppler or PagePainter (more effort), or hardcode some kind of

if ( inplaceIntent == Okular::TextAnnotation::InplaceIntent ) acolor.setAlpha(0);

in PagePainter.

Any other ideas?

In ui/pagepainter.cpp L 668, we can comment out acolor.setAlpha( opacity ) as here the value of opacity is 255 but changing the L 659 or 668 should be consistent in the case of other annotations too. Is commenting out L 668 accepted?

if ( inplaceIntent == Okular::TextAnnotation::InplaceIntent ) acolor.setAlpha(0);

in PagePainter.

Any other ideas?

How about changing ui/pagepainter.cpp L668 to

acolor.setAlpha( a->style().opacity() * a->style().color().alpha() );

It gives us transparent background for new typewriter annotations. It should be backwards/forwards compatible with *.okular documents from other Okular versions. And I believe it's more consistent to what happens in Poppler, because I assume in Poppler color[alpha] and opacity will also get multiplied to determine the final alpha of background color for FreeText, if both values are set. But haven't verified this assumption yet.

How about changing ui/pagepainter.cpp L668 to

acolor.setAlpha( a->style().opacity() * a->style().color().alpha() );

It gives us transparent background for new typewriter annotations. It should be backwards/forwards compatible with *.okular documents from other Okular versions. And I believe it's more consistent to what happens in Poppler, because I assume in Poppler color[alpha] and opacity will also get multiplied to determine the final alpha of background color for FreeText, if both values are set. But haven't verified this assumption yet.

I agree but why do we need to multiply alpha value and opacity to determine the final alpha of background color for FreeText in Poppler? It is already determined by alpha value solely. I mean isn't it enough to change the line to acolor.setAlpha( a->style().opacity() * a->style().color().alpha() ) as you suggested? Why do we need to change the Poppler's one?

Secondly, in my opinion, we should change L 628 to unsigned int opacity = (unsigned int)( a->style().color().alpha() * a->style().opacity() ) and here the variable opacity will determine the background color of FreeText.

I mean isn't it enough to change the line to acolor.setAlpha( a->style().opacity() * a->style().color().alpha() ) as you suggested? Why do we need to change the Poppler's one?

I didn't mean to change anything in Poppler. I meant it's the way it works in Poppler right now. I assume if both 'rg' operator and 'ca' entry are applied, alpha gets multiplied. Probably also depending on blend mode. I don't know how this works exactly, it's a longer read through AnnotFreeText::generateFreeTextAppearance and the spec (e.g. 11.6.3 Specifying Blending Colour and Blend Mode). Maybe you can figure it out on your own?

Secondly, in my opinion, we should change L 628 to unsigned int opacity = (unsigned int)( a->style().color().alpha() * a->style().opacity() ) and here the variable opacity will determine the background color of FreeText.

You're right, that's better.

Fixed typewriter transparency in PagePainter

I didn't mean to change anything in Poppler. I meant it's the way it works in Poppler right now. I assume if both 'rg' operator and 'ca' entry are applied, alpha gets multiplied. Probably also depending on >blend mode. I don't know how this works exactly, it's a longer read through AnnotFreeText::generateFreeTextAppearance and the spec (e.g. 11.6.3 Specifying Blending Colour and Blend Mode). Maybe you >can figure it out on your own?

It's also complicated for me to comprehend but there is a blending mode function B(Cb,Cs) that multiplies the source color with the backdrop color. If not necessary, I won't go into the details.

Btw I have updated the revision to have typewriter background transparent in PagePainter.

Typewriter annotations created with the initial typewriter tool have a border of width=1 (Okular started without existing okularpartrc). But we always want border width = 0 for typewriter, is it? It could even be hardcoded into drawing code.

This is because you set width = 0 only for EditAnnotToolDialog::m_stubann. But the initial tool entry created out of tools.xml does not use this stub annotation to determine properties. Only further typewriter tools created with the EditAnnotTool config dialog use the stub annotation. So my okularpartrc looks like this:

[Reviews]                                                                                                                       
AnnotationTools=[...]<tool type="typewriter" id="10"><engine type="PickPoint" color="#000000" block="true"><annotation type="Typewriter" color="#00ffffff"/></engine></tool>,<tool type="typewriter" id="11" name="My own typewriter"><engine type="PickPoint" color="#000000" block="true"><annotation type="Typewriter" color="#00ffffff" width="0"/></engine></tool>

As you see, only the second typewriter has width=0. The first uses default width (=1).

Fixed initial typewriter border width to 0

This revision was not accepted when it landed; it landed in state Needs Review.Jun 1 2018, 1:18 PM
This revision was automatically updated to reflect the committed changes.

Hell! Sorry for this mistake again. Whenever I update the revision, I push the commit to the gsoc2018_typewriter branch too and the revision landed in need review state by that commit (both have the same hash).
@ltoscano May you please reopen this revision?

Should I push the commit to the gsoc branch or not? I usually update revision in this manner:

  1. git branch

*gsoc2018_typewriter
master

  1. vim file.cpp
  2. git commit -m "msg"
  3. git checkout master
  4. git merge gsoc2018_typewriter
  5. arc diff --update D13203

It sends two new commit, one created in the gsoc branch and one at the point of merging to master branch

  1. git reset --hard <Commit ID>

Here <Commit ID> is the last commit before merging gsoc2018_typewriter branch

  1. git checkout gsoc2018_typewriter
  2. git push origin gsoc2018_typewriter

And finally, pushing the commit to origin/gsoc2018_typewriter branch.

Can you suggest me where I'm wrong?

tobiasdeiminger added inline comments.Jun 1 2018, 4:52 PM
ui/annotwindow.cpp
262

QColor(m_annot->style().color().name()) is a bit arcane way of saying "force alpha of the popup window background color to 1.0" πŸ˜„ If that's what you've actually intended?

How about removing const of newcolor (not bad, local scope only) and do newcolor.setAlpha(1.);?
Or something like

const QColor newcolor = m_annot->style().color().isValid() ? QColor(m_annot->style().color().red(), m_annot->style().color().green(), m_annot->style().color().blue(), 255) : Qt::yellow;

if you want to keep const. That would be more explicit and obvious.

Btw., sorry for commenting on an older revision. But ui/annotwindow.cpp is not included in the latest revision. Phabricator seems to be a bit too smart for our intended workflow.

dileepsankhla marked an inline comment as done.Jun 2 2018, 3:41 AM

The revision is closed. I'm unable to update it. Maybe someone with admin privileges should reopen it?

ui/annotwindow.cpp
262

I agree. It should be straightforward without any latent meaning. Updating the revision.

dileepsankhla reopened this revision.Jun 2 2018, 3:44 AM
dileepsankhla marked an inline comment as done.

Updating annotation popup window background color

This revision was not accepted when it landed; it landed in state Needs Review.Jun 4 2018, 6:58 AM
This revision was automatically updated to reflect the committed changes.
dileepsankhla marked an inline comment as done.
dileepsankhla reopened this revision.Jun 4 2018, 6:59 AM

Diff changed after commiting to gsoc2018_typewriter branch. Updating it.

tobiasdeiminger added inline comments.Jun 4 2018, 2:00 PM
conf/editannottooldialog.cpp
128–129

So from now on ALL annotations (even ink, highlight, inline note etc) that are created by a user-defined tool will store color as #AARRGGBB in *.okular document archive, right? Formerly it was #RRGGBB.

What will happen if user A with new Okular saves an inline note to mydoc.okular and sends mydoc.okular to user B who has an older Okular version. Will the older version be able to deal with #AARRGGBB? How is it vice versa, user B sending something to user A?

If this change is indeed forwards and backwards compatible, I think it's good to go this way. We prepare other annotations types for transparency without destroying something. Then we should however also adapt the color=... attributes of the default tools in tools.xml to the new #AARRGGBB format, for the sake of consistency.

dileepsankhla added inline comments.Jun 8 2018, 8:30 AM
conf/editannottooldialog.cpp
128–129

There is no any problem in the newer Okular version as what I have found is that it respects both #RRGGBB and #AARRGGBB formats. I don't know about the older version but it should be both forward and backward compatible. And I agree there should be consistency in the color formats.
So if we adopt the new color format in the default tools in tools.xml, do we need to alter all the color entries? Or is it okay to have two color formats?

Adding tests for typewriter annotation tool

autotests/parttest.cpp
115

Please resolve local merge conflict before submitting the diff.

1505

Please resolve local merge conflict before submitting the diff.

1556

From here on the code belongs logically to the next test case, is it? In a mixed purpose test like class PartTest, imho test methods should work independent from one another. I.e., in testDialogClosed don't assume that you've opened dialog in testTypewriterAnnotTool. Iow tests should be idempotent. If you want to retain state across several verifications, do everything in one method or create a new dedicated class PartTestAnnotations where it's more obvious to couple test methods tightly.

1558

Using CloseDialogHelper would be more RAII

1575

The QInputDialog is pure Qt functionality, just closing it is not that interesting in a typewriter test case. The more interesting thing is to verify if annotation is created correctly after closing.

conf/editannottooldialog.cpp
128–129

I don't know about the older version but it should be both forward and backward compatible.

Test for forwards compatibility (def: "allows a system to accept input intended for a later version of itself") is easy.

  • open a test.txt file in Okular with D13203 applied
  • add some annotations (inline note, line, ...)
  • save document to test.okular document archive

Because tools.xml was not yet patched to #AARRGGBB for all annotations, just let's simulate this patch:

  • unpack the okular archive
$ unzip test.okular
  • edit metadata.xml and change all <annotation>...color=...</annotation> attributes to #AARRGGBB format.
$ vi metadata.xml # do :%s/color="#/color="#00/g
  • pack the archive again
$ zip test_aarrggbb.okular content.xml test.txt metadata.xml
  • Open test_aarrggbb.okular in older Okular (i.e. compiled from master branch, or just a recent Ubuntu installation)
$ okular test_aarrggbb.okular
  • Check if annotations are painted with colors as expected.

Just tried it, it works.

Can you please come up with similar test steps for backwards compatiblity (def: "allows for interoperability with an older legacy system") and perform the test?

Reconsidering it, I'm not sure if it's worth the while to open a new Differential patch for the tools.xml alpha channel patch. Probably it's ok to incorporate it in D13203.

But let's at least write both the forwards and backwards compatibility test steps down in the Test Plan field of D13203. It's yet empty. May you please do this?

128–129

So if we adopt the new color format in the default tools in tools.xml, do we need to alter all the color entries? Or is it okay to have two color formats?

I'm not sure if I understand this question. Of course not all color entries that appear somewhere in okular source code πŸ˜ƒ We shall just be consistent with

const QString color = m_stubann->style().color().name(QColor::HexArgb);

So adapt everything to #AARRGGBB that will affect Okular::Annotation::style()->color(). Afaikt this are the color=... attributes of <annotation> elements in tools.xml. But not the color=... attributes of <engine> elements, because they don't affect Okular::Annotation::style()->color(). Are there other places?

dileepsankhla added inline comments.Jun 17 2018, 6:05 PM
autotests/parttest.cpp
1575

First of all, in CloseDialogHelper, m_clicked is always false as m_part->widget()->findChild<QDialog*>(); returns NULL for the QInputDialog being the activeModalWidget. So I didn't use it.

Secondly, I agree that the low tests should be independent but I'm using QTimer technique in testTypewriterAnnotTool to grab the pointer to the active QInputDialog widget for which I need to connect the SIGNAL timeout() to the SLOT testDialogClosed which eventually becomes another test method. So I can't do it in only a single method instead need two different methods in which one of them becomes the slot of QTimer implementation. In this case. do you think I should do the same in the dedicated class like PartTestAnnotation?

Thirdly, to verify if the annotation is created correctly, what parameter(s) should I check?

autotests/parttest.cpp
1575

m_part->widget()->findChild<QDialog*>(); returns NULL for the QInputDialog

That's because QInputDialog is not a child of the part, but a "window" with no parent. You can slightly modify CloseDialogHelper and it will work:

void closeDialog()
{
    //QDialog *dialog = m_part->widget()->findChild<QInputDialog*>();
    QWidget * dialog = qApp->activeModalWidget();

Afaikt this is also compatible with the remaining locations where CloseDialogHelper is used. If you apply the change, CloseDialogHelper::m_part is no longer required and can be removed. Construct CloseDialogHelper with QDialogButtonBox::Cancel or QDialogButtonBox::Ok, to click either the Ok or the Cancel button.

So I can't do it in only a single method instead need two different methods in which one of them becomes the slot of QTimer implementation.

The problem goes away if you use CloseDialogHelper.

Thirdly, to verify if the annotation is created correctly, what parameter(s) should I check?

A perfect UI test would be to check for the painted annotation. As you deferred it because it's too difficult, how about checking part->m_document->page(0)->annotations() for the expected typewriter annotation?

dileepsankhla marked 7 inline comments as done.Jun 19 2018, 8:57 AM

Adapt CloseDialogHelper for use with QInputDialog.
Close typewriter input dialog with ok button and test for expected typewriter annotation.

dileepsankhla edited the test plan for this revision. (Show Details)Jun 20 2018, 5:41 PM
dileepsankhla marked 4 inline comments as done.Jun 20 2018, 5:45 PM
dileepsankhla added inline comments.
conf/editannottooldialog.cpp
128–129

Are there other places?

No, there are not. I'm updating the revision with tools.xml having <annotation> elements color attributes incorporating #AARRGGBB color format.

dileepsankhla marked an inline comment as done.

Updating tools.xml <annotation> elements color attributes to incorporate #AARRGGBB color formats

Added font color in Okular

Updated generator/poppler for text color

generators/poppler/annots.cpp
182

Poppler::TextAnnotation::setTextFont is brand new poppler API (actually not yet merged). We still want to support builds with older poppler versions. So setTextFont needs to be surrounded by some

#ifdef HAVE_POPPLER_0_xy
#endif

where 0_xy is the poppler release for which we assume the API to be released. Maybe 0_67, to be optimistic.

To detect poppler version at build time, add a check_cxx_source_compiles for Poppler::TextAnnotation::setTextFont and define HAVE_POPPLER_0_67 accordingly in generators/poppler/CMakeLists.txt. Also add a cmakedefine to generators/poppler/CMakeLists.txt. You'll find existing examples for other versions if you look at these files.

generators/poppler/annots.cpp
182

Ah, meant setTextColor, not setTextFont, sorry. Btw, (how) can I edit my inline comments here in case of typos?

dileepsankhla added inline comments.Jul 17 2018, 4:00 PM
generators/poppler/annots.cpp
182
  1. Do I need to do something like this in okular/generators/poppler/annots.cpp:
#ifdef HAVE_POPPLER_0_67

ppl_txtann->setTextColor( okl_txtann->textColor() );

#endif //HAVE_POPPLER_0_67
  1. I didn't get what to do in generators/poppler/CMakeLists.txt. May you provide me an example?
sander added a subscriber: sander.Jul 17 2018, 7:24 PM
sander added inline comments.
generators/poppler/annots.cpp
182

1.) yes

  1. You need to write a test that sets the HAVE_POPPLER_0_67 flag. Have a look at that CMakeLists.txt file. It contains calls to check_cxx_source_compiles. Each call contains a small C++ program and a HAVE_POPPLER_*** flag. The build system tries to compile the program, and if that works, then the flag is set. You need to write a new call that tests for your setTextColor method, and set the HAVE_POPPLER_0_67 flat.
dileepsankhla added inline comments.Jul 18 2018, 7:59 PM
generators/poppler/annots.cpp
182

I have added the following lines in generator/poppler/CMakeLists.txt:

check_cxx_source_compiles("
#include <poppler-qt5.h>
#include <QColor>
int main()
{
    Poppler::TextAnnotation *annot = new Poppler::TextAnnotation( Poppler::TextAnnotation::InPlace );
    annot->setTextColor(Qt::blue);
    return 0;
}
" HAVE_POPPLER_0_67)

The typewriter text is created with the desired color but whenever I move the annotation, the text color falls back to "black". It means in `PopplerAnnotationProxy::notifyModification``, `ppl_txtann->setTextColor( okl_txtann->textColor() );`` is never get called. Why is this happening even after setting the flag in both CMakeLists.txt and annots.cpp?

sander added inline comments.Jul 18 2018, 8:08 PM
generators/poppler/annots.cpp
182

Is the flag really set (check in build/generators/poppler/config-okular-poppler.h)? If not: throw away the content of your 'build' directory and try again from scratch.

generators/poppler/annots.cpp
182

Hm, for me it works. By reading your comment I suppose you missed to add

/* Defined if we have the 0.67 version of the Poppler library */
#cmakedefine HAVE_POPPLER_0_67 1

to config-okular-poppler.h.cmake, is it?

From cmake hellp:

#cmakedefine VAR ... will be replaced with either:
#define VAR ...
or:
/* #undef VAR */
depending on whether VAR is set in CMake to any value not considered a false constant by the if() command
dileepsankhla marked 7 inline comments as done.Jul 19 2018, 3:59 AM

Set flag HAVE_POPPLER_0_67 in generator/poppler

I'd like to split this into two new Diffs

  1. Add typewriter (everything until ID 36397)
  2. Add fontcolor support (the reminder, depends on "Add typwriter")

It's easier to review and test. And, the first part could land quite immediately, the second part depends on a pending poppler patch that needs some more research.

If there are no objections posted the next few days, I'll do it.

Sounds like a good idea to me.

I'd like to split this into two new Diffs

Sounds like a good idea to me.

Done. Let's jump over to D15204 and D15205 for further actions.
@dileepsankhla: Could you close D13203, to avoid confusion?

dileepsankhla abandoned this revision.Sep 1 2018, 5:54 PM