Reveal cursor on panel expand

[albertxing]

@peterflynn Simple fix for issue #3003, and (nearly) identical to closed and outdated pull request #3931.

[busykai]

@albertxing, i believe this should be done in EditorManager._onEditorAreaResize. As far as I can tell, there's nothing specific to the panel manager that is needed to calculate if the cursor is outside the editor viewport.

[albertxing]

@busykai That's a good idea, but I'm not sure if we would want to reveal the cursor when the editor resizes or even when the panel is resized, only when the panel is shown.

[busykai]

@albertxing, that's fair, but i guess you could distinguish the types of resize events. it is much preferable to handle editor resize in a single place. there's a second param to the _onEditorAreaResize -- refreshFlag which is currently a string (it does not seem that its skip value being used at all). you use it to pass an scrollToCursor on panelExpanded. perhaps it would also be nice to re-organize refreshFlag to be a map so that multiple options could be passed.

[albertxing]

@busykai I see, do you think it would be alright if I added another REFRESH flag REFRESH_REVEAL_CURSOR?

[busykai]

@albertxing, currently that would be OK since we don't need to pass both refresh and scroll hints at the same time. a nicer solution would be pass an object as a refreshFlag, e.g. {refresh: REFRESH_FORCE, scrollToCursor: true}. note that to accomplish you only need to change to change triggerEditorResize and couple of its callers (in Panel manager) and editorAreaResize event processing (in Editor Manager).

[albertxing]

@busykai I just realized that we need to know the height of the expanded panel in order to determine whether or not to reveal the cursor. If we do this in EditorManager, we'll need to pass that information as an argument (which would be messy). Any ideas?

[busykai]

The function already accept the new editor's height, which is current editor height - panels height in this case, and you have to pass the new value when you trigger the event (see triggerEditorResize). Given two heights (current and new), it's the same algorithm you have currently to decide whether the cursor out of the view port or not. Do I miss something?

[albertxing]

I might be doing something wrong, but getting the current height and subtracting the new height gives me 0.

I suspect this is because PanelManager:101 sets the editor's height before it triggers the event.

[busykai]

no, i think you're right. this is how it was meant to function like this. i believe the cleanest way to fix this is to pass two heights (old and new) and hints as hash to the event (and to the handler).

[albertxing]

I can't find a clean place to fetch the old height of the editor - using calcEditorHeight() or $editorHolder.height() in the panelExpanded listener doesn't work as it gives the same number as the new height retrieved from triggerEditorResize...

Instead I decided to pass the height of the panel as a hint in refreshHint, and in EditorManager added the restriction that panelHeight in the hints must be defined in order to scroll to cursor.

I'll commit the changes, please let me know what you think.

[busykai]

Thanks! I will review it. Meanwhile, please merge with the latest master.

[busykai]

@albertxing, thanks for fixing the merge. I've taken a look around and spotted this method: Editor.centerOnCursor(). Using it eliminates the need to calculate cursor visibility in EditorManager as well as pass any additional info (heights and such) to the event handler. Sorry for not getting to you earlier, but the scheme we've been ending up seemed too heavy for the task. Using this method directly would cause the editor to scroll when the cursor is actually visible, but too close to the panel. Let me know if you have time to implement a method which would 1) scroll the view preserving the distance to the bottom and 2) if the distance from the bottom is higher than the new center, would center it (just like the centerOnCursor).

[albertxing]

@busykai, I agree that it is quite heavy. Optimally it would be just a few lines of code - but then we'd have more code in other files (which might not be a bad thing).

I tried the approach you mentioned by preserving the distance between the cursor and the bottom of the editor:

/**
 * Scrolls the editor viewport to maintain the distance between the cursor
 * and the bottom of the editor. Use only for panel expansion.
 */
Editor.prototype.pushUpCursor = function () {
    var $scrollerElement = $(this.getScrollerElement()),
        editorHeight = $scrollerElement.height(),
        statusBarHeight = $scrollerElement.outerHeight() - editorHeight,
        menuBarHeight = $scrollerElement.offset().top,
        bottom = window.innerHeight - menuBarHeight - statusBarHeight - editorHeight + 4;

    this.setScrollPos(null, this.getScrollPos().y + bottom);

    if (this._codeMirror.cursorCoords(null, "page").bottom < editorHeight / 2) {
        this.centerOnCursor();
    }
};

It works well when the cursor would be hidden by the panel, but if would not be hidden then the sudden movement might be confusing - especially if we also center the cursor.

I'm not sure if this is what you're looking for, let me know if there's anything that should change.

Also ote that I'm passing a scrollToCursor hint to the handler, though the panel height field has been removed. This is necessary so we don't push the cursor up when resizing the editor.

[busykai]

@albertxing, you have an extra character which breaks everything.

Related to the implementation: 1. you can calculate if the position of the cursor is currently visible and not do anything if it is visible. 2. currently you're scrolling twice. you can actually calculate where the cursor would be before scrolling to it (once).

[albertxing]

Thanks, @busykai. pushUpCursor (is that even a good name? Can you think of a better one?) now acts independently of centerOnCursor, computing the midpoint if maintaining distance to bottom will push the cursor past the middle.

I've also added the test for cursor visibility so it only scrolls when the cursor is hidden.

[sbruchmann]

@albertxing What about renaming pushUpCursor to scrollCursorIntoView?

[busykai]

@albertxing, thanks! now that it seems to be ok conceptually, i'll review the code over the weekend. and will think about the name, too. :)

[busykai]

Something like:

Determines if the cursor is currently visible and if not, scrolls the editor view port to make it visible. The distance between the cursor and the bottom is maintained, unless the cursor scrolls up higher than the editor center in which case line with the cursor will be centered.

[albertxing]

I just realized that maintaining distance to bottom is not strictly compatible with revealing the cursor, as if the cursor were initially hidden (before panel expand)then pushing the cursor up by the panel height will not reveal the cursor.

We'll have to decide whether we'd want to reveal the cursor in such a case by scrolling the minimal amount needed to show the cursor.

[busykai]

Of course, we should never push it higher than the center of the resized editor (no matter what the distance to the bottom was).

Edit: didn't your concern right. if the cursor was not visible then we should probably do the same thing as it will scroll the text that was visible. i believe we don't need to actually reveal cursor in such cases.

[albertxing]

Sure, that makes sense. Would revealCursor be a suitable name if we were to do this?

[busykai]

It actually can be used for other purposes as well, so Use only for panel expansion should be removed.

[busykai]

@albertxing, done with the first pass over the code. i'll also test some other cases (e.g. find in panel opened) and let you know.

[albertxing]

@busykai Thanks, I've looked over the comments and replied to some. I'll commit my changes (addressing everything except for the name change).

[peterflynn]

Seems like this comment could be more precise -- what does "maintaining the distance between the cursor and the bottom" mean given that the cursor is below the bottom at the time? Clearly we don't want to maintain that distance... It sounds like what we actually mean is something like "restoring the distance between the cursor and the bottom that existed before any bottom panels were shown"?

[peterflynn]

I also wonder if trying to preserve the cursor's offset from the bottom edge is the right goal. It might be better to just scroll the minimum amount needed to keep the cursor in view (so it would be on the bottommost line visible, just above the panel) -- this minimizes the amount that the editor jumps, and minimizes how far away from its original onscreen location the cursor winds up. That actually seems better for the user (and might be simple to implement, too?).

[peterflynn]

@albertxing @busykai Added some review comments

[albertxing]

@peterflynn @busykai I've switched to the delta call from setSize which Peter suggested. It seems cleaner and generally applicable to any resizing operation, which I guess is what we want.

I also managed to get rid of the stray 3's and 4's somehow.

Please let me know what you think.