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

Authored by knambiar on May 22 2019, 9:44 AM.

Details

Summary

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

CCBUG: 381629

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

Diff Detail

Repository
R223 Okular
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
knambiar created this revision.May 22 2019, 9:44 AM
Restricted Application added a project: Okular. · View Herald TranscriptMay 22 2019, 9:44 AM
Restricted Application added a subscriber: okular-devel. · View Herald Transcript
knambiar requested review of this revision.May 22 2019, 9:44 AM

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.

knambiar updated this revision to Diff 58531.May 23 2019, 7:06 AM

Rebase against current master

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

ui/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.

528

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

542

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.

knambiar updated this revision to Diff 58571.May 24 2019, 5:44 AM
knambiar marked 3 inline comments as done.
knambiar edited the summary of this revision. (Show Details)

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

ui/annotationwidgets.cpp
499

Is the nullptr check really necessary?

528

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

ui/annotationwidgets.cpp
499

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_ASSERTas we did it in TextAnnotationWidget::applyChanges? If you go for it, please also initialize members (as done for TextAnnotationWidgetin annotationwidgets.h).

knambiar updated this revision to Diff 58625.May 25 2019, 4:40 AM
knambiar marked 2 inline comments as done.

Add Q_ASSERT for the configs.

tobiasdeiminger accepted this revision.May 25 2019, 9:23 AM

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

This revision is now accepted and ready to land.May 25 2019, 9:23 AM
This revision was automatically updated to reflect the committed changes.