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

[Tobias Deiminger]

This revision was automatically updated to reflect the committed changes.
Closed by commit R223:b1c30cd0edd3: Okular Annotation: add line start/end style 
config only for Straight Line tool (authored by knambiar, committed by 
tobiasdeiminger).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D21332?vs=58625=58673#toc

REPOSITORY
  R223 Okular

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

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

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

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

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

[Tobias Deiminger]

tobiasdeiminger accepted this revision.
tobiasdeiminger added a comment.
This revision is now accepted and ready to land.


  Thanks. If nobody objects I'll land this tomorrow.

REPOSITORY
  R223 Okular

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

To: knambiar, #okular, tobiasdeiminger
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 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=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

[Tobias Deiminger]

tobiasdeiminger added inline comments.

INLINE COMMENTS

> knambiar wrote in annotationwidgets.cpp:499
> Is the `nullptr` check really necessary?

No, I'd accept anyways. I just think the implication "object pointer != 
nullptr" => "object valid" is more obvious to new contributors than "some 
member == some magic number" => "object valid".

How about adding a `Q_ASSERT`as we did it in 
`TextAnnotationWidget::applyChanges`? If you go for it, please also initialize 
members (as done for `TextAnnotationWidget`in `annotationwidgets.h`).

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

[Tobias Deiminger]

tobiasdeiminger added inline comments.

INLINE COMMENTS

> annotationwidgets.cpp:499
>  {
>  m_useColor = new QCheckBox( i18n( "Inner color:" ), gb2 );
>  gridlay2->addWidget( m_useColor, 1, 0 );

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.

> annotationwidgets.cpp:528
> +
> +for ( const QString : { i18n( " Square" ), i18n( " Circle" ), 
> i18n( " Diamond" ), i18n( " Open Arrow" ), i18n( " Closed Arrow" ),
> +i18n( " None" ), i18n( " Butt" ), i18n( " Right Open 
> Arrow" ), i18n( " Right Closed Arrow" ), i18n( "Slash" ) })

Are the leading whitespaces in " Square", " Circle", and so on, intentional?

> annotationwidgets.cpp:542
>  else if ( m_lineType == 1 )
>  {

Is this `else if ( m_lineType == 1 )` branch really necessary? Looks like the 
code within could just be moved to the previous `if ( m_lineType == 1 )` at L. 
497.

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

[Tobias Deiminger]

tobiasdeiminger added a comment.


  `arc patch` fails, could you please rebase on current master? You may also 
want to add CCBUG 
:
 381629 to the description.
  
  Besides that your changes look reasonable. I was first a bit in doubt about 
removing lineends from polygon widget, because in PDF speak Polyline is more 
related to Polygon than to Line annotation (see PDF32000_2008.pdf 
 
chapter 12.5.6.9). And polylines //can have// line endings. But Okular seems to 
go a different route, where polygons are always closed shapes (acc. to Okular 
Handbook), and polyline is more related to `ToolStraightLine` than to 
`ToolPolygon`.
  
  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`.

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