Remove null (gap) to prevent validation errors

[lehecht]

If single frame and clip annotations are linked and are not touching each other, then null is added. When deleting the single frame annotation, delete also the null.

Fix #488

[mzur]

There is still an error if I put the single-frame annotation before the multi-frame annotation.

[lehecht]

I found a related bug.
If you delete an annotation within a multiframe annotation, which is bordered by single frame annotations on each side, you will get the error message Invalid number of points for shape Point!. It occurs for all shapes, with whom you can create a multiframe annotation.

[mzur]

This works, thanks!

[mzur]

If you delete an annotation within a multiframe annotation, which is bordered by single frame annotations on each side, you will get the error message Invalid number of points for shape Point!.

Could you please also fix this in this PR?

[lehecht]

@mzur you can check it out now.

[mzur]

I have pushed the implementation of my idea and I still think it works. Can you please test it, too?

[lehecht]

@mzur it works. And now I understood what you meant. I was really confused before. Thanks!

[mzur]

Sorry, I'll try to be more clear next time 😉 Do we still need the other change? If yes, please move it to the VideoAnnotation model.

[mzur]

Thanks! I'm sorry to drag this on even more but could you now please add a test for the new validation behavior? The test should be written in a way that would fail before the change and pass with the change.

[lehecht]

The test should be written in a way that would fail before the change and pass with the change.

What do you mean by that? The change is implemented within the model, so it is always tested if the array is empty or not.

[mzur]

I would like to have a test like this one that checks the new behavior of validating an annotation with a gap. If the test would be executed before the changes of this PR, it should fail. After the changes of this PR, the test should pass.

[lehecht]

For me it sounds like, you want something like this. That is why I'm still confused, because the whole test will always fail.

$this->model->shape_id = Shape::pointId();
$this->model->points = [[]];
$this->model->validatePoints(); // does not fail
$this->expectException(Exception::class);
$this->model->points = [[]];
$this->model->validatePoints(); // should fail, but does not

[mzur]

I meant something like this: 16d1f2a

Before your change, this would fail, although it is perfectly valid. We could even extend the validation so it checks that there are no consecutive empty arrays or there is a mismatch between the empty array position and the null but I think we don't have to overengineer this now.

[mzur]

All good now 👍