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

tobiasdeiminger added a comment. > 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,

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

knambiar added a comment. 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

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

tobiasdeiminger added a comment. > 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

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

knambiar marked an inline comment as done. knambiar added a comment. In D20760#462747 , @aacid wrote: > Please fix the connect to use the new connect syntax, Review #D21092 submitted. > and this "Only for PDF documents"

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

aacid added a comment. 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

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

knambiar added a comment. 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,

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

This revision was automatically updated to reflect the committed changes. Closed by commit R223:db2dcdade399: Okular Annotation: add support for line ending style for Straight Line tool (authored by knambiar, committed by sander). CHANGED PRIOR TO COMMIT

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

sander added a comment. 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. REPOSITORY

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

tobiasdeiminger added a comment. >> 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

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

pino added inline comments. INLINE COMMENTS > annotationwidgets.cpp:549-558 > +m_termStyleCombo->addItem( QStringLiteral(u"\u25A1") + i18n( " Square" ) > ); > +m_termStyleCombo->addItem( QStringLiteral(u"\u25CB") + i18n( " Circle" ) > ); > +m_termStyleCombo->addItem(

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

sander added a comment. I think it should. But not in this patch. REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D20760 To: knambiar, #okular, #vdg, sander, ngraham Cc: sander, davidhurka, tobiasdeiminger, ngraham, okular-devel, joaonetto, tfella, darcyshen, aacid

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

tobiasdeiminger added a comment. 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 /

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

tobiasdeiminger added a comment. 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,

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

sander added a comment. I tried the circle line endings and they look a bit strange to me: F6810572: okular-strange-circle-line-ending.png Is this really how they are supposed to look? If not, is this a poppler bug, or is Okular missing some

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

ngraham accepted this revision. ngraham added a comment. Looks great to me now! REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D20760 To: knambiar, #okular, #vdg, sander, ngraham Cc: sander, davidhurka, tobiasdeiminger, ngraham, okular-devel, joaonetto, tfella,

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

sander added a comment. Thanks! Will commit on tuesday, if nowbody objects. REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D20760 To: knambiar, #okular, #vdg Cc: sander, davidhurka, tobiasdeiminger, ngraham, okular-devel, joaonetto, tfella, darcyshen, aacid

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

knambiar updated this revision to Diff 57533. knambiar added a comment. Use `QStringLiteral` with Unicode code points for line ending symbols. Thanks for the reviews! REPOSITORY R223 Okular CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D20760?vs=57001=57533 REVISION DETAIL

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

sander added a comment. > 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

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

tobiasdeiminger added a comment. In D20760#456328 , @davidhurka wrote: > Isn’t there a coding convention against non-ascii symbols in source code? There are Qt source code conventions

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

knambiar updated this revision to Diff 57001. knambiar added a comment. Add tooltips to clarify the line ending style works only for PDF documents. REPOSITORY R223 Okular CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D20760?vs=56942=57001 REVISION DETAIL

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

davidhurka added a comment. 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

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

davidhurka added a comment. 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. REPOSITORY R223 Okular REVISION

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

knambiar updated this revision to Diff 56942. knambiar added a comment. Place the symbols to left of ending style description for better alignment. REPOSITORY R223 Okular CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D20760?vs=56886=56942 REVISION DETAIL

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

knambiar added a comment. 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

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

ngraham added a comment. 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. REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D20760 To: knambiar, #okular, #vdg Cc: tobiasdeiminger,

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

tobiasdeiminger added a comment. 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

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

knambiar updated this revision to Diff 56886. knambiar added a comment. Merge the two commits (previous revision update had only the second change). REPOSITORY R223 Okular CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D20760?vs=56876=56886 REVISION DETAIL

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

knambiar added a comment. 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

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

tobiasdeiminger added a comment. In D20760#455266 , @knambiar wrote: > Add Unicode symbols to the line ending style. Didn’t find suitable symbols for ‘Right Closed Arrow’ and ‘Slash’. > F6790406: Okular-Straight-Line-End-Style-symbols.png

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

knambiar updated this revision to Diff 56876. knambiar added a comment. Add Unicode symbols to the line ending style. Didn’t find suitable symbols for ‘Right Closed Arrow’ and ‘Slash’. REPOSITORY R223 Okular CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D20760?vs=56790=56876

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

knambiar added a comment. 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

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

ngraham added a reviewer: VDG. ngraham added a comment. Hey, that's pretty cool! Any chance you could include a little icon/preview of the visual style for each item in the combobox? REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D20760 To: knambiar, #okular, #vdg

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

knambiar created this revision. knambiar added a reviewer: Okular. knambiar added a project: Okular. Herald added a subscriber: okular-devel. knambiar requested review of this revision. REVISION SUMMARY Poppler and Okular already have support for specifying Line End style (`TermStyle`) for the