Communicate napari layer deletions appropriately to the plugin

[saarah815]

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

• Bug fix
• Addition of a new feature
• Other

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:

• Deleting moving image sets self._moving_image and self._moving_image_data_backup to None.
• Deleting moving image removes it from the sample image dropdown menu and the next item in dropdown menu (equivalent to list of layers) is automatically selected as moving image.
• Deleting either one of the atlas layers (Reference or Annotations) automatically deletes the other layer, and clears any class attributes related to the atlas, sets self._atlas back to None, and sets the index for the atlas selection combobox back to 0.

Additional Features:

• If user deletes atlas and tries to re-run registration with atlas selection combobox index == 0, message box appears:

• If user deletes moving image and the new moving image is automatically set to the atlas layer (i.e. user forgets to change this / add a new moving image layer), message box appears:

References

Issue #44
Issue #53

How has this PR been tested?

• Tested by deleting the moving layer only --> works as expected.
• Tested by deleting the atlas layers only --> works as expected.
• Tested by deleting both layers --> works as expected.

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:

• The code has been tested locally
• Tests have been added to cover all new functionality (unit & integration)
• The documentation has been updated to reflect any changes
• The code has been formatted with pre-commit

[IgorTatarnikov]

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 the moving_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 the reset_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 with 0 calling _on_atlas_dropdown_index_changed, and there inside of the if index == 0: block we call _delete_atlas_layers.

[IgorTatarnikov]

As above you can check for None instead of hasattr (you can go through and fix this wherever it occurs)

[saarah815]

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 the moving_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 the reset_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 with 0 calling _on_atlas_dropdown_index_changed, and there inside of the if index == 0: block we call _delete_atlas_layers.

Sounds good, will do - thank you!

[saarah815]

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 ifs 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 ifs 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 :)

[IgorTatarnikov]

Thanks for addressing the comments! Just a few small things left. I just noticed that the tests seem to have broken, they enter a loop at the top of test_registration_widget. We'll have to take a look at this before merging.

[codecov]

Codecov Report

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 ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev      #49      +/-   ##
==========================================
+ Coverage   80.96%   82.93%   +1.96%     
==========================================
  Files           8        8              
  Lines         541      580      +39     
==========================================
+ Hits          438      481      +43     
+ Misses        103       99       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[IgorTatarnikov]

Looks great, thank you!