Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve positioning when opening file at line #19272

Merged
merged 3 commits into from May 7, 2019

Conversation

Projects
None yet
3 participants
@jasonrudolph
Copy link
Member

commented May 6, 2019

Issue or RFC Endorsed by Atom's Maintainers

#12253

Description of the Change

Prior to this change, when you opened a file at a specific line, Atom would scroll to the given line number, but only enough so that the line is visible at the bottom of the editor. #12253 describes the problem with this behavior as follows:

plugins like JSHint that are displayed at the bottom will cover the desired line number. Therefore, the user still has to scroll the content to view the line number, defeating the entire purpose of this feature.

With the changes in this pull request, when opening a file at a specific line, Atom will:

  • Attempt to scroll the editor as needed so that the specified line is centered vertically
  • If the file is already open and the requested line is hidden by a fold, unfold so that the line is visible

Alternate Designs

In response to the problem that "plugins like JSHint that are displayed at the bottom will cover the desired line number," #12253 suggests scrolling the requested line to the top of the editor.

We could scroll the line to the top of the editor. However, centering the line vertically also resolves the stated problem. In addition to that, it has a couple advantages over scrolling the line to the top of the editor:

  • It's often useful to see a few lines of context above the current line. (For decades, diff tools have made us accustomed to seeing a few lines of context above and below a given line. 馃槆馃懘) If we scroll the line to the top of the editor, we lose the context of the preceding lines.
  • Centering the line restores Atom's originally-intended behavior, as it existed prior to the regression described in #12253 (comment).

Possible Drawbacks

None that I'm aware of.

Verification Process

The following steps use a file named package.json as the test file. At the time of this writing, package.json contains 285 lines.

  • Open the file at a specific line and verify that Atom scrolls to that line
    • atom package.json:100 - Places cursor on line 100 and centers line 100 vertically in the editor
    • atom package.json:285
      • Places cursor on line 285 and centers line 285 vertically in the editor when editor.scrollPastEnd is set to true
      • Places cursor on line 285 and renders line 285 at bottom of the editor when editor.scrollPastEnd is set to false
    • atom package.json:10000
      • Places cursor on line 285 and centers line 285 vertically in the editor when editor.scrollPastEnd is set to true
      • Places cursor on line 285 and renders line 285 at bottom of the editor when editor.scrollPastEnd is set to false
    • atom package.json:1 - Places cursor on line 1 and renders line 1 at top of editor
  • When package.json is already open in Atom with the cursor on line 1 and the viewport scrolled so that you can only see lines 250-285, running atom package.json:100 places cursor on line 100 and centers line 100 vertically in the editor
  • When package.json is already open in Atom and line 100 is hidden by a fold, running atom package.json:100 places cursor on line 100, unfolds the fold, and centers line 100 vertically in the editor
  • Open file at specific line via URI handler, and verify that Atom scrolls to that line and centers it vertically in the editor (atom://core/open/file?filename=%2FUsers%2Fme%2Fgithub%2Fatom%2Fpackage.json&line=100)

Release Notes

When opening a file at a specific line number, if Atom needs to scroll down in order to show the requested line, Atom will attempt to center the line vertically in the editor so that you can quickly find the line and see its surrounding context.

Before and After

Before

before

After

after

jasonrudolph added some commits May 6, 2019

if (typeof item.unfoldBufferRow === 'function') {
item.unfoldBufferRow(initialLine)
}
if (typeof item.scrollToBufferPosition === 'function') {

This comment has been minimized.

Copy link
@jasonrudolph

jasonrudolph May 6, 2019

Author Member

This conditional and the one above are inspired by this pre-existing conditional:

atom/src/workspace.js

Lines 1065 to 1067 in 0c51771

if (typeof item.setCursorBufferPosition === 'function') {
item.setCursorBufferPosition([initialLine, initialColumn])
}

That conditional 鈽濓笍 was added in 11112cb to support the scenario "where a custom editor is opened that isn't a text editor" so that "it won't blow up if the custom editor doesn't implement setCursorBufferPosition."

That same motivation seems relevant to the calls to unfoldBufferRow and scrollToBufferPosition introduced in this pull request, so I've followed the same pattern.

@jasonrudolph jasonrudolph marked this pull request as ready for review May 6, 2019

item.unfoldBufferRow(initialLine)
}
if (typeof item.scrollToBufferPosition === 'function') {
item.scrollToBufferPosition([initialLine, initialColumn], { center: true })

This comment has been minimized.

Copy link
@jasonrudolph

jasonrudolph May 6, 2019

Author Member

I've included a test to verify the unfolding behavior, but I have not included a test for scrolling the line to the center of the editor.

After looking into what it would take to add a test for scrolling the line to the center of the editor, it seems like it the effort required to add and maintain that test probably isn't worth the rather minor benefit that we'd get from such a test. I think we'd have to set the window to a certain size and then check the coordinates of the line in the viewport. I think it's do-able, but the benefit doesn't feel like it's worth the effort. If anyone feels differently and has thoughts on how to best test this change, please let me know.

This comment has been minimized.

Copy link
@rafeca

rafeca May 7, 2019

Contributor

I agree, specially since we already have a test in the text editor that actually checks that when passing the center property the editor gets vertically centered.

@jasonrudolph

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

@nathansobo @rafeca: Would either of you be up for offering a code review for these changes?

@rafeca

rafeca approved these changes May 7, 2019

Copy link
Contributor

left a comment

Looks good to me!

I agree about centering the line on the editor (in fact the fuzzy finder does not center it when specifying a line number and I found this annoying, I'm gonna send a PR there to fix it 馃槃 ).

item.unfoldBufferRow(initialLine)
}
if (typeof item.scrollToBufferPosition === 'function') {
item.scrollToBufferPosition([initialLine, initialColumn], { center: true })

This comment has been minimized.

Copy link
@rafeca

rafeca May 7, 2019

Contributor

I agree, specially since we already have a test in the text editor that actually checks that when passing the center property the editor gets vertically centered.

it('unfolds the fold containing the line', () => {
let editor

waitsForPromise(() =>

This comment has been minimized.

Copy link
@rafeca

rafeca May 7, 2019

Contributor

A small improvement: you can make the spec function async to be able to use await on the workspace.open() function calls and then get rid of all the runs() and waitsForPromise() calls.

This comment has been minimized.

Copy link
@jasonrudolph

jasonrudolph May 7, 2019

Author Member

@rafeca: Thanks for that suggestion. 4ac6107 updates the test to adopt this approach.

So. Much. Better. 馃樆

@nathansobo

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

Looks like @rafeca left a decently thorough review, so I'll skip it unless you really really want my eyes on this.

@jasonrudolph jasonrudolph merged commit f1952c2 into master May 7, 2019

2 checks passed

Atom Pull Requests #20190507.10 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@jasonrudolph jasonrudolph deleted the improve-positioning-when-opening-file-at-line branch May 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.