Delay before hover preview #3531
Conversation
* Changes providers so they're not responsible for showing popover. They return an object with the info needed to show one later. * Add a 'popoverState' object that can represent both visible and pending (waiting on timer) popovers. * Pending popovers can wait on both a timeout delay AND an additional provider delay (needed for <img> loading). The wait can be canceled in case a pending popover is invalidated before being shown.
…inue to use its hack where it re-hides the popover as soon as it's shown (until image loaded); no separate preloading of image; only delay popover based on simple timeout (not compound of timeout + provider-specified delay). Also, ignore mousemove when a drag is in progress.
…w-delay * origin/master: (105 commits) Minor fixes after review update HTML file extensions for smarty and twig file extensions String in case we want to add a "More" link to get to the extension's home page Fix GitHub case String requested by adrocknaphobia Update 'de' locale Reworked the text of getting started make sure hex values end on boundary implement updated project panel styles Fix regressions in the display of literals and keywords in code hints. 1. keywords should be monospace. 2. literals should be dark grey. add new identifier colors from Lawrence Hsu. Properly force the Extension Manager to retrieve the registry on every dialog open Explain whether incompatible extensions require older or newer versions of Brackets. Remove hard-coded app name references in strings. Updated by ALF automation. Externalize strings from Extension Manager view item template define offset in constant fix problem where jQuery was not recognized as a builtin Increase max image size to 200x200 fixed padding add jquery api to hints ... Conflicts: src/extensions/default/HoverPreview/main.js
reduces the number of events handled by 8-10x while moving mouse around.
| @@ -22,7 +22,7 @@ | |||
| */ | |||
|
|
|||
| /*jslint vars: true, plusplus: true, devel: true, nomen: true, regexp: true, indent: 4, maxerr: 50 */ | |||
| /*global define, brackets, $, PathUtils, CodeMirror */ | |||
| /*global define, brackets, $, PathUtils, CodeMirror, setTimeout, clearTimeout */ | |||
There was a problem hiding this comment.
It might be better to use window.setTimeout and window.clearTimeout to have less global variables/functions.
There was a problem hiding this comment.
Good point -- that is how the majority of our modules do it. Will fix.
ghost
assigned …w-delay Still need to fix up Hover Preview unit tests to work post-merge. * origin/master: (22 commits) fix permissions on french translation Export all hover preview functions as private functions to unit tests. Update test file with some repeating-linear-gradients, repeating-radial-gradients, and some invalid color names. Update unit tests with the correct cursor positions for all unit tests. test popover positioning Few cleanups for OpenLine's tests Remove Italian strings for OpenLine. Add little changes to tests. Updated by ALF automation. french getting started translation from sprint 23 Include changes to OpenLinecode and new tests cleanup based on codereview comments: Adding image files used in unit tests Add unit tests for hover preview Add special case for last line in inline editor Initial test files creation Include TomMalbran's suggestions Remove cruft from the outdated stats-gathering branch. Add few improvements in OpenLine Fix OpenLine undo history Fix OpenLine problem with inline editors. Invert OpenLine shortcuts Improve Open Line feature. Add additional unit tests. ... Conflicts: src/extensions/default/HoverPreview/main.js
Update unit tests to use this new, cleaner way of getting info. Disabled some gradient unit tests where the provider is generating nonsensical CSS, pending resolution of #3535.
|
Merged with Raymond's unit tests. However, Raymond just merged some changes from Randy that require a second merge. Stand by... |
…w-delay - Be more explicit about testing the fix in pull #3482 - Add simple test for matched-text highlighting in UI - Cite bug numbers for all disabled tests * origin/master: fix unit tests for changes in this branch remove misguided attempt to filter gradients for Chrome 25 code cleanup ignore new gradient syntax, for now ignore new gradient syntax Conflicts: src/extensions/default/HoverPreview/main.js src/extensions/default/HoverPreview/unittests.js
|
@gruehle finally all merged up & ready for review |
| var coord = cm.charCoords(sPos); | ||
| var xpos = (cm.charCoords(ePos).left - coord.left) / 2 + coord.left; | ||
|
|
||
| var showHandler = function () { |
There was a problem hiding this comment.
The showHandler() is called after the delay, which means images won't be previewed until the delay time plus the time it takes for the image to be loaded. For small local images, this isn't a problem. For large remote images, this could mean a long delay before the image is previewed.
Here are a couple ways this could be mitigated:
- Add the HTML right away (which will initiate the load), but wait at least 350 milliseconds before showing it.
- Instead of hiding
$previewContainerhere, put a loading spinner that will get hidden when the image is loaded.
There was a problem hiding this comment.
I was thinking of it sort of as a feature that we don't kick off an image load right away on mouseover, since it could affect performance (including during scrolling, which has always been so sensitive). If the image is going to take an annoying length of time to load, the extra ~1/3 sec might not be noticeable anyway.
But your 2nd suggestion is the ideal in my mind. I held off on doing that originally because I was worried it'd make the pull request too disruptive... but happy to do it now.
There was a problem hiding this comment.
Yeah, option #2 seems like the best bet. If you have it ready, go ahead and add it in. If not, I'll file a bug and it can be done later.
|
Initial review complete. |
lines; don't add set/clearTimeout() to JSLint globals
|
@gruehle Pushed up responses to code review (including @TomMalbran's code-consolidation suggestion). I didn't do the spinner part yet since that seemed a little trickier and it sounded like we might want to land the rest of this sooner. |
|
Looks good. Merging. |
| } | ||
|
|
||
| } | ||
| return false; |
There was a problem hiding this comment.
It felt a little weird leaving this as an instance method of Editor given that it no longer has any dependency on Editor... but there didn't seem to be many cleaner homes for it (a static method on either Editor or Document feels imperfect too... and usually adds a module import that wasn't needed otherwise... but I'm open to moving it there if preferred).
popoverStateobject that can represent both visible and pending (waiting on timer) popovers. If mouse movement invalidates the popoverState, it is canceled -- hidden if visible, timer stopped if pending.