Ensure unique volume layer names when changing names in frontend #6289

MichaelBuessemeyer · 2022-06-20T17:09:17Z

This PR makes it impossible for the user to set layer names of volume annotation layers that already exist. The reason is to prevent duplicate names. The prevention is done when changing a name of a layer and when adding a new volume annotation layer. This pr does not change the names of existing volume layers but it marks them red if there are duplicates and asks the user to rename the layer with duplicated names.

URL of deployed dev instance (used for testing):

https://uniqueannotationlayernames.webknossos.xyz

Steps to test:

I suggest testing this locally as the first step required is not (easily) possible on a dev branch.

Create a volume annotation with multiple layers. Give the layers the same name. Add another layer that includes a / in its name.
Now checkout the new branch, there layer names in the layer settings should now be red and a tooltip should say that these layers should be renamed to avoid duplicated names. The layer with the / should say that this character is not allowed and it should also be marked red.
Try renaming the layers. This should work. Trying to enter a name that already exists should not be possible. Instead, an error toast should pop up. Entering a name that contains a / should also result in an error toast.
Rename the layer so no names are duplicated and no name contains a /. This should then work.
Try adding a new annotation layer. The suggested name should not match any of the existing names. Entering an existing name or one containing a / and then trying to confirm should result in an error toast.

Issues:

fixes #6251

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

Updated (unreleased) changelog
Ready for review

- refactor layer name validation functions

…que-annotation-layer-names

frontend/javascripts/oxalis/model/accessors/volumetracing_accessor.ts

MichaelBuessemeyer · 2022-06-21T12:04:32Z

frontend/javascripts/oxalis/view/components/editable_text_label.tsx

+    const validateAndUpdateValue = () => {
+      if (this.validateFields()) {
+        this.props.onChange(this.state.value);
+        this.setState({
+          isEditing: false,
+        });
+      }
+    };
+    if (this.props.trimValue) {
+      this.setState(
+        (prevState) => ({ value: prevState.value.trim() }),
+        // afterwards validate
+        validateAndUpdateValue,
+      );
+    } else {
+      validateAndUpdateValue();


Here I avoid having a volume layer name with trailing white spaces. Previous Layer1 and Layer1 was possible and not counted as a name duplication. This is fixed by setting the prop trimValue in the EditableTextLabel.

daniel-wer

Please see my round of rather high-level suggestions. My hope is to be able to simplify the code and make it easier to understand for future readers. Let me know whether this makes sense to you. Happy to jump on a call :)

Also, my impression was that not only forward slashes should be forbidden, but more characters. Since the internal discussion about that was not clear, I asked again. It would be great if you could incorporate the result of that discussion once it converged.

frontend/javascripts/oxalis/model/accessors/volumetracing_accessor.ts

frontend/javascripts/oxalis/view/components/editable_text_label.tsx

frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx

frontend/javascripts/oxalis/view/components/editable_text_label.tsx

…que-annotation-layer-names

MichaelBuessemeyer

Hi Daniel,
thanks for the review. I applied all your feedback and adjusted the validation of invalid characters so that URI encoding does not change them.
I had two ideas on how to approach this. The first is the "brute force" method: I remove invalid chars from the new layer name until the URI encoded version matches the original version. While doing so I track the invalid chars that are then shown to the user as invalid. This method should be 100% correct.
The second method uses a regex that removes all save uri characters that will not be encoded in uris according to (this post)[https://perishablepress.com/stop-using-unsafe-characters-in-urls/#character-encoding-chart]. The left over chars are invalid ones.
The second version is shorter and easier to understand, while I am not 100% sure whether the uri safe chars from the post are correct. But I would personally prefer the second version.

You can compare both versions in commit e281c4a

frontend/javascripts/oxalis/model/accessors/volumetracing_accessor.ts

frontend/javascripts/oxalis/view/components/editable_text_label.tsx

frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx

daniel-wer

Thank you for applying the feedback, the code already looks better and is easier to understand, agreed! 👍 There are a couple of functional issues left, please see my comments.

frontend/javascripts/oxalis/view/left-border-tabs/modals/add_volume_layer_modal.tsx

frontend/javascripts/oxalis/view/components/editable_text_label.tsx

frontend/javascripts/oxalis/view/left-border-tabs/modals/add_volume_layer_modal.tsx

MichaelBuessemeyer

@daniel-wer Thanks a lot for the quick review yesterday. And thanks for already finding the bugs in the code and providing a solution 🙏.
Sorry, for not testing before pinging you. I did not have the time for it.

frontend/javascripts/oxalis/view/left-border-tabs/modals/add_volume_layer_modal.tsx

daniel-wer

Great fixes, everything works as advertised 👍

Sorry, for not testing before pinging you. I did not have the time for it.

No worries :)

