Okular Annotation: add support for line ending style for Straight Line tool
ClosedPublic

Authored by knambiar on Apr 23 2019, 6:23 AM.

Details

Summary

Poppler and Okular already have support for specifying Line End style (TermStyle) for the Straight Line tool. Expose the functionality in configuration and hook up the correct slots.

Configure annotations (before):

Configure annotations (after):

Straight Line tool with Open Arrow end in action:

Test Plan
  1. Open a PDF in Okular
  2. Enable Review
  3. Right click on Review toolbar and Configure annotations
  4. Create (or edit existing) Straight Line tool
  5. Set the ‘Line End’ option on Style and Apply
  6. Use the Straight Line tool to draw a line and check the line ending style.

Diff Detail

Repository
R223 Okular
Lint
Lint Skipped
Unit
Unit Tests Skipped
knambiar created this revision.Apr 23 2019, 6:23 AM
Restricted Application added a subscriber: okular-devel. · View Herald TranscriptApr 23 2019, 6:23 AM
knambiar requested review of this revision.Apr 23 2019, 6:23 AM
knambiar edited the summary of this revision. (Show Details)Apr 23 2019, 6:46 AM
ngraham added a reviewer: VDG.Apr 23 2019, 6:19 PM
ngraham added a subscriber: ngraham.

Hey, that's pretty cool! Any chance you could include a little icon/preview of the visual style for each item in the combobox?

Hey, that's pretty cool! Any chance you could include a little icon/preview of the visual style for each item in the combobox?

That’s a good suggestion. I’ll see what I can do — may be easiest is to include some Unicode symbols in the string itself (I don't know how to otherwise include an icon in the combo box text).

knambiar updated this revision to Diff 56876.EditedApr 24 2019, 9:21 AM

Add Unicode symbols to the line ending style. Didn’t find suitable symbols for ‘Right Closed Arrow’ and ‘Slash’.

Add Unicode symbols to the line ending style. Didn’t find suitable symbols for ‘Right Closed Arrow’ and ‘Slash’.

Nice patch! Guess unicode symbols are good for a start.

Would it be useful if I tried to provide an SVG as replacement for the unicode symbols in an upcoming version, to resemble the exact line end drawing instructions as we do them in poppler code?

Nice patch! Guess unicode symbols are good for a start.

Thanks!

Would it be useful if I tried to provide an SVG as replacement for the unicode symbols in an upcoming version, to resemble the exact line end drawing instructions as we do them in poppler code?

Certainly. Meanwhile I read the documentation and see that QComboBox::setItemIcon can be used to set the icon for combo box text.

P.S.: I couldn't find the drawing methods in poppler at Annot.cc:1710, assuming it is at line 1576, method AnnotAppearanceBuilder::drawLineEnding.

knambiar updated this revision to Diff 56886.Apr 24 2019, 11:50 AM

Merge the two commits (previous revision update had only the second change).

Would it be useful if I tried to provide an SVG as replacement for the unicode symbols in an upcoming version, to resemble the exact line end drawing instructions as we do them in poppler code?

Certainly. Meanwhile I read the documentation and see that QComboBox::setItemIcon can be used to set the icon for combo box text.

It just came to my mind that we should better reuse Okulars own line ending drawing code for the icon, instead of an SVG. Then I realized that code doesn't even exist yet :-/

I mean the if ( type == Okular::Annotation::ALine ) path in PagePainter::paintCroppedPageOnPainter. It is responsible to draw line annotations for non-PDF documents, where the backend doesn't know about the concept of annotations.
It should handle LineAnnotation::TermStyle, but it doesn't.

So your drop down selection will currently be ignored for EPUB, DjVu, ..., only PDF works. @ngraham: Do you think the patch could land as PDF-only, or do we need multi-format support from the beginning?

P.S.: I couldn't find the drawing methods in poppler at Annot.cc:1710, assuming it is at line 1576, method AnnotAppearanceBuilder::drawLineEnding.

The link should get you to AnnotAppearanceBuilder::drawLineEndArrow. It's just one example. There are multiple methods, like AnnotAppearanceBuilder::drawLineEndDiamond, AnnotAppearanceBuilder::drawLineEndCircle, basically one for each style.

Nice! The symbol/icon should be to the left of the text. That way all the symbols and texts line up vertically in the combobox's pop-up.

knambiar added a comment.EditedApr 24 2019, 4:37 PM

So your drop down selection will currently be ignored for EPUB, DjVu, ..., only PDF works. @ngraham: Do you think the patch could land as PDF-only, or do we need multi-format support from the beginning?

Indeed, I thought about PDF alone (that’s my most pressing use case). In that case, should this combobox mention something like “only for PDF documents”?

knambiar updated this revision to Diff 56942.Apr 25 2019, 7:23 AM

Place the symbols to left of ending style description for better alignment.

Isn’t there a coding convention against non-ascii symbols in source code?

And I think real icons (like svg) would be better. What if someone changes the system font? At least I see some boring rectangles in the source code.

Indeed, I thought about PDF alone (that’s my most pressing use case). In that case, should this combobox mention something like “only for PDF documents”?

I think that is the most obvious way. Don’t disable it when no PDF is opened. You also could add some tooltips, although no other widgets there have tooltips.

knambiar updated this revision to Diff 57001.Apr 26 2019, 4:46 AM

Add tooltips to clarify the line ending style works only for PDF documents.

tobiasdeiminger added a comment.EditedApr 27 2019, 9:17 AM

Isn’t there a coding convention against non-ascii symbols in source code?

There are Qt source code conventions. Don't know if Okular policy exists for which conventions to follow, but I agree it makes sense to avoid non-ascii (don't rely on source code input encoding). QString::fromUtf8 combined with a u8 string literal should work, but haven't tested.

EDIT: QStringLiteral macros with u string literals should work as well, while saving runtime cost.

And I think real icons (like svg) would be better. What if someone changes the system font? At least I see some boring rectangles in the source code.

If we want to avoid the unicode approach completely, I'd advocate to implement drawing the lines by code, i.e. QPainter::drawLine and friends. Then reuse the same code to draw icons. So we gain non-PDF support while we are at it, and I guess it's not much more work than crafting the SVG.

sander added a subscriber: sander.May 2 2019, 8:49 PM

I'd advocate to implement drawing the lines by code, i.e. QPainter::drawLine and friends. Then reuse the same code to draw icons.

Agreed, but that is too much for a single patch.

Rajeesh, nice patch! Can you get rid of the unicode symbols in the source code using one of the ways Tobias proposed? After that I would say that patch is good and can be applied.

knambiar updated this revision to Diff 57533.May 4 2019, 10:39 AM

Use QStringLiteral with Unicode code points for line ending symbols.

Thanks for the reviews!

sander added a comment.May 4 2019, 7:43 PM

Thanks! Will commit on tuesday, if nowbody objects.

sander accepted this revision.May 4 2019, 7:43 PM
This revision is now accepted and ready to land.May 4 2019, 7:43 PM
ngraham accepted this revision.May 5 2019, 7:23 PM

Looks great to me now!

sander added a comment.May 5 2019, 7:45 PM

I tried the circle line endings and they look a bit strange to me:

Is this really how they are supposed to look? If not, is this a poppler bug, or is Okular missing some further line ending setup code?

Is this really how they are supposed to look? If not, is this a poppler bug, or is Okular missing some further line ending setup code?

Needs to be fixed in poppler, I'll have a look soon. I originally didn't note what you describe, probably because I mostly tested MR106 with filled paths where the spurious line segments are not visible.

Did you note more misbehavior for other line ending styles besides circle?

sander added a comment.May 5 2019, 8:54 PM

At least squares and diamonds are incorrect, too.

At least squares and diamonds are incorrect, too.

Ok, so at least we're consistent in doing wrong :) I'll fix it.

Btw., this raises the question if setting / changing interior color (i.e. fill color of circle, diamond, ...) should be supported in Okular? It's defined in PDF and supported by poppler. This patch doesn't use it yet.

sander added a comment.May 6 2019, 4:24 AM

I think it should. But not in this patch.

pino added a subscriber: pino.May 6 2019, 6:34 AM
pino added inline comments.
ui/annotationwidgets.cpp
549–558

I think this does not play well with translations: there is nothing that says what for the leading spaces are, so translators can easily not have them in the translations, resulting in symbol and first word attached together.
There is also the issue that some of the strings are no easy to get, for example "Butt" (yes, without context), or "-/ Slash".

My proposal is the following:

  • create a QIconEngine that draws a specific character/string as icon; this also makes sure all the symbols and texts are properly aligned
  • leave the strings as simple strings with no spaces nor other parts, adding contexts to them

At least squares and diamonds are incorrect, too.

Ok, so at least we're consistent in doing wrong :) I'll fix it.

@sander Could you verify poppler MR 269 along with D20760? (sorry, can't do it myself since my Okular dev environment broke on last KF5 dependency bump, need to fix that first).

This revision was automatically updated to reflect the committed changes.
sander added a comment.May 7 2019, 9:10 AM

I committed the patch, but without any icons at all. Even without them I think the patch is very helpful. The icons can now be added at ease in a separate patch.

Rajeesh, thanks for the patch. I'd be happy to see more of your patches in the future.

I committed the patch, but without any icons at all. Even without them I think the patch is very helpful. The icons can now be added at ease in a separate patch.

Rajeesh, thanks for the patch. I'd be happy to see more of your patches in the future.

Thanks!
I plan to improve the annotation UI (it is something I use everyday), but I'm not familiar with the okular code yet. I would ask for help when needed.

aacid added a subscriber: aacid.May 8 2019, 11:04 PM

I can't go on holiday it seems, you sneak things past me :D

Please fix the connect to use the new connect syntax, and this "Only for PDF documents" tooltip/what'sthis is a pretty poors man fix, but oh well, i'll pretend i didn't see the code and go on trying to be happy with my life.

knambiar marked an inline comment as done.May 9 2019, 6:14 AM

Please fix the connect to use the new connect syntax,

Review #D21092 submitted.

and this "Only for PDF documents" tooltip/what'sthis is a pretty poors man fix, but oh well, i'll pretend i didn't see the code and go on trying to be happy with my life.

What would be a better alternative?

What would be a better alternative?

Implementing line end drawing right away for non-PDF isn't that hard, see comments above. Anything else would be waste of time IMO, that's why I think the "Only for PDF documents" tooltip is somehow justified. But I'm interested in other ideas/opinions. I'll try to prepare a patch during the next weeks for that. Feel free to be faster.

There's another problem we haven't discussed yet: Line endings work only if you have poppler >= 0.72 installed, else they will be silently ignored. Version 0.72 is quite recent, a lot of people won't have it because their distro ships an older version. Should we try to handle this? If yes, how? We can use cmake to detect poppler version, but I don't see an easy way to propagate the information from generator to UI at runtime.

There's another problem we haven't discussed yet: Line endings work only if you have poppler >= 0.72 installed, else they will be silently ignored. Version 0.72 is quite recent, a lot of people won't have it because their distro ships an older version. Should we try to handle this? If yes, how? We can use cmake to detect poppler version, but I don't see an easy way to propagate the information from generator to UI at runtime.

Indeed.
There's a few #ifdef HAVE_POPPLER_mm_nn checks in the code, may be we could adapt it for 0.72?

There's a few #ifdef HAVE_POPPLER_mm_nn checks in the code, may be we could adapt it for 0.72?

Yes, we can add this to PDF generator code, that's the easy part.

But we can't simply ifdef UI code based on properties of a particular generator, because the UI shall be generator agnostic. This means for the UI we need a runtime decision whether to enable the feature. As said, honestly I don't know of a good way how to propagate the information from generator to UI. Maybe Albert has a suggestion.