Skip to content
This repository has been archived by the owner on Mar 3, 2023. It is now read-only.

Improve positioning when opening file at line #19272

Merged
merged 3 commits into from
May 7, 2019
Merged

Improve positioning when opening file at line #19272

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9921d1c
Unfold (if needed) when opening file at specific line
jasonrudolph May 6, 2019
0c51771
Scroll line to center when opening file at specific line
jasonrudolph May 6, 2019
4ac6107
:art: Use `await` to make the world a better place
jasonrudolph May 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions spec/workspace-spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -887,6 +887,20 @@ describe('Workspace', () => {
).toEqual([2, 11])
)
})

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

await workspace.open('../sample-with-many-folds.js')
editor = workspace.getActiveTextEditor()
editor.foldBufferRow(2)
expect(editor.isFoldedAtBufferRow(2)).toBe(true)
expect(editor.isFoldedAtBufferRow(3)).toBe(true)

await workspace.open('../sample-with-many-folds.js', { initialLine: 2 })
expect(editor.isFoldedAtBufferRow(2)).toBe(false)
expect(editor.isFoldedAtBufferRow(3)).toBe(false)
})
})

describe('when the file size is over the limit defined in `core.warnOnLargeFileLimit`', () => {
Expand Down
6 changes: 6 additions & 0 deletions src/workspace.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1065,6 +1065,12 @@ module.exports = class Workspace extends Model {
if (typeof item.setCursorBufferPosition === 'function') {
item.setCursorBufferPosition([initialLine, initialColumn])
}
if (typeof item.unfoldBufferRow === 'function') {
item.unfoldBufferRow(initialLine)
}
if (typeof item.scrollToBufferPosition === 'function') {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

item.scrollToBufferPosition([initialLine, initialColumn], { center: true })
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}
}

const index = pane.getActiveItemIndex()
Expand Down