atom / symbols-view

Jump to symbols in Atom
MIT License
163 stars 114 forks source link

Add support for quickly jumping to symbol positions for FileView #73

Closed brumm closed 9 years ago

brumm commented 9 years ago

symbols-view

brumm commented 9 years ago

Seeing that the other PR is from last November and still open - is participation via PRs welcomed here, or not?

kevinsawicki commented 9 years ago

Seeing that the other PR is from last November and still open - is participation via PRs welcomed here, or not?

Yes, was planning to look at this soon.

brumm commented 9 years ago

Awesome, thanks for getting back to me :)

kevinsawicki commented 9 years ago

Couple things I noticed after using this for a bit:

The editor scroll position is not restored, only the cursor position is, makes this case feel odd: canceling the list scrolls to the cursor but does not restore my original editor scroll position. In the gif below, I'm opening the view and then hitting escape, and my window scroll position changes.

scroll-cursor-not-visible

The column position on the line isn't restored. If I open the window and then hit escape, I lose my column position on the line.

column-position

If I have multiple cursors, open the view, then hit escape, I lose all but one of my cursors.

multi-cursor

brumm commented 9 years ago

Nice catch, thanks for having a look!

I was looking through atom core and was surprised thats there's no apparent way of serializing cursor/selection state of a TextEditor. Is this correct? I'd rather use the 'official' way of doing this.

kevinsawicki commented 9 years ago

I was looking through atom core and was surprised thats there's no apparent way of serializing cursor/selection state of a TextEditor.

You mean like storing temporarily while the list is showing? I think usually we call TextEditor::getSelectedBufferRanges before doing something, and then restore them using TextEditor::setSelectedBufferRanges.

izuzak commented 9 years ago

Hey @brumm -- sorry for not getting back to you on this! I just noticed that you pushed a new commit, but we don't get notified when that happens if you also don't add a comment.

It seems that this PR is no longer mergeable due to merge conflicts. Could you update it so that it is mergeable again and ping us when you do so that we can take another look? Thanks!

brumm commented 9 years ago

Hey Ivan, thanks for getting back to me! I'll update the code and also add another test for serializeEditorState and deserializeEditorState

brumm commented 9 years ago

Well, there's failing tests that I have no idea how to fix and after three months, I'm very much out of the loop with what happened on master. I'll be closing this and re-implementing the feature on a fresh master.

brumm commented 9 years ago

this lives on in #99