Add folders to dataset table

[MichaelBuessemeyer]

This PR adds the child folders of the currently active folder in the dashboard to the dataset table. This pr should include all changes mentioned in #6883, except for the drag & drop feature. I disabled this for the folders as suggested to maybe not include this in the first iteration. Although if you think this should already be included, I can do it. I do not have an opinion on this except that leaving this feature away makes this pr earlier merge ready

URL of deployed dev instance (used for testing):

• https://___.webknossos.xyz

TODOs:

• Open the folder settings upon clicking on some icon in the actions column for folders
• Implement context menu support for the folder rows
• disable drag & drop for the folders
• Decrease the width of the types column
• The settings actions for a folder should only be available to an admin or such.

Optionally:

• [ ] allow dragging dataset rows into a folder row
  • Not so important IMO: one can still drag the rows to the left sidebar and drop them there into the respective folder.

Bugs

• The expand icon should not be rendered for the folders
• The details for the folders seem broken

Steps to test:

• Open the dataset table in the dashboard
• Select the root folder in the left folder tree view
• Right-click on the root folder and create a new folder. The new folder should be listed in the table in the center.
• creating another folder should also result in the folder appearing in the table.
• Right-clicking on a folder in the table should open up the same context menu as if one would click on the folder in the folder hierarchy view on the left.
• single clicking a folder should show its details on the right (another single click should deselect it)
• selecting multiple folders should not work
• drag & drop of folders in the table should not work
• double-clicking a folder should open that folder's contents in the table
• If logged in as an unprivileged user (non admin / dataset manager) manipulating any folder setting should not work / not be available in the UI or the UI should be disabled.
• Please look at #6883 for more details on how the folder rows should behave. (But I think I covered everything in the list above)

Issues:

• fixes Display folders next to datasets in middle view #6883

---

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

• Updated changelog

[MichaelBuessemeyer]

I think everything should be working. Additionally, I looked through the code and it should be review ready.

@philippotto do you have time to take on this review? The pr is quite big/medium-sized. Or should I ask daniel?

[MichaelBuessemeyer]

I think this was prettier correcting the syntax

[MichaelBuessemeyer]

I think this was prettier correcting the syntax

Same here.

[philippotto]

@philippotto do you have time to take on this review?

Yes, I think so :)

[MichaelBuessemeyer]

Oh btw:
A thought that I had: In this pr, there is the problem that the table needs to handle both dataset-objects and folder-objects as a potential row. Thus there are quite a lot of ifs and else structs added in the code. IMO this is not ideal.

My suggestion to tackle this would be to write a class for datasets and one for folders that have the same interface but depending on their implementation behave/render differently:
e.g. in the dataset class, there would be a function to render the name column:

   renderNameColumn(){
             return (
                  <>
                    <Link
                      to={`/datasets/${datasetOrFolder.owningOrganization}/${datasetOrFolder.name}/view`}
                      title="View Dataset"
                      className="incognito-link"
                    >
                      {datasetOrFolder.name}
                    </Link>
                    <br />

                    {context.globalSearchQuery != null ? (
                      <BreadcrumbsTag parts={context.getBreadcrumbs(datasetOrFolder)} />
                    ) : null}
                  </>
                );
     }

And the folder class would look like:

 renderNameColumn(){
  return datasetOrFolder.name;
 }

And then we could just render the name column with:

   <Column
            title="Name"
            /*.........*/
            render={(_name: string, datasetOrFolder: DatasetOrFolder) => {
              return datasetOrFolder.renderNameColumn();
            }}
          />

But as we usually do not create classes for such cases I did not implement this in the first version and decided to simply propose this first. IMO this would make the code cleaner and also easier to read and in case another type of row is added, this would be easier to extend upon.

What do you think @philippotto?

[philippotto]

But as we usually do not create classes for such cases I did not implement this in the first version and decided to simply propose this first. IMO this would make the code cleaner and also easier to read and in case another type of row is added, this would be easier to extend upon.

I agree in principle, but I'm a bit hesitant, since this would be a new pattern in this code base. However, I think, that this would be alright as long as those instances don't live in the react state. Otherwise, this would seem weird to me since these instances would only be dumb wrappers with some rendering behavior living in react state (I wouldn't want the caching context to be responsible for constructing these instances).

I imagine something like this:

class DatasetRendering {
   renderNameColumn() {
    return ...;
   }
}

class FolderRendering {
   renderNameColumn() {
    return ...;
   }
}

function getRenderer(datasetOrFolder) {
  if (datasetOrFolder ... is dataset) {
    return DatasetRendering(datasetOrFolder);
  } else {
    ...
  }
}

renderSomething() {
  return getRenderer(datasetOrFolder).renderNameColumn();
}

I'd hope that the browser can optimize these short lived objects away. But maybe you could make a micro benchmark to test that this only has a negligible impact when being compared to a simple if-else.

What do you think?

[philippotto]

Awesome! Only commented some smaller stuff :) Also one other small thing: The "New Folder" in the Actions column might be clearer as "New Subfolder".

I know that the original issue outlined that dnd-features should come in another iteration. However, I noticed one particular dnd use case (that I just added to the issue) which is something I immediately tried out because it seemed intuitive. Namely: dragging a dataset from the dataset table to a folder within the same table. Could you have a look whether this is easy to achieve? I think, it's easier than the other dnd-features, because the dragged items (the datasets) are already implemented. the drag-target definition in the sidebar might be easy to reuse for the drag targets in the dataset table. if it takes more than 30 minutes, feel free to defer this to another PR :)

[MichaelBuessemeyer]

I agree in principle, but I'm a bit hesitant, since this would be a new pattern in this code base.

I am also fine with keeping the code as it is right now and only implementing my suggestion in case another type of rows is added.

I'd hope that the browser can optimize these short lived objects away. But maybe you could make a micro benchmark to test that this only has a negligible impact when being compared to a simple if-else.

To fix that we could have a single instance of DatasetRendering and FolderRendering each, make all their methods static, and e.g. initialize them lazily but cache them (In fact we do not need classes at all and could also simply use objects):
without lazy init (could be added, but doesn't add enough value imo)

const datasetRenderer = new DatasetRender();
const folderRenderer = new FolderRender();
function getRenderer(datasetOrFolder) {
  if (datasetOrFolder ... is dataset) {
    return datasetRenderer;
  } else {
    ...
  }
}
renderSomething() {
  return getRenderer(datasetOrFolder).renderNameColumn(datasetOrFolder); //<-- Needed as an argument as the method is static.
}

My preferred version would be working with objects:

const datasetRenderer = {
  renderColumnA: (record) => {...},
  renderColumnB: (record) => {...},
  ...
}
const folderRenderer = {
  renderColumnA: (record) => {...},
  renderColumnB: (record) => {...},
  ...
}

in the render function map the renders to each record:

render () {
// ...
const sortedDataSourceWithRenderer = sortedDataSource.map(record => isADataset(record) ? {...record, renderer: datasetRenderer} : {...record, renderer: folderRenderer});
//...
<FixedExpandableTable
    dataSource={sortedDataSource}
//...

Independent from which solution we choose, I'll do a micro-benchmark 👍

[philippotto]

My preferred version would be working with objects:

Yes, I like that one, too 👍

[MichaelBuessemeyer]

TODO for me:

• Found a 🐛: A folder with only subfolders and no datasets as children renders and empty folder / an empty table :/
• > I know that the original issue outlined that dnd-features should come in another iteration. However, I noticed one particular dnd use case (that I just added to the issue) which is something I immediately tried out because it seemed intuitive. Namely: dragging a dataset from the dataset table to a folder within the same table. Could you have a look whether this is easy to achieve? I think, it's easier than the other dnd-features, because the dragged items (the datasets) are already implemented. the drag-target definition in the sidebar might be easy to reuse for the drag targets in the dataset table. if it takes more than 30 minutes, feel free to defer this to another PR :)

[MichaelBuessemeyer]

Results of the micro-benchmark with the classes version (Mapping each data to a class wrapping this data)

Mapping more then 98 rows takes less then 0.1 milliseconds on my laptop (framework with very new hardware). I'd guess that this should also take up to ~2ms at max on a standard machine.
I'd say, that this time is negligible. Here are the measures I took using console.time.

mappingClasses: 0.01611328125 ms
mappingClasses: 0.014892578125 ms
mappingClasses: 0.04296875 ms
mappingClasses: 0.035888671875 ms
mappingClasses: 0.033935546875 ms
mappingClasses: 0.031982421875 ms
mappingClasses: 0.010986328125 ms
mappingClasses: 0.014892578125 ms
mappingClasses: 0.010986328125 ms

[philippotto]

great to see the refactoring worked out :)

[MichaelBuessemeyer]

Hi @philippotto,
I addressed all your feedback. This include that it is now possible to drop the datasets into the folders listed in the table.

A short discussion with daniel resulted in only highlighting the folders in the dataset table if the user drags datasets over them and not all the time as done in the left folder hierarchy view. We did this, because in the artificial scenario, where I had only 2 datasets but 70 folders due to the benchmark mentioned above, having all folders highlighted looked confusing. Additionally, it is not ideal that the "drop area highlight color" is the same color used to mark datasets in the table as selected.
Maybe my scenario with 70 folders and only two datasets was too artificial and the regular use case looks also alright when the folders in the table are highlighted all the time. Because highlighting the folder hierarchy view all the time but the folders in the dataset table only once they are hovered is a mismatch in behaviour, which might not be ideal.
Do you have any preference or idea on how to improve this? react-dnd seems not to support custom cursors on drag and drop to show indicate whether something can be dropped or not. There only seem to exist hacky solutions.

[philippotto]

Awesome, works great 👍

A short discussion with daniel resulted in only highlighting the folders in the dataset table if the user drags datasets over them and not all the time as done in the left folder hierarchy view. We did this, because in the artificial scenario, where I had only 2 datasets but 70 folders due to the benchmark mentioned above, having all folders highlighted looked confusing. Additionally, it is not ideal that the "drop area highlight color" is the same color used to mark datasets in the table as selected.
Maybe my scenario with 70 folders and only two datasets was too artificial and the regular use case looks also alright when the folders in the table are highlighted all the time. Because highlighting the folder hierarchy view all the time but the folders in the dataset table only once they are hovered is a mismatch in behaviour, which might not be ideal.
Do you have any preference or idea on how to improve this? react-dnd seems not to support custom cursors on drag and drop to show indicate whether something can be dropped or not. There only seem to exist hacky solutions.

I think the current way is absolutely fine 👍 The left sidebar is only highlighted during drag, so that the user knows that they can use the sidebar as a drop target (since the table and the sidebar are two visually disconnected components I found this sensible when I implemented this). Dragging a DS from the table to a folder in the table feels way more intuitive, since source and target are in the same component. Therefore, there's no need to highlight all valid drag targets in my opinion. Thus, only highlighting a folder while dragging a DS over it feels like the right approach to me 👍

[philippotto]

Excellent as usual 👍 :)

[philippotto]

remove before merging