Add line annotation ending arrows for non PDF documents
ClosedPublic

Authored by tobiasdeiminger on May 16 2019, 10:41 PM.

Details

Summary

This implements drawing the various line ending styles for 2 point lines in non-PDF documents.

Looks like this:

CCBUG: 381629

Test Plan
  • open a *.txt document
  • draw line annotations with different arrow styles: Square, Diamond, OpenArrow, ClosedArrow, ROpenArrow, RClosedArrow, Butt, Slash, Circle
  • ... as start and as end style (start needs D21238 to be configurable in GUI)
  • ... filled or not (fill color can't be configured in GUI yet)
  • ... with different leader line settings
  • ... at various angles
  • rotate and scale page
  • leader line with setting 100 gives 100 pixel line at 100% zoom (use kruler to verify)

Diff Detail

Repository
R223 Okular
Branch
feature/drawlineendings
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 12171
Build 12189: arc lint + arc unit
Restricted Application added a project: Okular. · View Herald TranscriptMay 16 2019, 10:41 PM
Restricted Application added a subscriber: okular-devel. · View Herald Transcript
tobiasdeiminger requested review of this revision.May 16 2019, 10:41 PM

@knambiar If this once lands, you can remove your "for PDF only" tooltip.

sander added a subscriber: sander.May 17 2019, 4:38 AM

Is there an existing class for affien matrix operations?

What about QMatrix or QTransform?

What about QMatrix or QTransform?

Thanks, using QTransform now, it's already widely used in Okular.

Added Butt and Slash. Shorten main line for arrow.

tobiasdeiminger edited the summary of this revision. (Show Details)May 17 2019, 7:33 AM

Added circle, expose LineAnnotPainer.

tobiasdeiminger retitled this revision from WIP: Add line annotation ending arrows for non PDF documents to Add line annotation ending arrows for non PDF documents.May 18 2019, 8:58 AM
tobiasdeiminger edited the summary of this revision. (Show Details)
tobiasdeiminger edited the test plan for this revision. (Show Details)
  • rebase on master
  • remove the "Only for PDF documents" tooltips
  • change drawing of (R)OpenArrow, (R)ClosedArrow, Slash so that combination with leader line makes sense for dimensioning purpose (I think poppler should be adapted accordingly)
  • fix drawing leader lines
sander added inline comments.May 21 2019, 8:25 AM
ui/pagepainter.cpp
450

Is that MISSING comment still relevant? What exactly does it mean anyway?

ui/pagepainter.h
83

Nitpick: The identation of that ); doesn't match the rest of the file.

ui/pagepainter.cpp
450

The comment below for HighlightAnnotation says MISSING: under/strike width, feather, capping, so it seems the comments had been intended to list annotation sub type features which are not yet implemented. In this case a consistent comment for LineAnnotation would be e.g. MISSING: caption, line endings for multi point lines. Will change it accordingly.

ngraham added a subscriber: ngraham.

Fix review comments. Minor cleanups.

tobiasdeiminger marked 3 inline comments as done.May 21 2019, 8:52 PM
tobiasdeiminger edited the summary of this revision. (Show Details)May 22 2019, 9:58 PM
aacid added inline comments.May 24 2019, 10:29 PM
ui/pagepainter.cpp
948

const here and for fImageHeight and penWidth

Add some const.

Passing QTransform by rvalue reference in LineAnnotPainter::LineAnnotPainter had no special meaning, change it to const reference.

tobiasdeiminger marked an inline comment as done.May 25 2019, 11:53 AM

Don't draw a line end if line end style is LineAnnotation::None.

I haven't tested it and haven't tried understanding the code farther than "this is a bunch of painting code".

On the other hand it's "just" painting code, and if it looks like that image, looks good to me.

I'd say give it some time (a week?) in case people want to review and otherwise just commit it.

Rebase on master.

Don't require a Page reference to construct LineAnnotPainter. Knowing the size is enough and makes reuse (e.g. draw preview icons) easier.

Renamed transformation arguments to make it a bit clearer what they are doing.

I'd say give it some time (a week?) in case people want to review and otherwise just commit it.

Ok, thanks, I'm about to land it and will care for bugfixes if something odd happens. Would anybody accept the revision , for the sake of workflow correctness?

@aacid Are there chances to get MR 269 into poppler in near future? It makes line end drawing more consistent with D21248 and fixes some cases.

sander accepted this revision.Jun 1 2019, 2:55 PM
This revision is now accepted and ready to land.Jun 1 2019, 2:55 PM
This revision was automatically updated to reflect the committed changes.