“Inner Color” configuration of Polygon tool was overlapping with the line start/end styles intended for only Straight Line tool. Fix it.
CCBUG: 381629
tobiasdeiminger |
Okular |
“Inner Color” configuration of Polygon tool was overlapping with the line start/end styles intended for only Straight Line tool. Fix it.
CCBUG: 381629
Lint Skipped |
Unit Tests Skipped |
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.
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. |
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). |