Closed manics closed 3 years ago
Could you add a bunch of comments to say what the code is doing / what the logic is? Including the whole block, starting at line 100. It took me a long time to understand that this is searching for rows that contain a URL (with some criteria?), grabbing the URL from there and using it to create an icon on the 'target' row and then hiding the URL row. Thanks.
👍 Thanks
Tested with (merge-ci) and without this PR (latest-ci) against two Web deployment both configured with
omero config append omero.web.mapr.config '{"menu": "gene","config": {"default": ["Gene Symbol"],"all": ["Gene Symbol", "Gene Identifier"],"ns": ["openmicroscopy.org/mapr/gene"],"label": "Gene"}}'
Test scenarios were generated by creating map annotation under the gene namespace and attaching it to an image for visual inspection
omero obj new MapAnnotation ns=openmicroscopy.org/mapr/gene
omero obj new ImageAnnotationLink parent=Image:5259 child=MaAnnotation:10375
and then adjusting the key/value pairs using omero obj map-set
to test the following key-value combinations:
Screen | latest-ci | |
---|---|---|
Gene Symbol: FIG1 | Gene Symbol: FIG1 | Gene Symbol: FIG1 |
Gene Symbol URL: https://www.yeastgenome.org/locus/YBR040W |
Gene Symbol URL: icon | Gene Symbol URL: icon |
Gene Symbol URL: https://www.yeastgenome.org/locus/YBR040W#FIG1 |
Gene Symbol URL: icon | Gene Symbol URL: icon |
Gene Symbol: FIG1 Gene Symbol URL: https://www.yeastgenome.org/locus/YBR040W |
Gene Symbol: FIG1 Gene Symbol URL: icon |
Gene Symbol: FIG1 icon |
Gene Symbol: FIG1 Gene Symbol URL: https://www.yeastgenome.org/locus/YBR040W#FIG1 |
Gene Symbol: FIG1 icon | Gene Symbol: FIG1 icon |
Gene Symbol: FIG1 Gene Symbol URL: https://www.yeastgenome.org/locus/YBR040W#FIG1 Gene Identifier: YBR040W |
Gene Symbol: FIG1 icon Gene Identifier: YBR040W40W |
Gene Symbol: FIG1 icon Gene Identifier: YBR040W40W |
Gene Symbol URL: https://www.yeastgenome.org/locus/YBR040W#FIG1 Gene Symbol: FIG1 |
Gene Symbol: FIG1 | Gene Symbol URL: icon Gene Symbol: FIG1 |
Gene Symbol: FIG1 Gene Identifier: YBR040W Gene Symbol URL: https://www.yeastgenome.org/locus/YBR040W#FIG1 |
Gene Symbol: FIG1 icon Gene Identifier: YBR040W40W |
Gene Symbol: FIG1 Gene Identifier: YBR040W40W Gene Symbol URL: icon |
Back to the original issue, this table shows the restriction is lifted and the inline iconification happens independently of the URL.
The only question I have is related to the added expectation in terms of the ordering of the keys, namely the last two lines of this table. It seems that the previous behavior was to look for the first subsequent URL key while this PR restricts the logic to the next immediate key. While this makes sense in terms of good practice, this could be seen as a breaking behavior for mapr implementations which would not have strictly ordered their key and would force them to rewrite their map annotations in order to restore compatibility.
From my testing perspective, if we lifted the previous restriction to search for the first following URL key, I would be inclined to consider this as a bug fix rather than a breaking change and release as 0.4.1
. Otherwise, this should probably be treated as a 0.5.0
.
Pushed a fix to https://github.com/will-moore/omero-mapr/commits/mapr-url-find so the 'Key' and 'Key URL' rows don't have to be adjacent. For any 'Key URL' row, we search previous rows in turn until we find 'Key' row. If not found then the 'Key URL' row with the icon-link will simply remain in place and visible.
Thanks, I've added it. Sorry for the delay, I assumed you could push directly to my branch since Allow edits by maintainers
is ticked.
Retested the last commits. Both combinations of Gene Symbol, Gene Identifier, Gene Symbol URL
and Gene Symbol, Gene Symbol URL, Gene Identifier
result in the UI below
Deployed on the idr0094 server
A mapr key of the form
xxx URL
is now only matched with a preceding row with keyxxx
. This is a change to the previous behaviour where all(?) rows were searched for a matching substring value.Closes https://github.com/ome/omero-mapr/issues/62