Persist tree visibility

[philippotto]

Remaining Todo

• backend: implement updateTreeVisibility update action
• frontend: send above action
• [ ] frontend: apply update tree group visibility action in skeleton.js
• frontend: clean up code (+ fix flow)
• frontend: write tests? :S

URL of deployed dev instance (used for testing):

• https://persisttreevisibility.webknossos.xyz

Steps to test:

• open a skeleton (or hybrid) tracing
• create a few trees and groups
• toggle the visibility of some trees and groups
• save & reload
• the visibilities should be as before

Issues:

• fixes Persist tree visibility across page refreshes  #3503

---

• Updated changelog
• Needs datastore update after deployment
• Ready for review

[philippotto]

@youri-k Could you add an extra "toggleTreeUpdateAction" instead of re-using the "updateTreeGroup" action? Due to how we are doing the compactation of the actions in the front-end, this would really help :)

[youri-k]

@philippotto The route should work. I'm currently doing further testing.

[philippotto]

@daniel-wer I think you can already review this PR if you feel like it. I'll add some tests in the mean time :)

[daniel-wer]

You solved this complicated issue very well in this PR! I especially like that you pointed out the compactToggleActions function for the high-level understanding in a comment. The function is extremely readable and makes it easy to understand what's happening 👍

I'll go on and test now :)

[daniel-wer]

I encountered some issues during testing:

• When loading a tracing that was created before this PR, all trees are invisible (at least that's shown in the trees tab), but they are shown in the viewports anyways. I think that's two issues:
  • The isVisible default for tracings that existed before this PR seems to be false but should be true.
  • Trees with isVisible set to false, are still shown in the viewports when loading the tracing.
• When toggling a group that contains an empty group in its subtree (among other groups and trees), the following error is thrown:

TypeError: Cannot read property 'isVisible' of undefined
    at eval (VM8089 compact_toggle_actions.js:115)
    at arrayAggregator (VM7013 lodash.js:497)
    at Function.eval [as partition] (VM7013 lodash.js:4829)
    at isCommonAncestorToggler (VM8089 compact_toggle_actions.js:115)
    at compactToggleActions (VM8089 compact_toggle_actions.js:169)
    at compactUpdateActions (VM8088 compact_update_actions.js:135)
    at saveTracingTypeAsync (VM8084 save_saga.js:277)
    at saveTracingTypeAsync.next (<anonymous>)
    at next (VM7995 proc.js:321)
    at currCb (VM7995 proc.js:398)

• Import the attached NML in a skeletontracing:
  • All trees are visible and selected. Reload the page, now all trees are invisible.
  • Now toggle all trees to be visible again. Reload the page, now only the trees in the root group are visible, but not those in the groups.

tree-visibility-test.zip

[daniel-wer]

I'll approve once the mentioned issues are resolved :)

[philippotto]

Thanks for the thorough testing and your feedback! In the meantime, I added some tests and extracted the defaultState into an own module, as discussed.

@youri-k Here's the backend todo list, we just discussed:

• ensure that trees created before this PR, get the isVisible=true property
• when creating a new tree (manually or via frontend NML upload), that tree needs to get the isVisible attribute
• double-check backend NML upload behavior

[philippotto]

backend:

• toggling a tree group should work for hierarchical groups

[philippotto]

The PR should be ready for another round of review, @daniel-wer.
@fm3, can you have a look at the back end code?

• When toggling a group that contains an empty group in its subtree (among other groups and trees), the following error is thrown:

I couldn't reproduce this. Maybe this was unintentionally fixed in the meantime? Can you please try to reproduce this again to ensure that I didn't miss anything? :)

[fm3]

Backend LGTM, added a comment regarding a possible minor refactoring

[fm3]

I think it should be possible to do this in one function (without the nested childIter), just start the match etc. directly on rootGroup. Also, I would then just call it group instead of rootGroup/currentGroup. (Since this is not actually used for the root group case of the tracings)

[fm3]

is there a special reason for this change?

[daniel-wer]

I couldn't reproduce this. Maybe this was unintentionally fixed in the meantime? Can you please try to reproduce this again to ensure that I didn't miss anything? :)

I can still reproduce this. Try toggling the root group in this tracing: https://persisttreevisibility.webknossos.xyz/annotations/Explorational/5cab0f9b0100001c00084973#612,702,502,0,2.000,176

Apart from that, this is looking all good! 👍

[philippotto]

I couldn't reproduce this. Maybe this was unintentionally fixed in the meantime? Can you please try to reproduce this again to ensure that I didn't miss anything? :)

I can still reproduce this. Try toggling the root group in this tracing: https://persisttreevisibility.webknossos.xyz/annotations/Explorational/5cab0f9b0100001c00084973#612,702,502,0,2.000,176

Ah, thanks for the repro 👍 I fixed it now :)