Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"Others" annotation display tweaks #530

Merged
merged 2 commits into from
Mar 12, 2024

Conversation

DavidStirling
Copy link
Contributor

@DavidStirling DavidStirling commented Feb 6, 2024

Lately we've been experimenting with ListAnnotation objects as a means to group other annotations together. Within OMERO.web's right panel these are apparently displayed under the "Others" section when populating the annotation list.

My understanding is that this section is intended to provide a basic display of miscellaneous or custom annotation types. The display attempts to show an entry for each with the format "Type: Value".

However, for ListAnnotations specifically there is no actual value field associated with them. The end result is this:

Screenshot 2024-02-06 at 09 35 33

To further complicate things, the tooltips providing more details are bound to the value field, meaning that the user cannot see any further details about these annotations. Attach 10 ListAnnotation objects and you'll just see the word "List" 10 times.

To attempt to improve this I've modified the code that handles this to fall back to using the 'name' field if no other value fields are present. This field is common to all annotation types (though optional) and might be a sensible thing to display in the absence of anything else. The result would be:

Screenshot 2024-02-06 at 09 35 15

Having some data in the name field also then restores the ability to view tooltips.

Unfortunately the webclient json API for annotations currently seems to return everything except for the name field, I'm not sure why. For the sake of this PR I've added that field to the returned results. Hope that doesn't break anything.

Testing: Create ListAnnotations with name/description/namespace info and attach them to an object. Without PR: OMERO.web right panel only shows "List" for each. With PR: Right panel shows "List: [name]". Other BasicAnnotation subtypes should still display their respective values, though their tooltips will now include the "name" field.

Copy link
Member

@sbesson sbesson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a code perspective, the changes proposed here make sense to me. The name attribute has been introduced on all annotations in OMERO 5.1.0 (and specifically ome/openmicroscopy#3362), so I suspect the code had simply never been updated accordingly.

Adding a few ListAnnotation objects using

omero obj new DatasetAnnotationLink parent=<Object>:<id> child=$(omero obj new ListAnnotation name=<name> ns=<ns>)

the UI now displays the new elements with the name populated

Screenshot 2024-02-07 at 08 51 36

@sbesson sbesson requested review from knabar and will-moore February 7, 2024 08:55
@DavidStirling
Copy link
Contributor Author

Thanks for the background @sbesson, I guess that does raise whether it's worth also adding the "name" field to tooltips for other annotation types? For this PR I only updated the "custom annotation" template.

@will-moore
Copy link
Member

There are a bunch of tests failing at https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-test-integration/349/ due to the addition of Name: None to the JSON.

Need to update the expected JSON values at e.g. https://github.com/ome/openmicroscopy/blob/develop/components/tools/OmeroWeb/test/integration/test_tree_annotations.py#L307

@DavidStirling
Copy link
Contributor Author

DavidStirling commented Feb 7, 2024

Okay, pushed a revised template which should now handle everything as proposed above instead of fudging the 'value' parameter.

This should now also handle cases where BasicAnnotation subtypes have missing values, like so:

Screenshot 2024-02-07 at 10 52 53

There's probably more useful info for Lists that could be shown as Will mentioned, but for now I hope this will suffice. If ListAnnotations start getting used more in practice then it might be something for a new PR.

Didn't realise that tests are in a different repo, I guess I should open a companion PR there?

@will-moore
Copy link
Member

Yes, you'll need a companion PR for tests. Legacy of omero-web once being in the openmicroscopy repo.

Copy link
Member

@will-moore will-moore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks.
I think the tests didn't run last night but should be fixed...

@knabar knabar added this to the 5.25.0 milestone Mar 12, 2024
@knabar knabar merged commit 71372e9 into ome:master Mar 12, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants