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

Remove id from image link description #339

Merged
merged 2 commits into from
Oct 3, 2019

Conversation

will-moore
Copy link
Member

This uses "Image:123" instead of "Image ID: 123" in the description of images created from figures.
As suggested (and supported) in ome/openmicroscopy#6087 (comment)

NB: That PR is not merged just now, so links aren't rendered, but layout looks cleaner:

Screen Shot 2019-08-30 at 11 17 41

@joshmoore
Copy link
Member

NB: That PR is not merged just now, so links aren't rendered, but layout looks cleaner:

Similar to ome/omero-scripts#152 (comment), we need to consider how this kind of statement will be communicated to users. e.g. someone who updates figure without updating the related server version will lose their links.

@will-moore
Copy link
Member Author

The rendering of Image: 123 to html link is not an essential feature from OMERO.figure point of view. The adding of Image references to the Description field was more about keeping track of the data provenance. So that will be the focus of the Figure release announcement.

The rendering of links has not previously been supported in webclient, so no one will lose their links by not using ome/openmicroscopy#6087.

@joshmoore
Copy link
Member

The rendering of Image: 123 to html link is not an essential feature from OMERO.figure point of view...

Fair enough.

The rendering of links has not previously been supported in webclient, so no one will lose their links

Ok. But perhaps a surprise for insight users. Mostly, I'm trying to stress that for any of these loosely-coupled dependencies (scripts, text-based formats, etc) we need to have solid strategies for dealing with breaking changes.

@jburel
Copy link
Member

jburel commented Sep 12, 2019

The space is not compliant with what was introduced a while ago when projecting an image from insight
Insight will not be able to currently handle the link without some adjustments
If we go for no space, we need to handle both case.

@will-moore
Copy link
Member Author

OK, I guess I'm fine with it either way.
Image:123 is nice because it's slightly more concise (easier to remember and not make typos) and the same format is supported in CLI etc.
Does support for that need back-porting to the "deprecated" Insight?

@jburel
Copy link
Member

jburel commented Sep 16, 2019

The fact that it unifies the passing of argument is a valid point CLI, figure etc. will then follow the same pattern.
This means that an adjustment should be made in insight and we will have to handle both space/no space in the UI

@jburel
Copy link
Member

jburel commented Sep 16, 2019

See insight adjustment ome/omero-insight#78

@will-moore
Copy link
Member Author

webclient PR to render links is now available for testing ome/omero-web#16

@jburel
Copy link
Member

jburel commented Sep 19, 2019

tested against
https://merge-ci.openmicroscopy.org/web/webclient/?show=image-91366
with and without ID in the description

@jburel jburel merged commit de6536c into ome:master Oct 3, 2019
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.

3 participants