-
Notifications
You must be signed in to change notification settings - Fork 185
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
Fix: loadFullSizeImageImageTimeout not clearing when mouse leaves image #1516
Merged
extesy
merged 8 commits into
extesy:master
from
LiliaDoe:Fix-loadFullSizeImageImageTimeout-not-clearing-when-mouse-leaves-image
Dec 31, 2024
Merged
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
6180193
Fix: loadFullSizeImageImageTimeout not clearing when mouse leaves image
LiliaDoe 5838a0c
Add comment
LiliaDoe b81de22
fix typo
LiliaDoe d285a98
Revert "Fix: loadFullSizeImageImageTimeout not clearing when mouse le…
LiliaDoe 2f2017a
Temporarily remove message sending block
LiliaDoe 415a662
add timeout
LiliaDoe 7dd42b7
Format change
LiliaDoe bf18f5e
Update hoverzoom.js
extesy File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I'm not quite sure I understand how this works. Isn't
chrome.runtime.sendMessage
asynchronous? I mean it returns immediately and at some later point invokes a call back function. Which means line 1096 could checkloadHoveredImage
value before the callback has opportunity to set it.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.
That is true, and that is also why the bug was happening, i think. The timeout id being set by setTimeout wasn't always being cleared by clearTimeout due to it being async. Maybe I'm misunderstanding why the bug was happening, though. I'm sure there is a better way of solving this
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.
I think it's better to temporarily remove that entire message sending block until a proper solution is found. Right now the value of
loadHoveredImage
would be reused from the previous image (because the current image would only set it after it was already checked) which is not correct.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.
If it's temporary, I could comment it out for now like
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.
I'd imagine @GrosPoulet would want to make it work at some point so commenting out seems fine to me.
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.
@extesy @LiliaDoe Happy New Year 🎉
I uncommented L1076-1088 and i can not reproduce the issue on Reddit or any other website. I have a 1 second delay before picture display.
My code:
`
// Temporarily removing until a better fix is found: sendMessage is async so it can't be used to set local variables
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.
@GrosPoulet I posted an easy repro steps in #1515 (comment) - please take a look. Anyhow, the code as you wrote it couldn't possibly work due to async nature of sendMessage() function.