D10859: Improve layout of annotation configuraton dialogs

[Rajeesh K Nambiar]

knambiar added a comment.


  Tested, LGTM.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D10859

To: simgunz, #okular, #vdg, ngraham
Cc: aacid, okular-devel, knambiar, ngraham, joaonetto, tfella, darcyshen

D21416: Add icons for line annotation arrow styles to combo box

[Rajeesh K Nambiar]

knambiar added a comment.


  Super!

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D21416

To: tobiasdeiminger, #okular
Cc: knambiar, ngraham, okular-devel, joaonetto, tfella, darcyshen, aacid

D10859: Improve layout of annotation configuraton dialogs

[Rajeesh K Nambiar]

knambiar added a comment.
Herald added a subscriber: okular-devel.


  Would be easier to test this change if it is rebased against current master, 
as many changes landed in `annotationwidgets.{h,cpp}` recently.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D10859

To: simgunz, #okular, #vdg, ngraham
Cc: okular-devel, knambiar, ngraham, joaonetto, tfella, darcyshen, aacid

D15580: New annotation toolbar

[Rajeesh K Nambiar]

knambiar added a comment.


  This change doesn't cleanly apply on the current master branch. Would you 
rebase the changes, or is there a branch it could be built against (I don't see 
annotation-toolbar branch yet)?

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular
Cc: knambiar, ngraham, tobiasdeiminger, okular-devel, joaonetto, tfella, 
darcyshen, aacid

D21332: Okular Annotation: add line start/end style config only for Straight Line tool

[Rajeesh K Nambiar]

knambiar updated this revision to Diff 58625.
knambiar marked 2 inline comments as done.
knambiar added a comment.


  Add `Q_ASSERT` for the configs.

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21332?vs=58571&id=58625

REVISION DETAIL
  https://phabricator.kde.org/D21332

AFFECTED FILES
  ui/annotationwidgets.cpp
  ui/annotationwidgets.h

To: knambiar, #okular
Cc: tobiasdeiminger, okular-devel, joaonetto, tfella, ngraham, darcyshen, aacid

D21332: Okular Annotation: add line start/end style config only for Straight Line tool

[Rajeesh K Nambiar]

knambiar added inline comments.

INLINE COMMENTS

> tobiasdeiminger wrote in annotationwidgets.cpp:499
> Checking a polygons Inner color checkbox now causes segfault. It's because 
> `LineAnnotationWidget::applyChanges` accesses `m_startStyleCombo` and 
> `m_endStyleCombo` unconditionally. But in case of `m_lineType == 1` that 
> QComboBoxes have never been constructed. Access should be guarded by `if ( 
> m_lineType == 0 )`, and probably an additional `nullptr` check.

Is the `nullptr` check really necessary?

> tobiasdeiminger wrote in annotationwidgets.cpp:528
> Are the leading whitespaces in " Square", " Circle", and so on, intentional?

No, it was an oversight from previous local patch. Fixed now.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D21332

To: knambiar, #okular
Cc: tobiasdeiminger, okular-devel, joaonetto, tfella, ngraham, darcyshen, aacid

D21332: Okular Annotation: add line start/end style config only for Straight Line tool

[Rajeesh K Nambiar]

knambiar updated this revision to Diff 58571.
knambiar marked 3 inline comments as done.
knambiar edited the summary of this revision.
knambiar added a comment.


  Fix crash, `m_{start,end}StyleCombo` are guarded by `m_lineType == 0` check 
for Straight Line tool.

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21332?vs=58531&id=58571

REVISION DETAIL
  https://phabricator.kde.org/D21332

AFFECTED FILES
  ui/annotationwidgets.cpp

To: knambiar, #okular
Cc: tobiasdeiminger, okular-devel, joaonetto, tfella, ngraham, darcyshen, aacid

D21332: Okular Annotation: add line start/end style config only for Straight Line tool

[Rajeesh K Nambiar]

knambiar added a comment.


  In D21332#468893 , 
@tobiasdeiminger wrote:
  
  > Btw., can we do polylines yet? The only way I found was to tweak engine 
points attribute for tool type straight-line in `~/.config/okularpartrc`.
  
  
  Thanks for the PDF reference document. I will do some research on PolyLine 
