Metadata for Segments & Trees

[fm3]

URL of deployed dev instance (used for testing):

• https://___.webknossos.xyz

Steps to test:

• create an annotation
• create trees and segments
• use the new interface to assign metadata to segments and trees
• use all different value types
• try out invalid values (namely, duplicate keys and a string in a number value)
• save & reload
• also do this in the dashboard for folders and metadata, since the code of that UI also changed in the name of feature unification

TODOs:

• Backend
  • Introduce properties in proto definitions
  • Update actions (Create+Update for Segment,Node,Tree)
  • NML Writer
  • NML Parser
  • Unit tests
  • Snapshots
  • Add assertions on duplicate keys? Might break annotation reloading… → we decided to go for deduplication to avoid inconsistent states and deadlocks
  • Adapt NML Writer/Parser with new format (parent tag)
• Frontend
  • UI to display/edit properties
    • segments
    • trees
    • [ ] nodes -> follow-up
    • all tag types
      • string
      • number
      • list
      • [ ] boolean -> follow-up
    • iterate
  • Update actions
  • NML Spec
  • NML Writer
  • NML Parser
• follow-up issue for libs → Access Metadata of Segments & Trees in Annotations webknossos-libs#1195
• follow-up end-to-end tests → e2e tests for segment + tree metadata #8089

Issues:

• fixes Allow to assign key/value properties to trees, nodes and segments #7483

---

• Updated changelog
• Considered common edge cases

[fm3]

I created a first version of this in the backend. It works for Trees, Nodes and Segments (not yet for groups).

In proto, each of these now has a field repeated UserDefinedPropertyProto userDefinedProperties, a type defined as

message UserDefinedPropertyProto {
  required string key = 1;
  optional string stringValue = 2;
  optional bool boolValue = 3;
  optional double numberValue = 4;
  repeated string stringListValue = 6;
}

They can be set both in the “Create” and “Update” UpdateActions of the respective items. For example, the UpdateNodeSkeletonAction now has an optional field userDefinedProperties: Option[Seq[UserDefinedProperty]] = None (None has the same semantic as empty list). The format for a UserDefinedProperty expected in the json update actions is

key: String,
stringValue: Option[String] = None,
boolValue: Option[Boolean] = None,
numberValue: Option[Double] = None,
stringListValue: Option[Seq[String]] = None

The backend expects all update actions to include the full list for the edited item. So if it was nonempty first and is then updated to None, it is all cleared.

The frontend should ensure that only one of stringValue, boolValue, numberValue, stringListValue is set for each item, and that each key is used only once per item.

I also updated the NML format as written and parsed by the backend to include the userDeinedProperties. Looks like this:

    <userDefinedProperty key="aKey" numberValue="5.7"/>
    <userDefinedProperty key="anotherKey" boolValue="true"/>
    <userDefinedProperty key="thirdKey" stringListValue-0="hello"/>
    <userDefinedProperty key="fourthKey" stringListValue-1="there"/>

[philippotto]

@fm3 I have two questions regarding the nml format:

1. what do you think about having a parent tag for the user defined properties? e.g.:

<thing ...>
  <nodes>...</nodes>
  <edges>...</nodes>
  <userDefinedProperties>
    <userDefinedProperty ... />
    ...
  </userDefinedProperties>
</thing>

I think this would fit more nicely with the existing structure, but I'm happy to be convinced otherwise.

2. Your example for stringListValue uses indices 0 and 1 with different keys. Did you mean it like this?

    <userDefinedProperty key="thirdKey" stringListValue-0="hello" stringListValue-1="there" />

Having all values for the array in the same XML tag would be easier to parse for the front-end because we process each xml tag individually. also it would be less redundant because the key would only be stated once.

what do you think?

[fm3]

a parent tag for the user defined properties

Sure, no objections :)

Did you mean it like this?

Yes!

[MichaelBuessemeyer]

@fm3 Can I review the backend part?

[fm3]

Yes. There is on open checkbox about which I am unsure (“Add assertions on duplicate keys”). Since the update actions are applied lazily, adding assertions there will break the next annotation loading. On the other hand, maybe there should rather be a readable error message than an inconsistent state. What do you think? I think for the other update actions, the backend does not do much checking 🤔

Other than that, the backend changes should be complete.

[MichaelBuessemeyer]

What do you think? I think for the other update actions, the backend does not do much checking 🤔

I am unfamiliar with the backend code responsible for receiving the update actions. But in case the code iterates over the actions, one could run a simple duplicate key assertion over the metadata. Of cause, this needs to be done before the update actions are stored to be lazily applied.

Another way to handle this could be to simply choose the first occurring entry with a key when applying the metadata update in the fossildb. Meaning dropping the duplicate key. This is already how the frontend behaves and I would say that this is fair to handle a "broken" update action this way.

On the other hand, maybe there should rather be a readable error message than an inconsistent state.

How do you expect the user to escape from this error message state and recover such a broken annotation?

[MichaelBuessemeyer]

Checked frontend code again and works 🎉

[MichaelBuessemeyer]

sorry, misclick 🙈, looking at the backend code now. I'll look at the "assertion" changes when they are pushed :)

Just to do the first review while I am still in context of this pr :)

[fm3]

Another way to handle this could be to simply choose the first occurring entry with a key when applying the metadata update in the fossildb. Meaning dropping the duplicate key. This is already how the frontend behaves and I would say that this is fair to handle a "broken" update action this way.

That’s a good plan, I’ll go for that

[MichaelBuessemeyer]

Backend looks good except for the fact, that it supports metadata for treenodes but the frontend doesn't. I am unsure wherther to keep it.

And the changelog entry is missing. Will check your newest commit now :)

[MichaelBuessemeyer]

• Ok changelog entry is done already :D
• Found one occurrence where the deduplication was still missing

Ok and lets wait for your answer on:

Backend looks good except for the fact, that it supports metadata for treenodes but the frontend doesn't. I am unsure wherther to keep it.

Before I give it a final green :D

[MichaelBuessemeyer]

I just noticed that the border around the empty metadata placeholder in the dashboard is gone:

Maybe change it back to:

The background gray is consistent with the gray used in the metadata table (at least it looks like that)

[fm3]

border around the empty metadata placeholder in the dashboard is gone

@MichaelBuessemeyer good catch, maybe you could fix that here directly?

[MichaelBuessemeyer]

@MichaelBuessemeyer good catch, maybe you could fix that here directly?

Yeah sure, do you think it looks visually fine this way? I mean the second image from my previous post

[fm3]

Yeah sure, do you think it looks visually fine this way? I mean the second image from my previous post

I think it’s fine, yes. Mostly it’s consistent. We can discuss later if we want to adapt the visual design again. (In fact, I’d probably vote to remove the illustration entirely. But not in this PR)

[MichaelBuessemeyer]

Then mergy merge?