frontend/javascripts/oxalis/view/left-border-tabs/modals/add_volume_layer_modal.tsx

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

…type * 'master' of github.com:scalableminds/webknossos: Ensure unique volume layer names when changing names in frontend (#6289) X-Auth Tokens everywhere (in datastore & tracingstore) (#6312) add mapping name param for rawcuboid (#6311) Fix zooming out for datasets with very large scale (#6304) Release 22.07.0 (#6307) added Sahil's publication to list

ensure unique volume layer names when changing names

5d52694

MichaelBuessemeyer added enhancement frontend labels Jun 20, 2022

MichaelBuessemeyer self-assigned this Jun 20, 2022

MichaelBuessemeyer added 4 commits June 20, 2022 19:36

fix duplication warning of layer names in layer settings not showing

83bf4d2

disallow slashes in volume layer names

87c12df

- refactor layer name validation functions

Merge branch 'master' of github.com:scalableminds/webknossos into uni…

caad094

…que-annotation-layer-names

Add changelog entry

2bd64a0

MichaelBuessemeyer commented Jun 21, 2022

View reviewed changes

MichaelBuessemeyer marked this pull request as ready for review June 21, 2022 12:07

MichaelBuessemeyer requested a review from daniel-wer June 21, 2022 12:08

daniel-wer requested changes Jun 22, 2022

View reviewed changes

MichaelBuessemeyer added 3 commits June 27, 2022 10:47

Merge branch 'master' of github.com:scalableminds/webknossos into uni…

fcc0551

…que-annotation-layer-names

apply pr feedback

b3ae26b

detect unsave uri characters via regex

e281c4a

MichaelBuessemeyer commented Jun 27, 2022

View reviewed changes

daniel-wer requested changes Jun 27, 2022

View reviewed changes

apply pr feedback

e455e69

MichaelBuessemeyer commented Jun 28, 2022

View reviewed changes

frontend/javascripts/oxalis/view/left-border-tabs/modals/add_volume_layer_modal.tsx Outdated Show resolved Hide resolved

daniel-wer approved these changes Jun 29, 2022

View reviewed changes

frontend/javascripts/oxalis/view/left-border-tabs/modals/add_volume_layer_modal.tsx Outdated Show resolved Hide resolved

frontend/javascripts/oxalis/view/left-border-tabs/modals/add_volume_layer_modal.tsx Outdated Show resolved Hide resolved

MichaelBuessemeyer and others added 2 commits July 4, 2022 11:56

Apply suggestions from code review

91180fc

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

Merge branch 'master' into unique-annotation-layer-names

a65f875

MichaelBuessemeyer merged commit ed90316 into master Jul 4, 2022

MichaelBuessemeyer deleted the unique-annotation-layer-names branch July 4, 2022 10:59

fm3 mentioned this pull request Jul 4, 2022

Make annotation layer names unique per annotation #6251

Closed

fm3 mentioned this pull request Jul 11, 2022

Make Annotation Layer Names Unique in DB #6326

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure unique volume layer names when changing names in frontend #6289

Ensure unique volume layer names when changing names in frontend #6289

MichaelBuessemeyer commented Jun 20, 2022 •

edited by daniel-wer

Loading

MichaelBuessemeyer Jun 21, 2022

daniel-wer left a comment

MichaelBuessemeyer left a comment

daniel-wer left a comment

MichaelBuessemeyer left a comment

daniel-wer left a comment

Ensure unique volume layer names when changing names in frontend #6289

Ensure unique volume layer names when changing names in frontend #6289

Conversation

MichaelBuessemeyer commented Jun 20, 2022 • edited by daniel-wer Loading

URL of deployed dev instance (used for testing):

Steps to test:

Issues:

MichaelBuessemeyer Jun 21, 2022

Choose a reason for hiding this comment

daniel-wer left a comment

Choose a reason for hiding this comment

MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

daniel-wer left a comment

Choose a reason for hiding this comment

MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

daniel-wer left a comment

Choose a reason for hiding this comment

MichaelBuessemeyer commented Jun 20, 2022 •

edited by daniel-wer

Loading