onFree callback for uiControl instances #250
Conversation
| int unitTestTeardown(void **_state) | ||
| { | ||
| struct state *state = *_state; | ||
|
|
||
| uiControlOnFree(uiControl(state->w), unitControlOnFree, state); |
There was a problem hiding this comment.
This test doesn't prove that unitControlOnFree() is ever actually called.
There was a problem hiding this comment.
Indeed; I'm unsure how to address it without some sort of state tracking. Since there's already a state variable, I've added a flag to its struct definition and using it to explicitly check whether the onFree callback is indeed called or not.
There was a problem hiding this comment.
I still struggle to understand why the Ardillo PHP binding needs an OnFree callback. How has this not come up as an issue for other bindings? Do they not care and simply leak memory? Or is the need for OnFree rather the need stemming from design choices made within the Ardillo binding? Because I would be surprised if PHP is too limited in language features to do without - when compared to other bindings.
Apart from that, thanks for providing unit tests!
This is sadly however not the way to test this.
For guaranteeing the invocation of a function call you want to use expect_function_calls(unitControlOnFree, 1); and function_called();. I provided hints in the PR review on how to do so with cmocka.
I personally am not really a fan of intermingling this testing with the setup and tear down of each individual test. I would prefer this to be done for each control individually. There may be a discussion to be had around this in general. I am facing the same dilemma in #245 which implements drag and drop for each uiControl.
Maybe there is a way to create a controlUnitTest setup and teardown that can be called by each individual uiControl as the first unit test case? Otherwise we'll have the entire test suite break in case something around the onFree callback goes wrong.
| @@ -31,6 +31,7 @@ int progressBarRunUnitTests(void); | |||
| struct state { | |||
| uiWindow *w; | |||
| uiControl *c; | |||
| int onFreeCalled; | |||
There was a problem hiding this comment.
| int onFreeCalled; |
| uiWindowSetChild(state->w, uiControl(state->c)); | ||
| uiControlShow(uiControl(state->w)); | ||
| //uiMain(); | ||
| uiMainSteps(); | ||
| uiMainStep(1); | ||
| uiControlDestroy(uiControl(state->w)); | ||
| assert_int_equal(state->onFreeCalled, 1); |
There was a problem hiding this comment.
| assert_int_equal(state->onFreeCalled, 1); |
| int unitTestTeardown(void **_state) | ||
| { | ||
| struct state *state = *_state; | ||
|
|
||
| uiControlOnFree(uiControl(state->w), unitControlOnFree, state); | ||
| state->onFreeCalled = 0; |
There was a problem hiding this comment.
| state->onFreeCalled = 0; |
| assert_non_null(c); | ||
| assert_non_null(data); | ||
| assert_ptr_equal(c, ((struct state *)data)->w); | ||
| ((struct state *)data)->onFreeCalled = 1; |
There was a problem hiding this comment.
| assert_non_null(c); | |
| assert_non_null(data); | |
| assert_ptr_equal(c, ((struct state *)data)->w); | |
| ((struct state *)data)->onFreeCalled = 1; | |
| function_called(); |
| int unitTestTeardown(void **_state) | ||
| { | ||
| struct state *state = *_state; | ||
|
|
||
| uiControlOnFree(uiControl(state->w), unitControlOnFree, state); | ||
| state->onFreeCalled = 0; | ||
| uiWindowSetChild(state->w, uiControl(state->c)); | ||
| uiControlShow(uiControl(state->w)); | ||
| //uiMain(); | ||
| uiMainSteps(); | ||
| uiMainStep(1); |
There was a problem hiding this comment.
| uiMainStep(1); | |
| uiMainStep(1); | |
| expect_function_calls(unitControlOnFree, 1); |
This pull request concerns the destroying of UI controls, the issue being some controls could be destroyed outside of the purview of the embedding application, possibly leading to memory safety issues. In order to keep the implementing application aware of controls' lifecycle/status, the PR implements an
onFreecallback foruiControlstructures (and its corresponding context asonFreeData).This topic was originally discussed here.