Fix calculation of TextAnnotation size so that select/move rectangle fits exactly around icons regardless of PDF page size and DPI settings
Needs RevisionPublic

Authored by tobiasdeiminger on Jan 2 2018, 6:42 PM.

Details

Reviewers
aacid
Group Reviewers
Okular
Summary

BUG: 387639
BUG: 388458

I'd consider it only as partial fix. Poppler forces us to use 24x24 pts, else we'd never get a match. Poppler silently ignores what size applications set in Poppler::Annotation::setBoundary or what comes from the PDF objects /Rect entry, in the case of an external writer. This should IMHO get fixed at Poppler side, or at least it should be clarified why forcing 24x24 pts was necessary. The PDF standard just says "readers shall provide predefined icon appearances for standard names: Comment, Key, Note, Help, NewParagraph, Paragraph, Insert" [12.5.6.4]. It doesn't say anything about a predefined size. It says, the /Rect entry defines position and size [12.5.2, 12.5.3].

Patch in F5620896 shows how poppler could help out.

Some further references to get an idea why 24x24 enforcement was done in Poppler:

Test Plan

Add TextAnnotations (the ones from the top entry of the annotation toolbar) to PDF documents with various page sizes (e.g. A3, A4, letter, ...).

  1. Select the icon by left-click. The size of the drawn rectangle shall match with the rendered icon.
  2. Move the icon around by left-click + dragging. The size of the drawn rectangle shall match with the rendered icon.

Open a file with externally created (e.g. LaTeX) text annotation, and repeat steps from above.

Start Okular with different hidpi setting, e.g.

$ QT_SCREEN_SCALE_FACTORS='DVI-I-1=2' okular

Repeat steps from above.

Diff Detail

Repository
R223 Okular
Lint
Lint Skipped
Unit
Unit Tests Skipped
tobiasdeiminger created this revision.Jan 2 2018, 6:42 PM
Restricted Application added a project: Okular. · View Herald TranscriptJan 2 2018, 6:42 PM
tobiasdeiminger requested review of this revision.Jan 2 2018, 6:42 PM
ngraham edited the summary of this revision. (Show Details)Jan 2 2018, 9:58 PM
tobiasdeiminger edited the test plan for this revision. (Show Details)

Remove unnecessary brackets.

tobiasdeiminger edited the summary of this revision. (Show Details)Jan 3 2018, 4:38 PM
tobiasdeiminger edited the summary of this revision. (Show Details)Jan 3 2018, 8:21 PM
tobiasdeiminger edited the test plan for this revision. (Show Details)
tobiasdeiminger edited the summary of this revision. (Show Details)Jan 5 2018, 8:23 AM
aacid requested changes to this revision.Feb 21 2020, 6:56 PM
aacid added a subscriber: aacid.

Please move as a Merge Request in https://invent.kde.org/kde/okular

We have pre-commit CI and lots of checks including clazy and clang-tidy there so it's a much better place for doing the review/approval/merge of the code.

This revision now requires changes to proceed.Feb 21 2020, 6:56 PM
Restricted Application added a subscriber: okular-devel. · View Herald TranscriptFeb 21 2020, 6:56 PM