Emit event from Points data setter

[jni]

Closes #6116

Description

Adds back an event emission from the Points layer data setter. Commits summary:

• Add failing test for layer data setter event
• Emit event on points data setter

Type of change

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

[codecov]

Codecov Report

Merging #6117 (3be54a7) into main (9186881) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 3be54a7 differs from pull request most recent head 87331e1. Consider uploading reports for the commit 87331e1 to get more accurate results

@@           Coverage Diff           @@
##             main    #6117   +/-   ##
=======================================
  Coverage   91.61%   91.62%           
=======================================
  Files         579      579           
  Lines       50636    50656   +20     
=======================================
+ Hits        46392    46414   +22     
+ Misses       4244     4242    -2     

Files Changed	Coverage Δ
napari/layers/points/_tests/test_points.py	100.00% <100.00%> (ø)
napari/layers/points/points.py	97.74% <100.00%> (+<0.01%)	⬆️

... and 1 file with indirect coverage changes

[melonora]

I think I found out why I removed the line you mentioned. When testing using this code:

import napari


def test(event):
    print(event.data_indices)
    print(event.vertex_indices)
    print(event.action)

viewer = napari.Viewer()
viewer.add_points()
viewer.layers[0].events.data.connect(test)
napari.run()

if you now start adding points you see that each time there are 2 emits. One coming from the data.setter which actually gives wrong information. I am not certain why in the shapes layer having the line self.events.data(value=self.data) in the setter does not gives issues while something similar in Points does. I will have a look after the break, but at least it seems to be the case that the data setter gets called every time the data is changed.

[jni]

Ah, that's ok, we've encountered situations like this in the past — the trick is then to create a "set data quietly" private method that all the emitting methods use, and then the setter simply uses that method and then emits.

The editing methods are using the setter because NumPy arrays aren't expandable — we could/should optimise that with an expandable array but it's never happened 😅, because NumPy is fast enough when creating an array with a million points anyway! 😅

[melonora]

Ah, that's ok, we've encountered situations like this in the past — the trick is then to create a "set data quietly" private method that all the emitting methods use, and then the setter simply uses that method and then emits.

The editing methods are using the setter because NumPy arrays aren't expandable — we could/should optimise that with an expandable array but it's never happened 😅, because NumPy is fast enough when creating an array with a million points anyway! 😅

If I understand correctly, the difference with Shapes is that here the data is edited in layer._data_view which is still separate from the data.setter. Am I correct?

[jni]

@melonora here you go, added a regression test and everything. 😊

[Czaki]

I think that we should use unittest Mock in tests instead of append on list.

[jni]

... because?

I'm not really sure how Mock would improve things here. As far as I can tell the list tests exactly what it purports to test?

[melonora]

Thanks @jni! I am happy with the change, but do have one question where I am not certain whether it would be something to change as part of this PR. The data event in the data.setter of shapes is different now from the data.setter for points. I would have data emitted in the same manner, where for the consistent API I would have the data.setter of Points be adapted by Shapes.

[jni]

@melonora i think that should come in a separate PR. It's good to keep PRs as compact as possible to achieve what they need to do (in this case, fix a bug in the points layer). Maybe make an issue?

[melonora]

@melonora i think that should come in a separate PR. It's good to keep PRs as compact as possible to achieve what they need to do (in this case, fix a bug in the points layer). Maybe make an issue?

Yeah I agree, I will open an issue and will approve this PR, though lets wait for the response of @Czaki before starting the clock.

[Czaki]

Readability?

[jni]

Thanks for the suggestions. 🙏

As someone who is unfamiliar with Mock but intimately familiar with lists (which I suspect describes many Python users), I actually find this less readable. But I don't feel strongly about it at all. This should be ready! 🚀

[Czaki]

For me, Mock is more readable, as I do not need to parse connected functions.

[melonora]

In it goes