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

In D20760#455010, @ngraham wrote:

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).

In D20760#455266, @knambiar wrote:

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?

In D20760#455290, @tobiasdeiminger wrote:

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.

In D20760#455307, @knambiar wrote:

In D20760#455290, @tobiasdeiminger wrote:

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.

In D20760#455351, @tobiasdeiminger wrote:

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”?

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.

In D20760#455512, @knambiar wrote:

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.

In D20760#456328, @davidhurka wrote:

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.

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.

Thanks! Will commit on tuesday, if nowbody objects.

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?

In D20760#461261, @sander wrote:

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?

At least squares and diamonds are incorrect, too.

In D20760#461314, @sander wrote:

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.

I think it should. But not in this patch.

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).

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.

In D20760#462025, @sander wrote:

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.

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.

In D20760#462747, @aacid wrote:

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.

In D20760#462808, @tobiasdeiminger wrote:

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.