Closed saarah815 closed 2 months ago
This is great, thanks! I've added a few comments throughout the code.
Also, I get an error if I remove the last layer using the
napari
layer manager. That's being caused by_on_sample_dropdown_index_changed
firing due to_update_dropdowns
causing themoving_image_index_change
signal to fire. This can be fixed by adding a check for an invalid layer index (-1) before running line 311-312.I wonder if we can take advantage of this interaction with signals and reduce some code duplication.
Perhaps,
_delete_atlas_layers
can be called inside of the_on_atlas_dropdown_index_changed
method. And the_handle_layer_deletion
method just calls thereset_atlas_combobox
method.So a layer is deleted, if the layer is the atlas or annotation layer then the
reset_atlas_combobox
method is called. This causes the_on_atlas_dropdown_changed
signal to fire with0
calling_on_atlas_dropdown_index_changed
, and there inside of theif index == 0:
block we call_delete_atlas_layers
.
Sounds good, will do - thank you!
Pushed all new changes. Just wanted to mention a few things:
In response to getting an error when all napari layers are deleted, adding a check for an invalid layer index in _on_sample_dropdown_index_changed
doesn't work as expected. Took me a while to figure out why but I think it's to do with the fact that each time the list of layers is changed in any way, update_sample_image_names
briefly causes the sample image dropdown menu to clear (line 101 in widgets/select_images_view.py
: self.available_sample_dropdown.clear()
). So checking for an invalid layer index and printing a warning when layer index = -1 basically ended up causing this warning to occur every time the list of layers is changed at all, rather than only when the list is empty. Tried working around this by adding a remove_item
method in class SampleImageComboBox(QComboBox)
in widgets/select_images_view.py
, which worked to remove items without clearing the whole list, but the warning was still appearing when unexpected. Ended up settling on adding a check in update_sample_image_names
to see if len(sample_image_names) == 0
and printing the warning then - this ended up being a much simpler fix and the warning now only appears if all layers have been deleted, as expected.
I kept the if
statements as successive if
s rather than if
and elif
blocks - I think because of all the repeated actions when deleting and adding layers, the code ends up short-circuiting and skipping actions as a result, which I noticed when combining some of the logic into if
and elif
blocks. Works as expected when I left it as successive if
s though, so I just kept it like that. But happy to change this if this was a misunderstanding on my end!
Other than those two points, all other changes have been made :)
Attention: Patch coverage is 88.23529%
with 6 lines
in your changes missing coverage. Please review.
Project coverage is 82.93%. Comparing base (
f690dea
) to head (c4e0cd5
).
Files | Patch % | Lines |
---|---|---|
brainglobe_registration/registration_widget.py | 86.95% | 6 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Before submitting a pull request (PR), please read the contributing guide.
Please fill out as much of this template as you can, but if you have any problems or questions, just leave a comment and we will help out :)
Description
What is this PR
Why is this PR needed?
"Currently, if a user deletes the atlas or moving image layer the plugin goes into undefined behaviour and will probably throw index out of bounds errors.
Need to make sure that layer deletions are communicated appropriately and the plugin can handle restarting the entire workflow without having to restart the plugin."
[#44]
What does this PR do?
Bug Fixes:
Additional Features:
References
Issue #44 Issue #53
How has this PR been tested?
Essentially user is able to delete layers as required and restart entire workflow without restarting plugin.
Is this a breaking change?
No.
Does this PR require an update to the documentation?
N/A
Checklist: