Add annotation color as a tool in the layer's render tab #692
Conversation
annotation layers
When a key is inserted in the localToolBinder, the "toJSON()" method is called on the tool. The JSON retrieved from this call is stringyfied and used as key. The "toJSON()" method acts differently on Tool and LegacyTool. When a LayerTool (inheriting from Tool) is created, the "jsonId" is returned by "toJSON()", the strinfigication of this value leads then to insert in the localToolBinder map "mykey" (with quotes) instead of simply mykey. This result in the need to explicitally use "JSON.stringify(...)" on a key before calling "removeJsonString(...)", which is not ideal. This patch on "removeJsonString" searches first for the key and fallsback on the "JSON.stringify(...)" if the key cannot be found in the first place.
CC-201 Style tooltip in local annotation layer
|
Hi @jbms, I just realized I forgot to add you as a reviewer on this PR, but unfortunately, I cannot do it by modifying the PR for some reasons. |
| @@ -455,6 +465,9 @@ export class AnnotationUserLayer extends Base { | |||
| this.annotationDisplayState.shaderControls.restoreState( | |||
| specification[SHADER_CONTROLS_JSON_KEY], | |||
| ); | |||
| this.annotationDisplayState.color.restoreState( | |||
There was a problem hiding this comment.
This is already restored by UserLayerWithAnnotationsMixin which is a base class.
| @@ -709,6 +722,7 @@ export class AnnotationUserLayer extends Base { | |||
| x[SHADER_JSON_KEY] = this.annotationDisplayState.shader.toJSON(); | |||
| x[SHADER_CONTROLS_JSON_KEY] = | |||
| this.annotationDisplayState.shaderControls.toJSON(); | |||
| x[ANNOTATION_COLOR_JSON_KEY] = this.annotationDisplayState.color.toJSON(); | |||
There was a problem hiding this comment.
as above, this is redundant with the base class
| title: | ||
| "Annotation shader default color (enabled with 'defaultColor()' in the shader code)", | ||
| toolJson: ANNOTATION_COLOR_JSON_KEY, | ||
| ...colorLayerControl( |
There was a problem hiding this comment.
The isValid property can be specified here instead of the hasDefaultColor logic above.
| @@ -531,7 +531,9 @@ export class LocalToolBinder< | |||
| } | |||
|
|
|||
| removeJsonString(toolJsonString: string) { | |||
| const key = this.jsonToKey.get(toolJsonString); | |||
| const key = | |||
There was a problem hiding this comment.
Is this change just to address the removeJsonString(ANNOTATION_COLOR_JSON_KEY); call that you added, or were there existing cases with this problem? This is supposed to receive the encoded JSON representation, so any JSON.stringify should be happening in the caller, not here.
|
Thanks for your work on this. For AnnotationUserLayer this seems like a good change. However, note that other layers also have an Annotations tab due to UserLayerWithAnnotationsMixin, which normally just shows the overall bounds of the data for that layer. For those layers, if we remove the color selector from the Annotations tab then we would need to put it somewhere else --- currently you have added it to the Rendering tab for AnnotationUserLayer only, but not to the Rendering tab of other layers that inherit from UserLayerWithAnnotationsMixin. We could either leave the color selector on the Annotations tab for non-AnnotationUserLayer layers, or could move it to the Rendering tab on those layers. Not sure what the best option is. It may be slightly undesirable to clutter the rendering tab on e.g. ImageUserLayer and SegmentationUserLayer, which already have many controls, with something that most users won't care about. |
Summary
This PR moves the widget for color management for local annotation from the annotation tab to the rendering tab, improving UI organization and aligning widget placement with its functionality. The local annotation default color widget is also now part of a
Toolso that it can be used in the new tool palettes.Additionally, this PR fixes an issue in the
removeJsonStringfunction to handle tool keys, resolving inconsistencies caused by differences in thetoJSON()method acrossToolandLegacyToolclasses.User Interaction
Widget Relocation: The widget previously located in the state tab is now found in the rendering tab as a tool. This change simplifies navigation for users working with segment-related settings.
To notify the user that the default color widget moved to the "rendering tab", a new icon with a question mark is displayed in the local-annotation layer, next to the buttons that allows to create new annotations, with a tooltip stating what are the buttons used for, and where is located the color control.
Implementation
Widget Relocation:
Code Changes: Updates in
layer/annotation/index.tsmoved the initialization and rendering logic of the widget to the rendering tab.removeJsonStringFix:Problem: The
toJSON()method forToolandLegacyToolbehaves differently. ForLayerTool(inheriting fromTool),toJSON()returns ajsonIdvalue, causing keys inlocalToolBinderto include quotes (e.g.,"mykey"instead ofmykey). This created mismatches when usingremoveJsonString(), as it required explicitJSON.stringify()calls.Solution: The fix first attempts to locate the key directly. If it is not found, it falls back to checking the
JSON.stringify()version. This approach ensures compatibility with bothToolandLegacyToolimplementations.