Pencil: Pencil2D crashes when activating Bezier option in Polyline tool

[J5lx]

--Issue Summary--

When activating the Bezier option of the Polyline tool, Pencil2D crashes.

--Actual Results--

Pencil2D crashes due to an assertion. Relevant terminal output:

ASSERT: "false" in file ../../app/tooloptionwidget.cpp, line 138
zsh: abort (core dumped)  build.Debug/bin/Pencil2D

--Expected Results--

Pencil2D does not crash.

--Steps to reproduce--

1. Start the program
2. Select the polyline tool
3. Activate the Bezier checkbox in the tool options

--System Information--

• Pencil2D Version: git master (4c21bb10ac34ef5e45d4879e8163ced9dd919dd0)

• Operating System: Arch Linux

[J5lx]

It’s rather late here and I’m not utterly familiar with the affected code, so I’ll not be looking into this today anymore. Everyone, feel free to investigate.

[romeojulietthotel]

Seems safe to comment out the Q_ASSERT until the intent of the Q_ASSERT is determined. Commenting that out caused no problems for me and bezier curves seem to work properly.

[J5lx]

False assertions indicate code paths that should never be reached, and in the default branch of a switch statement in particular they are triggered when the switch statement didn’t account for a value that it should have accounted for. So even if commenting it out causes no apparent harm, it still indicates a bug in the program that should be fixed, not worked around.

[romeojulietthotel]

SIGABRT seems extreme form of reminder to fix the code and more so since without that the bezier seems to work fine.

[J5lx]

Yeah, it’s extreme, but that’s okay because the idea behind assertions is that the program apparently reached a state that it was never supposed to reach, which therefore can not be expected to be handled correctly by the rest of the program. If such a condition went by unnoticed because of, say, an assertion that was commented out, it might cause the program to misbehave in subtle and hard-to-debug ways in some cases. If the program is aborted, on the other hand, that invalid state cannot possibly go unnoticed and will require a proper fix. However, it should be noted that assertions generally indicate errors that are caused by inherent mistakes made by the programmer, therefore they are normally disabled in release builds since those are assumed to be well-tested and free of such mistakes. That means that if we made a release out of our current code, that release would behave as if we had commented out the assertion the way you did. In other words, no harm done to users who stick with our stable releases. Nightly builds and development builds are intended for people who’d like to test the program or work on it, so it’s better when these people can notice problems early thanks to crashes/aborts, even if that’s not particularly elegant.

[romeojulietthotel]

Haha, ok I just need to build with CONFIG+=NDEBUG. Didn't know that. Thanks for taking the time.

[CandyFace]

The reason it fails is because there's no implementation to update the action of setUseBezier in ToolOptionWidget + the fact that the enum BEZIER isn't part of the switch case. Therefore when you press the bezier button in the toolbox, the program tries to update the action of the button but it will fail because there's no case that matches its type.

Conclusion: The bezier button doesn't update it's enable/disable state.

[romeojulietthotel]

Right, the fix is to add the plumbing for the radio button. The bezier feature must have been added as an all or nothing feature since it was implemented without a way to disable/enable.