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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ignore-folded-lines-when-setting-line-numbers #1

Merged
merged 1 commit into from Feb 17, 2016

Conversation

Projects
None yet
3 participants
@tonatiuh
Copy link

tonatiuh commented Nov 16, 2015

This is the issue this PR fixes:

  1. Given the following file is open:

unfolded file

  1. When I fold the lines:
  def talk
    puts 'hello world!'
  end
  1. Then I expect the line with the content def walk to be numbered as line 3 - this is the way VIM does it - as in:

folded - correctly numbered

  1. However, that wasn't happening given the folded lines were being taken into account, as in:

folded - incorrectly numbered

Extra:

I wanted to subscribe the package to the fold/unfold events, however the only way I found (here https://discuss.atom.io/t/event-to-listen-for-code-folding-unfolding/17827/5):

    atom.commands.add 'atom-text-editor',
      'editor:fold-all': => @_update()
      'editor:unfold-all': => @_update()
      'editor:fold-current-row': => @_update()
      'editor:unfold-current-row': => @_update()
      'editor:fold-selection': => @_update()
      'editor:fold-at-indent-level-1': => @_update()
      'editor:fold-at-indent-level-2': => @_update()
      'editor:fold-at-indent-level-3': => @_update()
      'editor:fold-at-indent-level-4': => @_update()
      'editor:fold-at-indent-level-5': => @_update()
      'editor:fold-at-indent-level-6': => @_update()
      'editor:fold-at-indent-level-7': => @_update()
      'editor:fold-at-indent-level-8': => @_update()
      'editor:fold-at-indent-level-9': => @_update()

didn't work, given the notification is sent prior the fold finishes. It seem there is no "onDid" event yet for the folds.

Last, @justmoon I found this repo doesn't have the issues enabled, when I click on submit an issue to this package (from Atom), I'm taken to the home page of the repo, is there any special reason why they were disabled? If not, I think they should be enabled. WDYT?

@justmoon

This comment has been minimized.

Copy link
Owner

justmoon commented Nov 17, 2015

Thanks for the PR! I enabled the issues, that was just an oversight on my part.

Regarding the change: Not opposed, just trying to understand. It seems like vim-mode does count folded lines for commands like d9d. As such it seems to make more sense to show the line numbers as counted?

Please excuse my ignorance, I'm a relative vim newbie and don't use folding.

@tonatiuh

This comment has been minimized.

Copy link

tonatiuh commented Nov 17, 2015

No problem at all. I did a tests with the combination you shared (d9d), and from what I saw vim-mode doesn't take into account the folded lines. Let me elaborate, given the following scenario (where I'm at the absolute line 1):

in-absolute-line-1

when I press d2d (or d2j), then the current line ("Class Human") and the whole method "talk" are deleted. The same applies if the method is 6 lines long, as long as it's folded, d2d will delete it.

Did I follow your question?

@justmoon

This comment has been minimized.

Copy link
Owner

justmoon commented Dec 24, 2015

@tonatiuh Finally tested this in vim and you're right, it works like you said. Don't know why I got the opposite impression the first time around. I'm happy to rebase your pull, but not sure when I'll have the time. If you'd like to rebase it, I'm ready to merge it.

@selevt

This comment has been minimized.

Copy link

selevt commented Jan 7, 2016

Thanks @tonatiuh for providing the fix. Is there an update on the state of the rebase? Otherwise I'd be willing to provide a new PR.

@tonatiuh

This comment has been minimized.

Copy link

tonatiuh commented Jan 7, 2016

Hey guys, sorry for the delay. I'll try to apply the rebase this week so that this can be merged.

@tonatiuh

This comment has been minimized.

Copy link

tonatiuh commented Jan 11, 2016

Guys, I wasn't able to finish this yet. Given the base code changed considerably, I need to debug more of what I expected. I hope I have news for you within the next three days.

@tonatiuh tonatiuh force-pushed the tonatiuh:ignore-folded-lines-when-setting-line-numbers branch from 4b73604 to ae6313e Jan 24, 2016

@tonatiuh

This comment has been minimized.

Copy link

tonatiuh commented Jan 24, 2016

@justmoon, I've added the functionality on top of the latest code in master, I needed to change some of the code, actually.

Based on my manual tests, the package is behaving correctly, imitating vim's "relativenumber" setting.

cc @selevt.

@tonatiuh

This comment has been minimized.

Copy link

tonatiuh commented Jan 27, 2016

Any news on this?

@tonatiuh

This comment has been minimized.

Copy link

tonatiuh commented Feb 13, 2016

@justmoon

This comment has been minimized.

Copy link
Owner

justmoon commented Feb 17, 2016

LGTM

Sorry for the delayed review!

justmoon added a commit that referenced this pull request Feb 17, 2016

Merge pull request #1 from tonatiuh/ignore-folded-lines-when-setting-…
…line-numbers

ignore-folded-lines-when-setting-line-numbers

@justmoon justmoon merged commit ae37a46 into justmoon:master Feb 17, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment