Refactor annotation groups #563

richartkeil · 2023-12-11T08:19:24Z

Closes #558.

Tonybodo

Nice work. The new code definitely looks much better.

Tonybodo · 2023-12-11T09:40:55Z

apps/editor/src/components/editor/layers/draggable-layer-list-item.tsx

+  const style = {
+    transform: CSS.Transform.toString(transform),
+    transition,
+    opacity: isDragged ? 0.3 : 1,


We could add the 0.3 as a variable draggedLayerListItemOpacity (or similar) to the theme.

Same here with the style?

It is still open but maybe not necessary.

From my point of view its fine, if you want to add it to the theme, feel free :D

Feat: Add new Annotation Group

…tation-groups

Refactor annotation groups UI

Add and delete annotation groups

Refactor saving

Tonybodo · 2024-02-24T18:36:42Z

I've noticed a problem with also existent in the Visian app deployment; when exporting a zip file with image layers and the "exclusive segmentations" setting enabled, there's a loss of information in the image layers. This shouldn't happen. Should I create a ticket for it or should we fix it in this branch?

…oups

TimRiedel · 2024-02-27T13:49:52Z

Hi, I am happy that this huge and important feature is now ready for review! I already started the code review and will finish it soon, here is my functionality review so far.

What I like

the look and feel of the annotation layer UI is great and exactly what we imagined it to be
dragging and dropping the layers/groups works really good while displaying the top group in the front and the lower groups in the back
adding and deleting annotation groups works well and intuitive, especially when they are empty
skipping through the annotation groups within the review bar is a nice feature and works like a charm

Areas for improvement / bugs

As this PR is quite huge and I lost count of which tickets fall into this PR, please let me know if any of my remarks were not in the scope of this ticket. Especially if you do not deserve the praise or blame for these things :D

When opening a BraTS image (in create mode, i.e. clicking on the image in the list), importing a single annotation from the import menu and then saving the image, it says that the dataUri is not valid. I think that we should set the name of imported annotation groups so that they can be stored in the backend without renaming them again. Furthermore it appears as if the warning has a different font/font size and that there is a lack of padding between the warning and other elements, which makes it inconsistent with the rest of the UI.

Importing a second annotation (e.g. the same one again) and storing it, correctly displays the error that there is already an annotation with that name in the backend. However when changing the name in the save popup, the name of the layer group does not get updated. Now we have the annotation group with different names in the backend, but two times the same in the UI.

When trying to rename that group, the entire text is highlighted and I was not able to click into the text and set the cursor to the text position I would like to update. I either had to delete the entire name or move the arrow keys, which was a bit unintuitive.
Modifying an already stored annotation and then clicking "save" displays an empty field "Overwrite existing annotation" with an enabled "Save" button. I am not sure what to make of this empty field as an end user, I simply wanted to save the existing one.

Adding a new layer to an annotation group that exists already in the backend and saving the group again, results in an error. The error occurs in both cases: if the new layer is empty and if the new layer is not empty. It appears to be some kind of react error or so and the call to the backend was not even made. Interestingly there are two types of errors:

when I imported the annotation, saved it, then added the layer and saved it again, it displayed the following (which probably has something to do with the empty "Overwrite existing annotation" field):

when I exited the editor after saving the imported annotation, reopened the image, added the layer and saved it, it displayed this. The same happened when making changes on an existing layer in review mode:

Handling long annotation group names appears to be a problem in the review bar UI. We could get rid of the little dot by greying out the save button when an annotation group has no changes. This should be addressed when refactoring the review bar to look better in a new issue.
I am unsure which button to press when toggling the verified button in the review bar. At which point is it stored in the backend? When saving the annotation? When moving on to the next one? Immediately? Apparently it does not cause changes on the annotation group layer and does not add the grey dot to the name of the annotation group in the review bar.

I am very sorry, that there are so many things that I had to complain about! Probably its also due to the fact that I did not have contact with these things for quite some time and that we try to merge all these things at once, rather than having them as individual feature branches.
Although I am not quite done yet with the code review itself, I will publish it right now, so that you can start working on the things I already looked at. I suggest that I do another review once we clarified, which points to fix in this PR and for which to add new tickets.

TimRiedel

Temporary review. Files not reviewed yet:

layers.tsx
save-popup.tsx
annotation-group.ts

apps/editor/src/assets/de.json

apps/editor/src/components/data-manager/confirmation-popup/confirmation-popup.props.ts

apps/editor/src/components/editor/layers/draggable-group-list-item.tsx

TimRiedel · 2024-02-27T10:33:45Z

apps/editor/src/components/editor/layers/draggable-layer-list-item.tsx

+  const style = {
+    transform: CSS.Transform.toString(transform),
+    transition,
+    opacity: isDragged ? 0.3 : 1,


Same here with the style?

apps/editor/src/models/editor/document.ts

TimRiedel · 2024-02-27T11:01:00Z

apps/editor/src/models/editor/document.ts

+          path.basename(filteredFiles.name, path.extname(filteredFiles.name)),
+          this.getMetadataFromFile(filteredFiles),
+        );
+      }


Couldn't you create a new method that accepts the filtered Files and call this method instead? Because you are using the same code down below again and this importFiles method is already hard to understand, so I would suggest extracting the code.

But it is basically one function call, I think to hide the is "annotationGroupId" in filteredFiles check is not the right way to refactor it and would make it even more obscure. But you are right the method really needs a refactoring.

Although it is only one function call, nevertheless these are 10 lines of code duplicated in the same, already hard to read method. I am insisting on this a bit, because we won't be the ones refactoring this whole method and should do our best to at least not make this method worse.

apps/editor/src/models/editor/document.ts

TimRiedel · 2024-02-27T11:09:25Z

libs/ui-shared/src/lib/types/editor/layers.ts

+  /** Sets the flag if the group experiences a change in the number of layers. */
+  setHasUnsavedChanges(value: boolean): void;


Okay, so but if the user made changes to a single layer (annotating something), this boolean is NOT set? Only, when the number of layers changed by deleting or adding some? In that case, I would probably rename the method to match the description.

Is this the method based on which we display or grey out the save button of the group?

Richard suggested that name, originally it was different. This flag indicates if there are changes in the number of layers of one group (e.g. caused by deletion or dragging). HasUnsavedChanges is a protected variable on annotation-groups usually you use hasChanges for one group, which checks the hasUnsavedChanges flag and the hasChanges flag for each layer of this group.

Additionally, we could set this flag on toggle the verification slider (then you would see the gray dot after toggle).

Okay thanks Tony for the explanation!
@Tonybodo @richartkeil what about setDidLayerNumberChange? That would make it more obvious in the hasChanges method of the annotation group, what factors trigger a change for an annotation group.

I like the idea with the verified slider. Am I right that as soon as the Annotation Group has changes, the popup is opened that asks the user how to proceed with the annotation group, if he wants to continue to the next task? This would enforce the behavior and make it more understandable 👍🏼

TimRiedel · 2024-02-27T11:11:40Z

libs/ui-shared/src/lib/types/editor/layers.ts

+  /** All layer ids in the group. */
+  layerIds: string[];
  /** All layers in the group. */
  layers: ILayer[];


Why must we store layers and layerIds in separate arrays? I think that makes it harder to synchronize them. Couldn't we instead add a layerIds method that filters the layers for their ids?

I'm not 100% sure of this, but I think Richard and Konrad decided to do it this way.

When I remember correctly, layers and layerIds do not contain the same information. Annotation Groups for example also have a layerId for the layerMap but they are not a layer. Do you get the point?

Okay, I spoke with @richartkeil and he is himself unsure, why it was decided like that. He remembers some kind of limitation with the other proposed system, but will look into it and add his findings as a single sentence to the documenting comments, explaining the difference between the two arrays.

apps/editor/src/models/review-strategy/review-strategy.ts

Tonybodo · 2024-02-27T15:32:57Z

Thanks for your detailed review, even if many of the complained things have nothing to do with the initially pull request.
I will go through your feedback step by step:

I think this error occur because you have a .nii file instead a nii.gz file, this is an old issue in our system, but I can fix it.
That is great finding and should be changed in this pull request.
Nice that you want to have this, but renaming the layers works in the same way. I suggest that we create a issue for both or fix it after the merge.
That definitely should not be the case, maybe something was disrupted in the merge, i will have a look into this.
Same here
I agree that it is necessary to create a new issue for this, but I really like the gray dot and would rather keep it. Also, I think that integrating this dot could be an improvement and possibly replace the "Save" button entirely. Also, the inclusion of overflow text here could increase usability, although we will need to adjust the margin settings accordingly.
You are right this is still an issue we have to address, currently the annotation toggle does not save the annotation group in the backend, so we have to set the hasChanges flag to true on toggle. The problem would be, that when you toogle and then toogle again, so that the value represents the original one, it would indicate that there are unsaved changes. Otherwise we could make a patch call on each toggle what do you think?

Tonybodo · 2024-02-27T16:14:19Z

For the saving problems I'm very sorry I did a late change and use the path library but the pathextname was not correct for our format nii.gz and i didn't rechecked the case with .nii files after the merge. I think now should everything work.

Tonybodo · 2024-02-27T16:41:32Z

For the point with the verified, I don't have a clue what is the best workflow here. When you have an idea I can implement this. For instance, we could say on toggle you make changes on the annotation group an have to save again. Like this:

public trySetIsVerified(value: boolean) {
if (isMiaAnnotationMetadata(this.metadata)) {
this.metadata = {
...this.metadata,
verified: value,
};
this.hasUnsavedChanges = true;
}
}

But we should handle it in a different merge request, because it is completely unrelated to the work here.

Tonybodo · 2024-02-27T17:34:44Z

@TimRiedel Thanks again for your review I currently pushed my changes for your suggestions and wrote something below your comments.
Open tasks:

Continue review and recheck if bugs were solved
Create new tickets for minor things so that we can work on them after the merge

TimRiedel

Thanks for implementing many of my suggestions. This was a code review and a lot of the things are already better.
I will do the functionality review (bug testing) later this day.

TimRiedel · 2024-03-06T12:21:46Z

apps/editor/src/models/editor/document.ts

+          path.basename(filteredFiles.name, path.extname(filteredFiles.name)),
+          this.getMetadataFromFile(filteredFiles),
+        );
+      }


Although it is only one function call, nevertheless these are 10 lines of code duplicated in the same, already hard to read method. I am insisting on this a bit, because we won't be the ones refactoring this whole method and should do our best to at least not make this method worse.

TimRiedel · 2024-03-06T12:22:19Z

apps/editor/src/models/editor/document.ts

+        if (uniqueValues.size === 1 && uniqueValues.has(0)) {
+          createdLayerId = await this.importAnnotation(
+            { ...imageWithUnit, name: `${image.name}` },
+            undefined,
+            false,
+          );
+          if (files instanceof File) {
+            this.addLayerToAnnotationGroup(createdLayerId, files);
+            this.addMetadataToLayer(createdLayerId, files);
+          }
+        }


Same here, I think that could be extracted to a separate method as well, with a descriptive naming of what this code block is trying to achieve.

TimRiedel · 2024-03-06T12:24:30Z

apps/editor/src/models/editor/document.ts

+      if ("annotationGroupId" in filteredFiles) {
+        const typedFilteredFiles = filteredFiles as FileWithAnnotationGroup;
+        const newUnzippedFiles = unzippedFiles.map((unzippedFile) => {
+          const newFile = unzippedFile as FileWithAnnotationGroup;
+          newFile.annotationGroupId = typedFilteredFiles.annotationGroupId;
+          newFile.metadata = typedFilteredFiles.metadata;
+          return newFile;
+        });
+        await this.importFiles(newUnzippedFiles);
+      } else {
+        const fileName = path.basename(filteredFiles.name);
+        await this.importFiles(
+          this.createAnnotationGroup(
+            unzippedFiles,
+            fileName.slice(0, fileName.indexOf(".")),
+            this.getMetadataFromFile(filteredFiles),
+          ),
          filteredFiles.name,
-          this.getMetadataFromFile(filteredFiles),
-        ),
-        filteredFiles.name,
-      );
+        );
+      }


