Move the _is_created assignment to the top #5078
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5078 +/- ##
=======================================
Coverage ? 92.88%
=======================================
Files ? 618
Lines ? 56484
Branches ? 0
=======================================
Hits ? 52465
Misses ? 4019
Partials ? 0 ☔ View full report in Codecov by Sentry. |
|
Click here to download the docs artifacts |
|
Sounds like a reasonable change! However, should we then somehow expose this publicly a bit more clearly? Maybe by having a separate event for like |
|
To be honest I am not sure if a shape_completed event will make it more clearl as it furthers separates the way Point and Shapes layers and different Shapes within the Shapes layer behave. That way the information would be present in the set_data event itself? |
|
Yeah, maybe we can somehow stick this in the |
You could pass arbitrary kwargs when emmit event. But You need to update the docstring and all places where the event is emitted. |
|
I would suggest to go directly for the |
| # If there was no move event between two clicks value[1] is None | ||
| # and needs to be taken care of. | ||
| if value[1] is None: | ||
| value = layer._moving_value |
There was a problem hiding this comment.
Does any of this have a negative effect on the layer._last_cursor_position = np.array(event.pos) line in the add_vertex_to_path function? That's the only line in this function that didn't originally exist when this PR was first created. I assume the tests will pick up any problems?
There was a problem hiding this comment.
Yes, it won't be reset to None after completing drawing the path when clicking at the same position twice with Add path enabled.
There was a problem hiding this comment.
This seems not to be an issue anymore as clicking twice will always create a small line.
|
Discussed at the PR party, we'd like @melonora to weigh in also. |
|
If we do want to merge this, I pushed some changes to use |
I did have another look. The approach that @Czaki has suggested has been implemented in the meantime and is documented. The use case of the user is covered by this: If you add any shape now it will print That being said the question with this PR is more if the |
…en canvas clicked twice in same position
| mouse_release_callbacks(layer, event) | ||
|
|
||
| # If there was no move event between the two clicks, we expect value[1] must be None | ||
| assert layer.get_value(event.position, world=True)[1] is None |
There was a problem hiding this comment.
Is this a sufficient test for the concern you raised earlier @melonora?
There was a problem hiding this comment.
I suspect it is not, because it passes without any changes to the code, but I don't quite understand what we need to check if not this exact thing. Can you clarify things for me?
|
@melonora would you have time to update the PR with those changes? It would be good to get this over the line... |
|
ping @melonora |
|
Ok, tests are passing, there are three approvals, and it's been marked ready to merge for the last week (seems like plenty of time). Thank you @melonora! |
Description
Now playing with the Shapes layer, I found an issue with the
_is_creatingflag.I am creating a plugin which is connecting the
set_dataevent of the layer to identify when data is changed.However, during the creation of a
Pathshape (should also happen forPolygon) I want to wait until the editing is finished and skip the update if it is still in creation.Therefore, I use:
But it turns out, that I cannot rely on the
_is_creatingflag, because it is set too late in the drawing process.Therefore, I propose to move the assignment before the creation of the very first vertex.
Before (The
Falseafter the very first click on the left side):After:
Addition (The
Trueafter the very first click on the left side):If one clicks two times while
polygonorpathis selected, an error is thrown. I also added a test and a fix for this scenario.Type of change