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

Delegate error printing to the ajax caller. #2142

Merged
merged 2 commits into from
Apr 15, 2022
Merged

Delegate error printing to the ajax caller. #2142

merged 2 commits into from
Apr 15, 2022

Conversation

Aiosa
Copy link
Contributor

@Aiosa Aiosa commented Apr 7, 2022

By default, OSD prints an error if ajax call results in an error response code (other than 2xx). This is a bit problematic since one can use custom error codes to route the behaviour and the console gets bloated by unnecessary error messages (I am having a scenario where tiles are expected to be very unpredictable, and the system can automatically detect that and remove the tiled image: having hundreds identical errors in the console does not help much). Other problem is that each error response generates two error messages, following the how not to scenario:

try:
    raise Error("Test error")
except e:
    print(str(e))  # do not print error messages if the routine error handling happens elsewhere
    raise e

so the ajax function should print an error only if no error handler is registered, otherwise, it should be the handler's responsibility to do so. Following the future PR I am planning, the user will have also the ability to re-define how tiles are downloaded and thus control the errors completely.

Copy link
Member

@iangilman iangilman left a comment

Choose a reason for hiding this comment

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

Awesome... Great cleanup!

Copy link
Member

@iangilman iangilman left a comment

Choose a reason for hiding this comment

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

Actually, one thought...

}

$.console.error(msg);
Copy link
Member

Choose a reason for hiding this comment

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

Should we not write to the console here if somebody has subscribed to the open-failed event? We may need to add something like hasHandler to EventSource to make that possible, but it shouldn't be hard.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, maybe not... The problem is open-failed event is registered inside OpenSeadragon to print the message into the error html container, so in 99,9999% scenarios this equals to not logging the message at all...

@Aiosa
Copy link
Contributor Author

Aiosa commented Apr 13, 2022

I found out while adding test case for this commit (yeah, should do it in one commit) that OSD actually uses the event internally, so the event handler count is almost always > 0 and therefore the error gets never printed... I would revert the last commit and just go with it as it was... I mean 'open' event happens once per tile source, so it is not that big a problem, unlike the image tile fetching itself. What do you think?

@iangilman
Copy link
Member

Good catch! Yeah, please undo that change. Might as well keep numberOfHandlers though; seems like it could be useful in the future (if even just for testing).

…w message style. Add 'numberOfHandlers()' method for events.
@Aiosa
Copy link
Contributor Author

Aiosa commented Apr 15, 2022

Done

Copy link
Member

@iangilman iangilman left a comment

Choose a reason for hiding this comment

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

Fantastic! Thank you for the tests for numberOfHandlers :)

@iangilman iangilman merged commit 334e8fb into openseadragon:master Apr 15, 2022
iangilman added a commit that referenced this pull request Apr 15, 2022
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.

2 participants