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

Persist skeleton group expansion state #7939

Merged
merged 28 commits into from
Aug 5, 2024

Conversation

dieknolle3333
Copy link
Contributor

@dieknolle3333 dieknolle3333 commented Jul 23, 2024

https://moveskeletonexpansiontostore.webknossos.xyz/

Steps to test:

  • Go to skeletons tab, make sure to have some trees, ordered in some groups
  • Expand/collapse groups and subgroups, make sure everything works well
  • reload site and check that the same groups are expanded/collapsed, except root group

TODOs:

  • frontend
    • Add isExpanded to nml
      • read
      • write
      • fix tests
  • backend
    • Improve NML Support
  • fix bug where empty groups cant be dragged

Issues:

  • fixes #

(Please delete unneeded items, merge only when none are left open)

@dieknolle3333 dieknolle3333 reopened this Jul 24, 2024
@dieknolle3333
Copy link
Contributor Author

dieknolle3333 commented Jul 24, 2024

@MichaelBuessemeyer would you be willing to make the according changes in the backend as in the segment group PR? the plan is the same here, to be able to store whether a segment group is collapsed or expanded.

@MichaelBuessemeyer
Copy link
Contributor

Sure :)

@dieknolle3333
Copy link
Contributor Author

@philippotto the backend part is missing so testing will hopefully just yield the same behaviour as before. But I'm requesting review already for the first iteration so that most of the review is already done :) (funny: GitHub automatically set the right base branch.)

@dieknolle3333 dieknolle3333 marked this pull request as ready for review July 24, 2024 15:42
Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

first round of feedback :) will do testing in another iteration.

@dieknolle3333
Copy link
Contributor Author

faster than lightning ⚡

@MichaelBuessemeyer
Copy link
Contributor

MichaelBuessemeyer commented Jul 25, 2024

The basics should work (the isExpanded state is persisted in the backend). However, I still need to check the nml support of the new "isExpanded" field

@philippotto
Copy link
Member

However, I still need to check the nml support of the new "isExpanded" field

If the NML format is extended with an isExpanded property, the frontend parser and writer also need to be adapted. Wklibs, too. I wonder whether that's necessary (on the other hand, it's probably relatively easy to do)... cc @fm3

@fm3
Copy link
Member

fm3 commented Jul 29, 2024

It would not be complex to add that, I think. However, I’m also not sure if losing the expansion state during download + reupload would maybe be fair?

@MichaelBuessemeyer
Copy link
Contributor

If the NML format is extended with an isExpanded property, the frontend parser and writer also need to be adapted. Wklibs, too. I wonder whether that's necessary (on the other hand, it's probably relatively easy to do)... cc @fm3

So we just have everything collapsed or expanded by default? That's fine by me :)

@dieknolle3333 dieknolle3333 changed the title WIP: move skeleton group expansion to store Persist skeleton group expansion state Jul 31, 2024
@philippotto
Copy link
Member

code looks mostly good to me 👍 however: during testing I couldn't move an empty group into a child group of the root. I tried dragging group 4 into group 1, but nothing happened. on https://treetree.webknossos.xyz/ this works, so it seems to be related to this PR? can you reproduce this?

image

@dieknolle3333
Copy link
Contributor Author

dieknolle3333 commented Aug 1, 2024

can you reproduce this?

oh, yes. I will look into it!
for the record, the bug still persists (edit: fixed)

@MichaelBuessemeyer
Copy link
Contributor

MichaelBuessemeyer commented Aug 1, 2024

Please wait before merging. I just noticed that the backed review is still pending :)

@dieknolle3333
Copy link
Contributor Author

@philippotto super weird: I was able to reproduce the bug on https://moveskeletonexpansiontostore.webknossos.xyz/ but cannot reproduce it locally. Even before the small change I made. It doesnt seem to be browser related, but locally I was able to drag and drop empty groups with the current changes (also at commit 5e4c06d, where I addressed the second round of review).

@philippotto
Copy link
Member

hm, weird indeed! however, your "fix commit" makes sense to me and now dnd seems to work reliably. so, from my side, this should be good to go :)

@dieknolle3333
Copy link
Contributor Author

dieknolle3333 commented Aug 1, 2024

oh its works for you now? because it wasnt working for me on the dev instance like 10 minutes ago. but I can confirm that now its working?! whatever, maybe a weird browser caching thing 🤷‍♂️ what matters is that its working now :) 👍

@philippotto
Copy link
Member

because it wasnt working for me on the dev instance like 10 minutes ago.

maybe you had an old tab open? after running /install_dev it can take one or two minutes until the new files are served. if you mistakenly assumed, that the new stuff was already served and kept that tab open, this might explain it. cache problems are plausible, too, of course.

