webknossos icon indicating copy to clipboard operation
webknossos copied to clipboard

Refactor Skeletontree tab as Antd <Tree /> component

Open hotzenklotz opened this issue 1 year ago • 2 comments

In an effort to use the same antd <Tree /> tree component for the skeletons, comments, and segments, I refactored the skeleton tab to use this component.

URL of deployed dev instance (used for testing):

  • https://treetree.webknossos.xyz/

Steps to test:

  • Do something outside the skeleton view so that it must update (select different tree, node, etc)
  • Rename skeletons
  • Drag and drop skeletons
  • Try context menu stuff
    • color changes
    • expand/collapse everything
  • Try really large skeleton
  • Try single select
  • Try multi select with CTRL/meta or SHIFT

TODOs:

  • [x] Drag & Drop
  • [x] Auto-Scrolling
  • [x] Styling
  • [x] Multi-Select
  • [x] Search
  • [x] Expand All / Expand Subgroup
  • [x] Fix --- label on hover
  • [x] Fix repeated SHIFT selection
  • [x] Hide drag handler icon
  • [x] Fix crash for "delete tree"
  • [x] Empty groups should not show checkbox
  • [ ] Fix drag and drop moving too many trees
  • [ ] (Maybe) fix search behavior

Issues:

  • Contributes to #7765
  • Fixes #5872
  • Fixes #5241

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

  • [x] Updated changelog
  • [ ] Updated migration guide if applicable
  • [ ] Updated documentation if applicable
  • [ ] Adapted wk-libs python client if relevant API parts change
  • [ ] Removed dev-only changes like prints and application.conf edits
  • [x] Considered common edge cases
  • [ ] Needs datastore update after deployment

hotzenklotz avatar May 22 '24 14:05 hotzenklotz

Here is my pre-review testing feedback:

  • [x] The tree items are too spacy: This is the current master version which does not line break the tree names and thus is horizontally scrollable but more dense image And this is your version: image I do prefer the dense (master) version

  • [ ] Rearranging trees within a group does no longer work on your branch.

  • [x] Empty tree groups are opened by default on the master when:

  1. creating a new group and
  2. moving some trees into them.

On your branch one needs to expand the group as step 3.

  • [x] On the master, the level intend seems to be smaller (due to the missing drag handle). Is it possible to make this a little narrower to make the view more compact?

These are the thing I noticed during testing. So far it looks very good beside this few points :rocket:

MichaelBuessemeyer avatar Jun 05 '24 11:06 MichaelBuessemeyer

Rearranging trees within a group does no longer work on your branch.

I think this can't be helped since all trees are always (re-)sorted. Yes, one was able to temporarily rearrange trees in the old version, but upon reload this "state" was discarded and all skeletons were re-sorted according to the selected sorting method. We would need to add and persist some sort of of "sorting index" to get this fixed correctly.

I am open to lower key ideas for fixing this though.

hotzenklotz avatar Jun 12 '24 13:06 hotzenklotz

Really looking forward to see this merged! I tested this PR for a bit and found some things:

  • [x] the scroll bar is not visible in dark mode (because the background color of the scroll bar is too dark?). in light mode, it works fine
  • [x] when selecting a tree, it is always scrolled to be at the top of the view (maybe both points can be solved with scrollTo({..., align: "auto"})?
    • [x] when manually selecting an item from the list, this is very annoying, because the tree item was perfectly visible, anyway.
    • [x] when selecting a tree/node in the data viewport, this auto-scroll behavior also always enforces the item to be at the top of the view (if the list is long enough). however, when the item was visible anyway, it's not necessary to scroll at all.

won't fix?

  • if a tree name is very long, you cannot scroll horizontally. apparently, this is a limitation of antd. so probably a won't fix. the full name can be seen via the tooltip or the rename-field.

philippotto avatar Aug 05 '24 08:08 philippotto

Hmm, I found another thing:

  • [x] shift select often doesn't work. could it be that it performs the range by sorting alphabetically? however, the visible sorting might be by creation time. in the screenshot, the blue item was active and I shift-clicked on the gray item. nothing happens in that case

image

philippotto avatar Aug 05 '24 08:08 philippotto

Please wait until the backend changes of this pr have been reviewed (they haven't been reviewed yet. See: https://github.com/scalableminds/webknossos/pull/7939#issuecomment-2269370736)

@normanrz Could you take over looking over these changes? There shouldn't be many changes.

MichaelBuessemeyer avatar Aug 05 '24 16:08 MichaelBuessemeyer