-
Notifications
You must be signed in to change notification settings - Fork 30
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
validate zoomLevelScaling #359
Conversation
This pull request has been mentioned on Image.sc Forum. There might be relevant details there: https://forum.image.sc/t/omero-iviewer-shows-blank-for-some-images/44631/7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imported the test image, and it's working today on merge-ci. I'll leave the code review to someone else.
for (var r in this.image_info_['zoomLevelScaling']) | ||
tmp.push(1 / this.image_info_['zoomLevelScaling'][r]); | ||
for (var r in this.image_info_['zoomLevelScaling']) { | ||
var scale = 1 / this.image_info_['zoomLevelScaling'][r]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to avoid surprises, I think we need to add a check that this.image_info_['zoomLevelScaling'][r]
is not 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These values come from the server via https://github.com/ome/omero-py/blob/99ff5fd87b327a774dc55613b5d639fce93f4eaf/src/omero/gateway/__init__.py#L8714.
So it is very unlikely we will get a 0
here, and if so then there is something very wrong with the image so we won't be able to provide an alternative value here that will allow the image to be viewed.
And I think it's a bit of an edge case to be implementing an error dialog etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you add just a note unlikely to get 0
coming from ...
so we know that is expected
Fixes #358.
To test: