Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Delay before hover preview #3531

Merged
merged 9 commits into from
Apr 23, 2013
221 changes: 174 additions & 47 deletions src/extensions/default/HoverPreview/main.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be better to use window.setTimeout and window.clearTimeout to have less global variables/functions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point -- that is how the majority of our modules do it. Will fix.


define(function (require, exports, module) {
"use strict";
Expand All @@ -40,30 +40,70 @@ define(function (require, exports, module) {
var defaultPrefs = { enabled: true },
enabled, // Only show preview if true
prefs = null, // Preferences
previewMark, // CodeMirror marker highlighting the preview text
$previewContainer, // Preview container
$previewContent, // Preview content holder
currentImagePreviewContent = ""; // Current image preview content, or "" if no content is showing.
lastPos; // Last line/ch pos processed by handleMouseMove

// Constants
var CMD_ENABLE_HOVER_PREVIEW = "view.enableHoverPreview",
HOVER_DELAY = 350, // Time (ms) mouse must remain over a provider's matched text before popover appears
POSITION_OFFSET = 38, // Distance between the bottom of the line and the bottom of the preview container
POINTER_LEFT_OFFSET = 17, // Half of the pointer width, used to find the center of the pointer
POINTER_TOP_OFFSET = 7, // Pointer height, used to shift popover above pointer
POSITION_BELOW_OFFSET = 16; // Amount to adjust to top position when the preview bubble is below the text

/**
* There are three states for this var:
* 1. If null, there is no provider result for the given mouse position.
* 2. If non-null, and visible==true, there is a popover currently showing.
* 3. If non-null, but visible==false, there is a provider result but it has not been shown yet because
* we're waiting for HOVER_DELAY, which is tracked by hoverTimer. The state changes to visible==true as
* soon as hoverTimer fires. If the mouse moves before then, the popover will never become visible.
*
* @type {{
* visible: boolean,
* editor: !Editor,
* hoverTimer: number, - setTimeout() token
* start: !{line, ch}, - start of matched text range
* end: !{line, ch}, - end of matched text range
* content: !string, - HTML content to display in popover
* onShow: ?function():void, - called once popover content added to the DOM (may never be called)
* xpos: number, - x of center of popover
* ytop: number, - y of top of matched text (when popover placed above text, normally)
* ybot: number, - y of bottom of matched text (when popover moved below text, avoiding window top)
* marker: ?CodeMirror.TextMarker - only set once visible==true
* }}
*/
var popoverState = null;



// Popover widget management ----------------------------------------------

/**
* Cancels whatever popoverState was currently pending and sets it back to null. If the popover was visible,
* hides it; if the popover was invisible and still pending, cancels hoverTimer so it will never be shown.
*/
function hidePreview() {
if (previewMark) {
previewMark.clear();
previewMark = null;
if (!popoverState) {
return;
}

if (popoverState.visible) {
popoverState.marker.clear();

$previewContent.empty();
$previewContainer.hide();

} else {
clearTimeout(popoverState.hoverTimer);
}
$previewContent.empty();
$previewContainer.hide();
currentImagePreviewContent = "";

popoverState = null;
}

function positionPreview(xpos, ypos, ybot) {
var top = ypos - $previewContainer.height() - POSITION_OFFSET;
function positionPreview(xpos, ytop, ybot) {
var top = ytop - $previewContainer.height() - POSITION_OFFSET;

if (top < 0) {
$previewContainer.removeClass("preview-bubble-above");
Expand All @@ -83,11 +123,29 @@ define(function (require, exports, module) {
}
}

function showPreview(content, xpos, ypos, ybot) {
hidePreview();
$previewContent.append(content);
/**
* Changes the current hidden popoverState to visible, showing it in the UI and highlighting
* its matching text in the editor.
*/
function showPreview() {

var cm = popoverState.editor._codeMirror;
popoverState.marker = cm.markText(
popoverState.start,
popoverState.end,
{className: "hover-preview-highlight"}
);

$previewContent.append(popoverState.content);
$previewContainer.show();
positionPreview(xpos, ypos, ybot);

popoverState.visible = true;

if (popoverState.onShow) {
popoverState.onShow();
}

positionPreview(popoverState.xpos, popoverState.ytop, popoverState.ybot);
}

function divContainsMouse($div, event) {
Expand All @@ -99,6 +157,9 @@ define(function (require, exports, module) {
event.clientY <= offset.top + $div.height());
}


// Color & gradient preview provider --------------------------------------

function colorAndGradientPreviewProvider(editor, pos, token, line) {
var cm = editor._codeMirror;

Expand Down Expand Up @@ -144,20 +205,18 @@ define(function (require, exports, module) {
xPos;

xPos = (cm.charCoords(endPos).left - startCoords.left) / 2 + startCoords.left;
showPreview(preview, xPos, startCoords.top, startCoords.bottom);
previewMark = cm.markText(
startPos,
endPos,
{className: "hover-preview-highlight"}
);
return true;

return { start: startPos, end: endPos, content: preview, xpos: xPos, ytop: startCoords.top, ybot: startCoords.bottom };
}
match = colorRegEx.exec(line);
}

return false;
return null;
}


// Image preview provider -------------------------------------------------

function imagePreviewProvider(editor, pos, token, line) {
var cm = editor._codeMirror;

Expand Down Expand Up @@ -200,53 +259,83 @@ define(function (require, exports, module) {
var imgPreview = "<div class='image-preview'>" +
" <img src=\"" + imgPath + "\">" +
"</div>";
if (imgPreview !== currentImagePreviewContent) {
var coord = cm.charCoords(sPos);
var xpos = (cm.charCoords(ePos).left - coord.left) / 2 + coord.left;
var ypos = coord.top;
var ybot = coord.bottom;
showPreview(imgPreview, xpos, ypos, ybot);

var coord = cm.charCoords(sPos);
var xpos = (cm.charCoords(ePos).left - coord.left) / 2 + coord.left;

var showHandler = function () {
Copy link
Member

Choose a reason for hiding this comment

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

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:

  1. Add the HTML right away (which will initiate the load), but wait at least 350 milliseconds before showing it.
  2. Instead of hiding $previewContainer here, put a loading spinner that will get hidden when the image is loaded.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

// Hide the preview container until the image is loaded.
$previewContainer.hide();
$previewContainer.find("img").on("load", function () {

$previewContainer.find(".image-preview > img").on("load", function () {
$previewContent
.append("<div class='img-size'>" +
this.naturalWidth + " x " + this.naturalHeight + " pixels" +
"</div>"
);
$previewContainer.show();
positionPreview(xpos, ypos, ybot);
positionPreview(popoverState.xpos, popoverState.ytop, popoverState.ybot);
});
previewMark = cm.markText(
sPos,
ePos,
{className: "hover-preview-highlight"}
);
currentImagePreviewContent = imgPreview;
}
return true;
};

var popover = { start: sPos, end: ePos, content: imgPreview, onShow: showHandler, xpos: xpos, ytop: coord.top, ybot: coord.bottom };

return popover;
}
}
}

return false;
return null;
}

function queryPreviewProviders(editor, pos, token, line) {

// Preview hide/show logic ------------------------------------------------

/**
* Returns a 'ready for use' popover state object:
* { visible: false, editor, start, end, content, ?onShow, xpos, ytop, ybot }
* Lacks only hoverTimer (supplied by handleMouseMove()) and marker (supplied by showPreview()).
*/
function queryPreviewProviders(editor, pos, token) {

var line = editor.document.getLine(pos.line);

// FUTURE: Support plugin providers. For now we just hard-code...
if (!colorAndGradientPreviewProvider(editor, pos, token, line) &&
!imagePreviewProvider(editor, pos, token, line)) {
hidePreview();
var popover = colorAndGradientPreviewProvider(editor, pos, token, line) ||
imagePreviewProvider(editor, pos, token, line);

if (popover) {
// Providers return just { start, end, content, ?onShow, xpos, ytop, ybot }
$.extend(popover, { visible: false, editor: editor });

return popover;
}
return null;
}


/**
* Returns true if pos is contained within popover's start-end range (start inclusive, end exclusive)
*/
function containsPos(popover, pos) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be nicer if this used Editor.posWithinRange. Although that function has an inclusive end, but maybe the idea of using Editor.indexFromPos to check if pos is inside popover might make it look a lot simpler.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm... indexFromPos() doesn't look super efficient -- in CM, it appears to walk the document "chunk" structure from the start to the pos in order to accumulate an offset (three times, in the case of posWithinRange()). Since the mouseMove() code gets hit a lot it might be better to avoid that. I suppose I could just move my implementation into posWithinRange() and then it'd be (mostly) shared...

Copy link
Contributor

Choose a reason for hiding this comment

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

That is a nice idea, although in posWithinRange the end is inclusive, so it might need an extra parameter to have it exclusive for this case, which might make the code a bit more complicate to read, but it would be shared, and since posWithinRange is an useful API, the new addition might become useful too.

if (popover.start.line <= pos.line && popover.end.line >= pos.line) {
return (popover.start.line < pos.line || popover.start.ch <= pos.ch) && // inclusive
(popover.end.line > pos.line || popover.end.ch > pos.ch); // exclusive
}
return false;
}


function handleMouseMove(event) {
if (!enabled) {
return;
}

if (event.which) {
// Button is down - don't show popovers while dragging
hidePreview();
return;
}

// Figure out which editor we are over
var fullEditor = EditorManager.getCurrentFullEditor();

Expand Down Expand Up @@ -277,17 +366,53 @@ define(function (require, exports, module) {
}

if (editor && editor._codeMirror) {
// Find char mouse is over
var cm = editor._codeMirror;
var pos = cm.coordsChar({left: event.clientX, top: event.clientY});

if (lastPos && lastPos.line === pos.line && lastPos.ch === pos.ch) {
return; // bail if mouse is on same char as last event
}
lastPos = pos;

var showImmediately = false;

// Is there a popover already visible or pending?
if (popoverState) {
if (containsPos(popoverState, pos)) {
// That one's still relevant - nothing more to do
return;
} else {
// That one doesn't cover this pos - hide it and query providers anew
showImmediately = popoverState.visible;
hidePreview();
}
}

// Query providers for a new popoverState
var token = cm.getTokenAt(pos);
var line = cm.getLine(pos.line);
popoverState = queryPreviewProviders(editor, pos, token);

if (popoverState) {
// We have a popover available - wait until we're ready to show it
if (showImmediately) {
showPreview();
} else {
popoverState.hoverTimer = setTimeout(function () {
// Ready to show now (we'll never get here if mouse movement rendered this popover
// inapplicable first - hidePopover() cancels hoverTimer)
showPreview();
}, HOVER_DELAY);
}
}

queryPreviewProviders(editor, pos, token, line);
} else {
// Mouse not over any Editor - immediately hide popover
hidePreview();
}
}


// Menu command handlers
function updateMenuItemCheckmark() {
CommandManager.get(CMD_ENABLE_HOVER_PREVIEW).setChecked(enabled);
Expand All @@ -298,6 +423,8 @@ define(function (require, exports, module) {
enabled = _enabled;
var editorHolder = $("#editor-holder")[0];
if (enabled) {
// Note: listening to "scroll" also catches text edits, which bubble a scroll event up from the hidden text area. This means
// we auto-hide on text edit, which is probably actually a good thing.
editorHolder.addEventListener("mousemove", handleMouseMove, true);
editorHolder.addEventListener("scroll", hidePreview, true);
} else {
Expand Down