(which //seems// to be implemented in the backend).

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D21332

To: knambiar, #okular
Cc: tobiasdeiminger, okular-devel, joaonetto, tfella, ngraham, darcyshen, aacid

D21332: Okular Annotation: add line start/end style config only for Straight Line tool

[Rajeesh K Nambiar]

knambiar updated this revision to Diff 58531.
knambiar added a comment.


  Rebase against current master

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21332?vs=58459&id=58531

REVISION DETAIL
  https://phabricator.kde.org/D21332

AFFECTED FILES
  ui/annotationwidgets.cpp

To: knambiar, #okular
Cc: tobiasdeiminger, okular-devel, joaonetto, tfella, ngraham, darcyshen, aacid

D21332: Okular Annotation: add line start/end style config only for Straight Line tool

[Rajeesh K Nambiar]

knambiar created this revision.
knambiar added a reviewer: Okular.
Herald added a project: Okular.
Herald added a subscriber: okular-devel.
knambiar requested review of this revision.

REVISION SUMMARY
  “Inner Color” configuration of Polygon tool was overlapping with the line 
start/end styles intended for only Straight Line tool. Fix it.

TEST PLAN
  1. Configure annotations
  2. Create/Edit Polygon tool
  3. Observe that no Line Start/End styles are visible
  4. Create/Edit Straight Line tool
  5. Observe that line start/end styles can be configured

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D21332

AFFECTED FILES
  ui/annotationwidgets.cpp

To: knambiar, #okular
Cc: okular-devel, joaonetto, tfella, ngraham, darcyshen, aacid

D21238: Okular Annotation: add support for line start style for Straight Line tool

[Rajeesh K Nambiar]

knambiar added a comment.


  In D21238#468127 , 
@tobiasdeiminger wrote:
  
  > @knambiar The config dialog for polygon annotations has seemingly regressed 
with recent changes:
  
  
  Ouch. Going to fix it in a new review.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D21238

To: knambiar, #okular, tobiasdeiminger
Cc: ngraham, tobiasdeiminger, okular-devel, joaonetto, tfella, darcyshen, aacid

D21238: Okular Annotation: add support for line start style for Straight Line tool

[Rajeesh K Nambiar]

knambiar updated this revision to Diff 58236.
knambiar added a comment.


  Whitespace fix

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21238?vs=58235&id=58236

REVISION DETAIL
  https://phabricator.kde.org/D21238

AFFECTED FILES
  conf/editannottooldialog.cpp
  ui/annotationwidgets.cpp
  ui/annotationwidgets.h
  ui/pageviewannotator.cpp

To: knambiar, #okular
Cc: ngraham, tobiasdeiminger, okular-devel, joaonetto, tfella, darcyshen, aacid

D21238: Okular Annotation: add support for line start style for Straight Line tool

[Rajeesh K Nambiar]

knambiar updated this revision to Diff 58235.
knambiar marked an inline comment as done.
knambiar added a comment.


  Use `const QString &` instead of `auto` in range loop

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21238?vs=58157&id=58235

REVISION DETAIL
  https://phabricator.kde.org/D21238

AFFECTED FILES
  conf/editannottooldialog.cpp
  ui/annotationwidgets.cpp
  ui/annotationwidgets.h
  ui/pageviewannotator.cpp

To: knambiar, #okular
Cc: ngraham, tobiasdeiminger, okular-devel, joaonetto, tfella, darcyshen, aacid

D21092: Okular Annotation: use the new signal-slot connect syntax

[Rajeesh K Nambiar]

knambiar added a comment.


  Review D21238  created for the line start 
style.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D21092

To: knambiar, #okular, aacid
Cc: sander, tobiasdeiminger, aacid, okular-devel, joaonetto, tfella, ngraham, 
darcyshen

D21238: Okular Annotation: add support for line start style for Straight Line tool

[Rajeesh K Nambiar]

knambiar created this revision.
knambiar added a reviewer: Okular.
Herald added a project: Okular.
Herald added a subscriber: okular-devel.
knambiar requested review of this revision.

REVISION SUMMARY
  Similar to the line ending style, add support for line start style for the 
Straight Line annotation tool

TEST PLAN
  1. Go to Configure annotations
  2. Create (or edit existing) Straight Line tool
  3. Set the ‘Line Start’ option on Style and Apply
  4. Use the Straight Line tool to draw a line and check the line starting 
style.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D21238

AFFECTED FILES
  conf/editannottooldialog.cpp
  ui/annotationwidgets.cpp
  ui/annotationwidgets.h
  ui/pageviewannotator.cpp

To: knambiar, #okular
Cc: okular-devel, joaonetto, tfella, ngraham, darcyshen, aacid

D21092: Okular Annotation: use the new signal-slot connect syntax

[Rajeesh K Nambiar]

knambiar marked an inline comment as done.
knambiar added a comment.


  In D21092#464202 , 
@tobiasdeiminger wrote:
  
  > @knambiar
  >  Rajeesh, would you be around for another patch? PDF / poppler allow to 
draw arrows on both ends of a line (aka start style, end style). Your patches 
currently target only the line end. Would you implement "start style" in the UI 
too?
  
  
  Certainly. I did notice the `startStyle` as well, but decided to ignore it 
for the time being.
  I'm afraid we'll have similar objections to the `endStyle`, though, 
specifically “PDF only” — would that be okay?

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D21092

To: knambiar, #okular, aacid
Cc: tobiasdeiminger, aacid, okular-devel, joaonetto, tfella, ngraham, darcyshen

D21092: Okular Annotation: use the new signal-slot connect syntax

[Rajeesh K Nambiar]

knambiar updated this revision to Diff 57892.
knambiar added a comment.


  Drop the combobox change

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21092?vs=57843&id=57892

REVISION DETAIL
  https://phabricator.kde.org/D21092

AFFECTED FILES
  ui/annotationwidgets.cpp

To: knambiar, #okular
Cc: aacid, okular-devel, joaonetto, tfella, ngraham, darcyshen

D21092: Okular Annotation: use the new signal-slot connect syntax

[Rajeesh K Nambiar]

knambiar updated this revision to Diff 57843.
knambiar added a comment.


  Use `QOverload` instead of `static_cast`

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21092?vs=57791&id=57843

REVISION DETAIL
  https://phabricator.kde.org/D21092

AFFECTED FILES
  ui/annotationwidgets.cpp

To: knambiar, #okular
Cc: aacid, okular-devel, joaonetto, tfella, ngraham, darcyshen

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

[Rajeesh K Nambiar]

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

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D20760

To: knambiar, #okular, #vdg, sander, ngraham
Cc: aacid, pino, sander, davidhurka, tobiasdeiminger, ngraham, okular-devel, 
joaonetto, tfella, darcyshen

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

[Rajeesh K Nambiar]

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

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D20760

To: knambiar, #okular, #vdg, sander, ngraham
Cc: aacid, pino, sander, davidhurka, tobiasdeiminger, ngraham, okular-devel, 
joaonetto, tfella, darcyshen

D21092: Okular Annotation: use the new signal-slot connect syntax

[Rajeesh K Nambiar]

knambiar created this revision.
knambiar added a reviewer: Okular.
Herald added a project: Okular.
Herald added a subscriber: okular-devel.
knambiar requested review of this revision.

REVISION SUMMARY
  Use the new signal-slot connect syntax

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D21092

AFFECTED FILES
  ui/annotationwidgets.cpp

To: knambiar, #okular
Cc: okular-devel, joaonetto, tfella, ngraham, darcyshen, aacid

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

[Rajeesh K Nambiar]

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

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D20760

To: knambiar, #okular, #vdg, sander, ngraham
Cc: pino, sander, davidhurka, tobiasdeiminger, ngraham, okular-devel, 
joaonetto, tfella, darcyshen, aacid

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

[Rajeesh K Nambiar]

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&id=57533

REVISION DETAIL
  https://phabricator.kde.org/D20760

AFFECTED FILES
  conf/editannottooldialog.cpp
  ui/annotationwidgets.cpp
  ui/annotationwidgets.h
  ui/pageviewannotator.cpp

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

[Rajeesh K Nambiar]

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&id=57001

REVISION DETAIL
  https://phabricator.kde.org/D20760

AFFECTED FILES
  conf/editannottooldialog.cpp
  ui/annotationwidgets.cpp
  ui/annotationwidgets.h
  ui/pageviewannotator.cpp

To: knambiar, #okular, #vdg
Cc: davidhurka, tobiasdeiminger, ngraham, okular-devel, joaonetto, tfella, 
darcyshen, aacid

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

[Rajeesh K Nambiar]

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&id=56942

REVISION DETAIL
  https://phabricator.kde.org/D20760

AFFECTED FILES
  conf/editannottooldialog.cpp
  ui/annotationwidgets.cpp
  ui/annotationwidgets.h
  ui/pageviewannotator.cpp

To: knambiar, #okular, #vdg
Cc: tobiasdeiminger, ngraham, okular-devel, joaonetto, tfella, darcyshen, aacid

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

[Rajeesh K Nambiar]

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 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 be added only for PDF documents?

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D20760

To: knambiar, #okular, #vdg
Cc: tobiasdeiminger, ngraham, okular-devel, joaonetto, tfella, darcyshen, aacid

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

[Rajeesh K Nambiar]

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&id=56886

REVISION DETAIL
  https://phabricator.kde.org/D20760

AFFECTED FILES
  conf/editannottooldialog.cpp
  ui/annotationwidgets.cpp
  ui/annotationwidgets.h
  ui/pageviewannotator.cpp

To: knambiar, #okular, #vdg
Cc: tobiasdeiminger, ngraham, okular-devel, joaonetto, tfella, darcyshen, aacid

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

[Rajeesh K Nambiar]

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

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D20760

To: knambiar, #okular, #vdg
Cc: tobiasdeiminger, ngraham, okular-devel, joaonetto, tfella, darcyshen, aacid

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

[Rajeesh K Nambiar]

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&id=56876

REVISION DETAIL
  https://phabricator.kde.org/D20760

AFFECTED FILES
  ui/annotationwidgets.cpp

To: knambiar, #okular, #vdg
Cc: ngraham, okular-devel, joaonetto, tfella, darcyshen, aacid

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

[Rajeesh K Nambiar]

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

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D20760

To: knambiar, #okular, #vdg
Cc: ngraham, okular-devel, joaonetto, tfella, darcyshen, aacid

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

[Rajeesh K Nambiar]

knambiar edited the summary of this revision.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D20760

To: knambiar, #okular
Cc: okular-devel, joaonetto, tfella, ngraham, darcyshen, aacid

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

[Rajeesh K Nambiar]

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 Straight Line tool. Expose the functionality in 
configuration and hook up the correct slots.

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.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D20760

AFFECTED FILES
  conf/editannottooldialog.cpp
  ui/annotationwidgets.cpp
  ui/annotationwidgets.h
  ui/pageviewannotator.cpp

To: knambiar, #okular
Cc: okular-devel, joaonetto, tfella, ngraham, darcyshen, aacid