Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 85.94%. Comparing base (e38cc72) to head (18d2fb8).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##           master      #85      +/-   ##
==========================================
- Coverage   86.08%   85.94%   -0.14%     
==========================================
  Files          13       13              
  Lines        1272     1274       +2     
==========================================
  Hits         1095     1095              
- Misses        177      179       +2     

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

Upon further prodding and poking I have realised that the issue is caused by the surface layer not having a properties attribute by default, which is causing the issues with this specific layer. This also cannot be fixed by loading the table with the function provided here.

Hi @Cryaaa , hi @haesleinhuepf ,

I brought #87 here (since they are closely related) and added some conditions to make this work with the Surface and Points layers.

I had to change a bit the github actions for the moment (related issues are linked to my commits). Tests work locally.

This should close #84 , #86 and haesleinhuepf/napari-process-points-and-surfaces#89

Thanks @zoccoler, I think it makes sense to merge these PRs! I just tested it in action in the napari cluster plotter and I think you may have reintroduced the problem (see my comment on the code). I will try and dive deeper into this later

Alright, I made some changes to mirror features to properties if features was set, and to mirror properties to features if properties was set (this latter may not be necessary for the set_contents method, but at least it keeps things consistent IMO.

I also fixed macos automated tests as suggested here and ubuntu version problem, which was already solved now.

@Cryaaa let me know if it works now and if you are OK with the proposed changes

@zoccoler,
Seems to be working for me now! I think we should take care to only set features via the load csv function in our examples though, since there are some cases where you could break this if you are set on doing so: If you have a layer with properties like points layer and you only set the feature layer to contain info as a dataframe it will load the empty dictionary into the table widget. I'm not sure how to solve those cases without a convoluted mess of if and else statements... Do you think I should give it a shot in this PR or cross that bridge when we get to it?

@zoccoler, Seems to be working for me now! I think we should take care to only set features via the load csv function in our examples though, since there are some cases where you could break this if you are set on doing so: If you have a layer with properties like points layer and you only set the feature layer to contain info as a dataframe it will load the empty dictionary into the table widget. I'm not sure how to solve those cases without a convoluted mess of if and else statements... Do you think I should give it a shot in this PR or cross that bridge when we get to it?

As of how it is right now, yes, that would happen for a layer like the points layer. Maybe we could do this mirroring for all layers (except Image)? Then, if a user sets features and not properties, properties will get updated and table would display correctly. If a user sets properties, we are already good to go and could optionally mirror it to features.

We could replace this line by something like if not isinstance(layer, napari.layers.Image):
What do you think?

Actually I just tested setting the features of the Points layer via code and calling show table and the table is not empty.

Actually I just tested setting the features of the Points layer via code and calling show table and the table is not empty.

Nice then it was just a problem that I thought was there, all good to go then for me!

Alright! @haesleinhuepf , if you agree with the changes here, feel free to merge. Otherwise, let us know!