let's test again tomorrow and then merge this branch into tree-tree. on monday, we can hopefully merge tree-tree.

@dieknolle3333
Copy link
Contributor Author

if you mistakenly assumed, that the new stuff was already served and kept that tab open, this might explain it.

yeah, you're right, I might have been to fast. It felt longer than 1-2 minutes that I was switching between dev and local instance, and I felt like I was refreshing quite often, but maybe this was the cause after all. Thanks for the explanation, I will look out for this in the future.

@philippotto
Copy link
Member

yeah, you're right, I might have been to fast. It felt longer than 1-2 minutes that I was switching between dev and local instance, and I felt like I was refreshing quite often, but maybe this was the cause after all. Thanks for the explanation, I will look out for this in the future.

by the way, you can always check the version number that is running here:

image

@philippotto
Copy link
Member

works for me :)

@philippotto philippotto merged commit 8a67ff4 into tree-tree Aug 5, 2024
2 checks passed
@philippotto philippotto deleted the move-skeleton-expansion-to-store branch August 5, 2024 07:28
@MichaelBuessemeyer
Copy link
Contributor

MichaelBuessemeyer commented Aug 5, 2024

Please wait before merging. I just noticed that the backed review is still pending :)

I think the backend review was still missing 🙈
But as it is now part of tree-tree, it can be reviewed there I guess

Copy link
Member

@fm3 fm3 left a comment

Choose a reason for hiding this comment

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

Backend LGTM

@philippotto
Copy link
Member

Oops, I missed your comment between the other discussion.

hotzenklotz added a commit that referenced this pull request Aug 6, 2024
* refactor tree hierarchy view as functional component

* WIP refacator as antd <Tree>

* restore checkbox and expaneded keys

* restored expand/collapse all behaviors

* re-use <DotIcon> components but with rgba values

* restore search

* fix drag and drop for single tree

* fix multi select

* remove react-sortable-tree library

* updated styling and text wrap

* make tree height/width smaller

* collapse sup groups

* added shift selection to skeleton tree

* updated changelog

* Update frontend/javascripts/oxalis/view/right-border-tabs/skeleton_tab_view.tsx

Co-authored-by: MichaelBuessemeyer <[email protected]>

* Update frontend/javascripts/oxalis/view/right-border-tabs/tree_hierarchy_view_helpers.ts

Co-authored-by: MichaelBuessemeyer <[email protected]>

* Update frontend/javascripts/oxalis/view/right-border-tabs/tree_hierarchy_view.tsx

Co-authored-by: MichaelBuessemeyer <[email protected]>

* Update frontend/javascripts/oxalis/view/right-border-tabs/tree_hierarchy_view.tsx

Co-authored-by: MichaelBuessemeyer <[email protected]>

* apply PR feedback

* fix tooltip

* add autofocus to tree search

* fix active tree updates highlighting tree

* fix shift selection

* fix crash on tree deletion

* apply darker background colors for trees in dark mode

* disable node drag icon

* disabled checkboxes for empty groups

* use darker colors for <Tree> components in dark mode

* fix multi select drag and drop

* formatting

* fix CSS alignment of tree checkboxes

* use antd themeing instead of CSS overrides

* format

* Persist skeleton group expansion state (#7939)

* move skeleton expanded/collapsed group state to store

* WIP: expand and collapse all skeleton groups

* fix expand/collapse skeleton groups

* expand group if tree is dropped

* clean up code

* lint

* address review

* address review 2/2

* remove console.log

* lint

* add small condition for ondrop

* persist isExpanded state in backend

* make groups default expanded if field is absent in nml (backend only)

* make groups default expanded if for written nmls (backend only)

* fix backend treegroup update action tests

* add isExpanded to frontend nml support

* update snapshots

* omit object creation in var

* omit useless return statement in map

* add isExpanded prop to tree groups in backend testing fixtures (dummies)

* add test to backend nml test to test properly setting default to expanded

* add changelog

* WIP: address review

* fix bug where drag and drop of empty groups was failing

* remove comment

---------

Co-authored-by: Michael Büßemeyer <[email protected]>
Co-authored-by: Philipp Otto <[email protected]>

* fix tree sorting

* fix off-by-one error in shift select

* override <Tree> scroll handle in dark mode

* use scrollTo(auto)

* format

* fix typo

---------

Co-authored-by: MichaelBuessemeyer <[email protected]>
Co-authored-by: Philipp Otto <[email protected]>
Co-authored-by: Charlie Meister <[email protected]>
Co-authored-by: Michael Büßemeyer <[email protected]>
Co-authored-by: Philipp Otto <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants