Add typewriter annotation tool
ClosedPublic

Authored by tobiasdeiminger on Sep 1 2018, 10:36 AM.

Details

Summary

Typewriter is originally specified by the PDF reference as special FreeText annotation, where Intent=FreeTextTypewriter. It features opaque letters on transparent background, so that users can fill non interactive forms. Herewith typewriter is implemented natively for PDF, and there's also an Okular specific implementation for other document types. The added tool reuses the inline note UI.

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

FEATURE: 353401

Test Plan
  • okularpartrc is generated (if not yet existing) with typewriter as 10th tool
  • typewriter tool is also available in Annotation Tools -> Add, Typ "Typewriter"
  • selecting the tool and left click into document opens inline note input dialog
  • finishing creates an annotation similar to inline note, but with transparent background
  • saving into PDF results in /Subtype FreeText /IT /FreeTextTypeWriter
  • saving typewriter into archive stores color with alpha channel = 0x00
  • opening annotated archive works, if archive was created with old Okular, and opened in patched Okular
  • opening annotated archive works, if archive was created with patched Okular, and opened in old Okular

Diff Detail

Repository
R223 Okular
Branch
typewriter_td1 (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 2515
Build 2533: arc lint + arc unit
Restricted Application added a project: Okular. · View Herald TranscriptSep 1 2018, 10:36 AM
Restricted Application added a subscriber: okular-devel. · View Herald Transcript
tobiasdeiminger requested review of this revision.Sep 1 2018, 10:36 AM
sander added a subscriber: sander.Sep 4 2018, 1:32 PM

Would it a good idea to split those changes that deal with the color alpha channel into a separate patch? That would make reviewing easier, and lead to more legible git history.

autotests/parttest.cpp
45

Is this change really needed for the Typewriter tool, or is it 'just' related cleanup of the CloseDialogHelper class? If the latter, can we move it into a separate patch?

ui/pageviewannotator.cpp
274

Whitespace change

Would it a good idea to split those changes that deal with the color alpha channel into a separate patch? That would make reviewing easier, and lead to more legible git history.

Good idea. Alpha channel changes for other annotations are small in LOC and independent from typewriter. We can easily shift that to a new revision.

autotests/parttest.cpp
45

Is this change really needed for the Typewriter tool

It was indeed required. qApp->activeModalWidget()was the simples way to lookup the modal dialog. As a consequence, Okular::Part *p was no longer required. QMetaObject::invokeMethod(button, "click", Qt::QueuedConnection) was required to avoid crashes.

Though the latter is probably a workaround for a hidden bug in Okulars PickPointEngine::addTextNote. Try this: Fire up okular in KDABs gammaray, select inline note tool, and click into a page. Now 20..50 QInputDialogs pop up immediately, instead of 1. That's basically the same as what we encountered in the test prior using invokeMethod.

Rebased on D15279, to factor out ARGB format change. Fix whitespace change.

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

D15279 has landed, we can depend on master again.
Factor out autotests. Will be moved on top of revision stack.

tobiasdeiminger edited the summary of this revision. (Show Details)Sep 8 2018, 2:39 PM

Code is looking good to me. I tested it and couldn't find any problems.

Minor nit-picking: The background of the text-editing widget is white rather than yellow (which is used for inline notes). This is not really pleasant to read if the paper color is white, too

conf/editannottooldialog.cpp
274

Default the text input popup to some light yellow.

ui/annotationwidgets.cpp
194

Better handle this in an override in TextAnnotationWidget.

204–224

Really only if TextAnnotation::Unknown? What about TextAnnotation::Callout?

222–223

Those generic signal slot connections must not be dependent on if it's a TextAnnotation.

289–292

Leave connect where it was before, don't return early and delegate widget setup to private methods createPopupNoteStyleWidget, createInlineNoteStyleWidget, createTypeWriterStyleWidget.

ui/annotationwidgets.h
95

It's not nice to have such a special type in the base class. Should better go to TextAnnotationWidget.

102

All pointer type members of TextAnnotationWidget should be default initialized to nullptr (unrelated cleanup).

ui/pageviewannotator.cpp
153

Getting input from user by dialog and adding the annotation are two different things. Divide it into two methods (or just leave QInputDialog::getMultiLineText in PickPointEngine::end).

tobiasdeiminger marked 8 inline comments as done.Sep 24 2018, 8:26 AM

This revision should be quite complete now.

AnnotWindow::reloadInfo if ( m_annot->subType() == Okular::Annotation::AText ) seems like a wrong place to do special typewriter handling (make popup dialog pastel yellow), but I had no better idea. Guess it's not worth the while to invent some derived TypewriterAnnotWindow, because typewriter should get a complete different UI as midterm goal anyway. Any suggestion?

And I've got an open question: What is the original intention of the engine [color] attribute from tools.xml? Enrico Ros introduced it 13 years ago, but I see no reason why we couldn't get along with solely the annotation [color] attributes. As far as I can tell, engine color is always in sync with annotation [color], therefore it's redundant, and it's not clear when to use what. I'm ignoring it for typewriter for now.

sander accepted this revision.Sep 25 2018, 2:22 PM
This revision is now accepted and ready to land.Sep 25 2018, 2:22 PM

@sander Thanks for the review!

I'll commit soon. Is there someone who can update user manual and translations? How is this handled usually?

AFAIK for the manual you simply have to update doc/index.docbook.

Totally awesome to see this so close to being merged!

tobiasdeiminger edited the summary of this revision. (Show Details)Sep 25 2018, 8:44 PM
This revision was automatically updated to reflect the committed changes.