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

Fixed #549 and refactored the image tooltip code #558

Merged
merged 1 commit into from Apr 23, 2019

Conversation

worstje
Copy link
Contributor

@worstje worstje commented Apr 21, 2019

Issue #549 was caused because the request and response objects are not garantueed to be the same. Redirects are the most common cause, but a malformed URL apparently also qualifies. We now make sure to look at the original request.

The code confused me a bit while I was working on it, and I decided to refactor and document it so I could understand what was going on. I am glad I did: I stumbled across another bug involving the rapid-firing of tooltip requests, and the processing dict never had its entries removed either, leading to a (very slow) memory leak.

@gedakc
Copy link
Collaborator

gedakc commented Apr 23, 2019

Thank you @worstje for fixing the issue #549 crash. In my testing your code enhancement did indeed prevent the issue #549 crash.

Good catch on lower case letter starting the name of a class ("imageTooltiper" versus "ImageTooltip"). I also appreciate the addition of comments.

When reviewing the code I came across one spelling mistake in the commit message ("garantueed" instead of "guaranteed").

Would you like to fix the spelling mistake before I commit this PR?

Issue olivierkes#549 was caused because the request and reply object urls are not
guaranteed to be the same. Redirects are the most common cause, but a
malformed URL apparently also qualifies. We now make sure to look at the
original request.

Because the code confused me while I was working on it, I decided to
refactor and document it in order to understand what was going on. I am
glad I did: I found another crashing bug involving the rapid-firing of
tooltip requests, and the processing dict never had its entries removed
either, leading to a (very slow) memory leak over time.

All is good in the world of image tooltips now.
@worstje
Copy link
Contributor Author

worstje commented Apr 23, 2019

That word gets me every single time. No matter how many times I check and re-check, I always end up forgetting and doubting which version I need. And the one time I think I do know... I get it wrong again.

Fixed. Sorry about that.

@gedakc
Copy link
Collaborator

gedakc commented Apr 23, 2019

Thanks for the update. I'll do a quick retest, and after the Travis CI processes complete I will plan to merge this PR.

@gedakc gedakc merged commit 0238cce into olivierkes:develop Apr 23, 2019
@gedakc gedakc added this to the 0.10.0 milestone Apr 23, 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.

None yet

2 participants