Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove internal event connection on SelectableEventedList #5339

Merged

Conversation

andy-sweet
Copy link
Member

@andy-sweet andy-sweet commented Nov 15, 2022

Description

We try to avoid internal event connections, likely to avoid circular references. This PR removes such an internal event connection and also closes that particular TODO/FIXME.

It also adds tests to ensure we still get the desired behavior of removing a deleted item from the current selection.

Type of change

  • Bug-fix (non-breaking change which fixes an issue)

References

An alternative to #4534

How has this been tested?

  • added new tests to cover expected behavior
  • all existing tests pass with my change

@github-actions github-actions bot added the tests Something related to our tests label Nov 15, 2022
Copy link
Contributor

@alisterburt alisterburt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thanks for taking over. For info I think there were/are two reasons for not liking the code as it was, internal event connection and connection to a lambda. As I understand it internal event connection is just a bit harder to reason about and connecting to lambdas is a potential cause of the cleanup bugs we see in Qt tests sometimes

@andy-sweet
Copy link
Member Author

andy-sweet commented Nov 15, 2022

Looks like there's a real failure related to LayerList. Will look into it later today.

@codecov
Copy link

codecov bot commented Nov 15, 2022

Codecov Report

Merging #5339 (1039d89) into main (c969968) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #5339      +/-   ##
==========================================
- Coverage   89.10%   89.06%   -0.04%     
==========================================
  Files         582      583       +1     
  Lines       49313    49336      +23     
==========================================
+ Hits        43938    43939       +1     
- Misses       5375     5397      +22     
Impacted Files Coverage Δ
napari/components/_tests/test_layers_list.py 100.00% <ø> (ø)
napari/components/layerlist.py 92.51% <100.00%> (+0.05%) ⬆️
napari/utils/events/_tests/test_selectable_list.py 100.00% <100.00%> (ø)
napari/utils/events/containers/_evented_list.py 95.90% <100.00%> (ø)
napari/utils/events/containers/_selectable_list.py 80.00% <100.00%> (+0.33%) ⬆️
napari/_tests/test_draw.py 36.84% <0.00%> (-63.16%) ⬇️
napari/layers/tracks/_track_utils.py 87.50% <0.00%> (-3.41%) ⬇️
napari/_qt/widgets/qt_color_swatch.py 71.54% <0.00%> (-3.26%) ⬇️
napari/utils/interactions.py 74.52% <0.00%> (-2.84%) ⬇️
napari/utils/info.py 80.00% <0.00%> (-2.23%) ⬇️
... and 6 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@andy-sweet
Copy link
Member Author

Looks like there's a real failure related to LayerList. Will look into it later today.

Fixed. Needed to call the super implementation of the deletion handler in LayerList's implementation.

@alisterburt alisterburt merged commit f2a2508 into napari:main Nov 16, 2022
@andy-sweet andy-sweet deleted the selectable-list-remove-internal-event branch November 16, 2022 15:34
@andy-sweet andy-sweet added this to the 0.5.0 milestone Dec 8, 2022
@Czaki Czaki modified the milestones: 0.5.0, 0.4.18 Jun 18, 2023
@Czaki Czaki added the maintenance PR with maintance changes, label Jun 18, 2023
Czaki pushed a commit that referenced this pull request Jun 18, 2023
* Add tests and fix with _process_delete_item

* Also remove from selection in LayerList
@Czaki Czaki mentioned this pull request Jun 18, 2023
Czaki pushed a commit that referenced this pull request Jun 19, 2023
* Add tests and fix with _process_delete_item

* Also remove from selection in LayerList
Czaki pushed a commit that referenced this pull request Jun 21, 2023
* Add tests and fix with _process_delete_item

* Also remove from selection in LayerList
Czaki pushed a commit that referenced this pull request Jun 21, 2023
* Add tests and fix with _process_delete_item

* Also remove from selection in LayerList
Czaki pushed a commit that referenced this pull request Jun 21, 2023
* Add tests and fix with _process_delete_item

* Also remove from selection in LayerList
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance PR with maintance changes, tests Something related to our tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants