Scroll to cursor if hidden by JSLint panel

[albertxing]

Scroll to show cursor above panel if it would have been hidden. Reference error #3003.

[TomMalbran]

If 180 is the height of the panel, you should know that the panels are resizeables, so the height can be different from time to time. You might want to the the height of the panel with jQuery.

[TomMalbran]

Please use double quotes here, since that is part of the coding convention, and same with $('#jslint-results') below.

[albertxing]

Sure, wasn't aware of that. If there is a specific style guide for Brackets, where can I find it? (I've already figured out the 4-space indentation part)

[TomMalbran]

Here it is: https://github.com/adobe/brackets/wiki/Brackets-Coding-Conventions

[albertxing]

Thanks!

[TomMalbran]

Now that the code is all together, you can move the scroll into view to line 212, change line 196 to if (editor && _gotoEnabled) and remove mustShow.

[albertxing]

Wow, you're like a human minifier.

[TomMalbran]

Great, this looks a lot better now, thanks for all the fast fixes :)

[albertxing]

@TomMalbran Thanks for all the good suggestions.

If we were to generalize this for all bottom panels, which file would it be implemented to? The panels are just HTML elements that are created from a template.

I was thinking (since all panels have class .bottom-panel) to either have a jQuery event listener attached to $('.bottom-panel').show() or to have one attached to Editor.resize(). Any ideas?

[albertxing]

@TomMalbran I've tried adding the code to the EditorManager.js file in the resizeEditor() function, and it seems to work for all panels:

https://gist.github.com/albertxing/8ef55ee58a72748f338a

Do you think this is good enough to go into a commit?

[peterflynn]

@albertxing @TomMalbran I'm concerned that the current code is JSLint-specific. We already have one other bottom panel in core (Find in Files results), and a number of extensions add bottom panels as well. So we need a generalized approach that all panels can share.

Some combination of EditorManager & Resizer seems like the best place to do this -- panel clients should probably call into Resizer so we know when panels are shown/hidden, and perhaps EditorManager should own the actual scroll-into-view logic. We'd need to change JSLint & Find in Files to use Resizer's hide/show APIs instead of the generic jQuery calls, but we probably need to do that migration soon anyway (see my comment in #2015). Extensions that haven't made this switch yet shouldn't break, but they wouldn't automatically get the benefit of this fix until they do migrate.

Note also that there are other situations where changes can make the cursor no longer visible. For example, window resize, panel resize, toggling word wrap on/off, closing one or more inline editors via Esc... we should think about which of those cases should have similar "auto scroll to visible" behavior.

[peterflynn]

One more thought: we should add a small number of unit test to cover this too... perhaps in EditorManager-test.

[albertxing]

@peterflynn I've tested out an general implementation through EditorManager, and it seems to work for show/hide as well as panel/window resize. See https://gist.github.com/albertxing/8ef55ee58a72748f338a

The only shortfall (if it even is one) is that resizing the windows without any panels open does not trigger the scrolling (it lets the cursor go out of view).

I'm waiting for some code review before I make it a commit (it does change a pretty important part of Brackets).
I haven't had any experience coding unit tests, so I'll probably need some help with that.

[TomMalbran]

@peterflynn I've been thinking in the reviews about making it less JSLint specific, the only thing that is specific is the $("#jslint-results") (which I realized that it should use $lintResults instead). So it can easily be moved to a new function just by passing the jQuery element, and it should work for any panel. But I wasn't sure if this was needed now or later.

I was thinking of moving it to Resizer.showPanel($element)/hidePanel($element). Or as you said move it to EditorManager.autoScrollToView() or similar, but I don't think that it should be added to EditorManager.resizeEditor(). Actually resize editor could be moved to show/hide panel before calling the auto scroll function. Show panel should also have a boolean parameter to let it know if it needs to auto scroll or not. I think that for some panels it shouldn't need to auto scroll into view, or is not even needed. JSLint is a special case, since the panel can open on save, and while you are editing, and not like Find in Files that opens after a specific call to it, and while you are actually trying to search something and not keep editing in most cases.

I don't think that most of the other situations (window resize, panel resize, toggling word wrap on/off) need this behavior since you are moving away from the coding to do this actions with the mouse. But we can leave that as a possibility if it gets requested. It would be worth adding it for closing inline editors since in this case you are using ESC to close the editor while coding, but I need to think how this 2 cases be refactored to work with the same code. Right now, it seems easier to add specific code for the editors problem (check if the cursor position is greater than 36 and less than the editor height before closing and check if the cursor position is less than 36 after).

[albertxing]

@peterflynn @TomMalbran Moving it to Resizer.show(element) does work when we replace $lintResults.show() with Resizer.show($lintResults), so doing that is not a problem.

I just wanted to note that adding it to Resizer.hide() doesn't help too much, since hiding a panel wouldn't interfere with the cursor position.

Putting the code in Resizer, however, does not trigger autoscrolling when the window is resized with the cursor still focused in the editor, whereas in EditorResize it does - so that's another difference to think about. I agree with @TomMalbran that this is not critical, and some may find it unnecessary/annoying.

I don't see how this links too much to inline editors, since their behaviour doesn't in most scenarios push the cursor off the screen.

[TomMalbran]

Right, it isn't needed in Resizer.hide().

If it would be necessary for the window resize, I wouldn't put it inside the EditorManager.resizeEditor, I would add it in a new function in EditorManager and call it on a window/editor resize. The same function would be called by Resize.show().

It does push the cursor off the screen. Open an inline editor and scroll the editor, so that it is out of view in the top part. Place the cursor on the first visible line. Press ESC. The inline editor will disappear and the cursor got out of view.

[albertxing]

@TomMalbran @peterflynn I've generalized into Resizer for now, tell me if you want it in EditorManager as a separate function.
Also, any ideas for function names?

Side note: @TomMalbran did you see what just happened there? There was a merge commit again, and I didn't even merge. This is ridiculously frustrating. 😖

[TomMalbran]

As I mentioned in your gist, I don't think this is the best way to do it, since if you have for example the find in files panel open and the scroll behind it, so for the user is not in the view, and then the JSLint panel pops up, and that cursor not in view ends up in the view.

[albertxing]

Oh, didn't think of that (also didn't get notified for the gist).

[TomMalbran]

Replying to the side note: I haven't seen any merge commits, but if there is a Merge remote-tracking branch 'upstream/master'... that is ok, it just because you merged the new commits on master on this branch. Sometimes those are needed because of merge issues or testing.

[albertxing]

I've changed it to only use the single element (that's being called) height instead of the total height of all the panels.

@TomMalbran I've grown to become afraid of them because they often mess up my own commits, mention committers who aren't involved as well as other issues that shouldn't be referenced (see my previous encounter in num#3248)
See the second commit? Is that okay?

[TomMalbran]

@albertxing Yes that is ok. You don't really need to update your branch too much, it should only be needed when there are merge issue, or if there is something new that could alter the functionality of your code and you need to test, change your code. This is what makes branches great, you can have a branch stand for several days and still be mergeable, even after many commits where done to brackets.

[albertxing]

Still learning... 📖

Any ideas about a standalone function? Or is that something @peterflynn has to confirm? It seems pretty big to me.

[TomMalbran]

I'll wait on @peterflynn opinions about this.

[peterflynn]

@albertxing Sorry for the slow reply. I've pushed up a branch with a proposal for cleaner APIs around panel management: pflynn/panel-management. It should provide a nicer place to attach this functionality: I think you can just put some code in PanelManager.listenToResize() to attach a "panelExpanded" listener, and in that listener do what you're currently doing in Resizer.show().

This branch isn't entirely done yet, but if you'd like to try porting your fix over to use this code it'd be great to get your feedback.

[peterflynn]

Oh, and by "not entirely done yet" I just mean there's some stuff I'm going to add to fix parts of #2015 before this is merged. But it should already be working just as well as the current Brackets master (let me know if you see any bugs!).

[albertxing]

@peterflynn Did you mean change the code here to use the changes implemented through PanelManager? I probably misunderstood.

[peterflynn]

@albertxing We just spotted this while reviewing old PRs. Sorry this fell through the cracks! It looks like PR #3931 is actually the more up-to-date patch, but it was auto-closed by GitHub when the branch is was based off of landed (in PR #3943)... so we lost track of it. (I hate that GitHub "feature," for the record :-P).

Are you still interested in this? I think the next step would just be to reopen a new version of PR #3931 targeted at master. It looks like a good overall approach to me.

Sorry again about the mixup & long delay...

[peterflynn]

Also, I'm going to close this PR since it's the out of date one. I'll lave a note in bug #3003 pointing to the other PR as the most up to date implementation of a fix.

[albertxing]

Wow, I completely forgot about this already.
I've been busy and haven't had much time to contribute, but I'll definitely be willing to finish what I started.

I'll make a new pull request for issue #3003 soon.