Same here, why not extract this to a method with a descriptive name?

TimRiedel · 2024-03-06T12:54:22Z

libs/ui-shared/src/lib/types/editor/layers.ts

+  /** All layer ids in the group. */
+  layerIds: string[];
  /** All layers in the group. */
  layers: ILayer[];


Okay, I spoke with @richartkeil and he is himself unsure, why it was decided like that. He remembers some kind of limitation with the other proposed system, but will look into it and add his findings as a single sentence to the documenting comments, explaining the difference between the two arrays.

TimRiedel · 2024-03-06T13:00:59Z

libs/ui-shared/src/lib/types/editor/layers.ts

+  /** Sets the flag if the group experiences a change in the number of layers. */
+  setHasUnsavedChanges(value: boolean): void;


Okay thanks Tony for the explanation!
@Tonybodo @richartkeil what about setDidLayerNumberChange? That would make it more obvious in the hasChanges method of the annotation group, what factors trigger a change for an annotation group.

I like the idea with the verified slider. Am I right that as soon as the Annotation Group has changes, the popup is opened that asks the user how to proceed with the annotation group, if he wants to continue to the next task? This would enforce the behavior and make it more understandable 👍🏼

TimRiedel · 2024-03-06T13:03:47Z

apps/editor/src/components/editor/layers/draggable-layer-list-item.tsx

+  const style = {
+    transform: CSS.Transform.toString(transform),
+    transition,
+    opacity: isDragged ? 0.3 : 1,


From my point of view its fine, if you want to add it to the theme, feel free :D

TimRiedel · 2024-03-06T13:20:00Z

For the point with the verified, I don't have a clue what is the best workflow here. When you have an idea I can implement this. For instance, we could say on toggle you make changes on the annotation group an have to save again.

I like the suggestion on letting the annotation group have changes when the verified button is toggled. Even if toggling it twice does not result in "no changes" anymore, this is better than not communicating to the user that these changes must be saved. Even in day-to-day programs like Word or VSCode: when you manually add some letters and then remove them with backspace, it still shows that you must save the document. Only if you hit "undo" the previous state is restored, but VISIAN does not allow this functionality currently. So the proposed workflow is fine 👍🏼

But we should handle it in a different merge request, because it is completely unrelated to the work here.

But doesn't this PR include the Issue and Branch "Refactor Saving"? In any case, I would definitely say it is in the scope of this PR, because this saving functionality was touched here to a great extent. Everything we do not do now will probably be on hold forever, because we are finishing this project soon.

TimRiedel · 2024-03-06T13:29:25Z

‼️ We also just merged the MIA SDK into the hub and into VISIAN (#538). Please make sure to update this branch accordingly and search for all API calls that you did do here in this PR. They must be replaced by calls to the MIA SDK (examples for that are in the annotation-mutations file). That also includes any mutations that are used (examples for mutations can be found in the dataset-page file).
You might need to install yarn version 4 or greater in order to install all the dependencies.

TimRiedel · 2024-03-06T13:34:19Z

When opening a BraTS image (in create mode, i.e. clicking on the image in the list), importing a single annotation from the import menu and then saving the image, it says that the dataUri is not valid. I think that we should set the name of imported annotation groups so that they can be stored in the backend without renaming them again. Furthermore it appears as if the warning has a different font/font size and that there is a lack of padding between the warning and other elements, which makes it inconsistent with the rest of the UI.

I think this error occur because you have a .nii file instead a nii.gz file, this is an old issue in our system, but I can fix it.

Sorry to hear that this is an old issue. I really thought at least BraTS is working entirely for import and export. Its up to you, either create a new issue or solve it right here. But I think we should fix that

When trying to rename that group, the entire text is highlighted and I was not able to click into the text and set the cursor to the text position I would like to update. I either had to delete the entire name or move the arrow keys, which was a bit unintuitive.

Nice that you want to have this, but renaming the layers works in the same way. I suggest that we create a issue for both or fix it after the merge.

Okay, lets create a new issue!

I agree that it is necessary to create a new issue for this, but I really like the gray dot and would rather keep it. Also, I think that integrating this dot could be an improvement and possibly replace the "Save" button entirely. Also, the inclusion of overflow text here could increase usability, although we will need to adjust the margin settings accordingly.

Sure, you can create a new issue, that is indeed a bit unrelated to this PR.

You are right this is still an issue we have to address, currently the annotation toggle does not save the annotation group in the backend, so we have to set the hasChanges flag to true on toggle. The problem would be, that when you toogle and then toogle again, so that the value represents the original one, it would indicate that there are unsaved changes. Otherwise we could make a patch call on each toggle what do you think?

Lets fix this here as described in one of my previous comments!

refactor: remove external dependency for layer sorting

cf2d072

richartkeil requested a review from Tonybodo December 11, 2023 08:19

Tonybodo suggested changes Dec 11, 2023

View reviewed changes

Tonybodo and others added 25 commits December 13, 2023 14:17

fix: Import annotation groups correctly

edf8ab0

fix: Import for single Images

2dd3c82

fix: DnD for annotation groups and Image Layers

be5dd36

Feat: Add new Annotation Group

21139d0

feat: Add Annotation Groups via UI

9b49968

Merge pull request #568 from HealthML/annotation-groups-only

951ce82

Feat: Add new Annotation Group

Fix: Can not drag and drop after import

d241ab4

Feat: Delete AnnotationsGroups

16940ae

Merge branch 'refactor-annotation-groups-ui' into add-and-delete-anno…

b96499c

…tation-groups

Feat: Delete annotation references from database

455f90a

Feat: Refactor saveAs method, so that its add metadata

bee5af2

Fix: First annotation-layer not draggable

aa379ae

Feat: Confirmation popups for delete annotation groups

3d9643f

Fix: Review adjustments

d59c816

Refactor: SaveAs changes metadata for existing group

58be698

Fix: Flanky import and imported group naming

079385b

Fix: Updating hasChanges on annotation groups

01750d4

Fix: Create squashed Nifti and export popup

79d7877

Fix: Fixes for presentation

2c6a2c7

Merge pull request #567 from HealthML/refactor-annotation-groups-ui

8c0350f

Refactor annotation groups UI

Merge pull request #573 from HealthML/add-and-delete-annotation-groups

ed85c4d

Add and delete annotation groups

Merge branch 'refactor-annotation-groups' into refactor-saving

244efd9

Fix: Merge request

6a9566c

Refactor: Remove popup for groups without metadata

46f2f94

Merge pull request #577 from HealthML/refactor-saving

e66bd44

Refactor saving

Tonybodo requested a review from TimRiedel February 24, 2024 18:25

Merge branch 'develop-annotation-service' into refactor-annotation-gr…

075847f

…oups

Tonybodo changed the title ~~Remove external dependency for layer sorting~~ Refactor annotation groups Feb 24, 2024

Tonybodo self-assigned this Feb 24, 2024

TimRiedel requested changes Feb 27, 2024

View reviewed changes

Fix: File endings corrupt saving

70f3eaf

Fix: Review changes

da4c03b

Tonybodo approved these changes Feb 27, 2024

View reviewed changes

Tonybodo requested a review from TimRiedel February 27, 2024 22:59

TimRiedel requested changes Mar 6, 2024

View reviewed changes

		/** Sets the flag if the group experiences a change in the number of layers. */
		setHasUnsavedChanges(value: boolean): void;

Refactor annotation groups #563

Are you sure you want to change the base?

Refactor annotation groups #563

Uh oh!

Conversation

richartkeil commented Dec 11, 2023

Uh oh!

Tonybodo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tonybodo commented Feb 24, 2024

Uh oh!

TimRiedel commented Feb 27, 2024

What I like

Areas for improvement / bugs

Uh oh!

TimRiedel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Tonybodo commented Feb 27, 2024

Uh oh!

Tonybodo commented Feb 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tonybodo commented Feb 27, 2024

Uh oh!

Tonybodo commented Feb 27, 2024

Uh oh!

TimRiedel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimRiedel commented Mar 6, 2024

Uh oh!

TimRiedel commented Mar 6, 2024

Tonybodo commented Feb 27, 2024 •

edited

Loading