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

Toggle context lines #908

Merged
merged 13 commits into from Jun 30, 2017

Conversation

Projects
None yet
5 participants
@dirk-thomas
Contributor

dirk-thomas commented Jun 3, 2017

Description of the Change

The package has config settings for the number of context lines before and above each matched line. The default values are zero in order to not change previous behavior (and show no context by default). When any of these values is set to non-zero the view always shows the context lines.

This patch adds a button to the results pane to toggle between showing and hiding the context lines. If the config values for the context lines are both zero the button is not shown since it wouldn't do anything.

Benefits

The user can toggle the context lines without the need to change the package settings and redo the query.

Possible Drawbacks

Currently there is no option to hide the context lines by default (when non-zero values are in the config). Maybe the latest state can be made persistent to use it for the next search result?

Applicable Issues

  • The scroll position doesn't stay around the same matches when the context lines are being toggled.

Future changes

As a consequence I think it would make sense to change the default values of the config from zero to e.g. three. If the context lines are hidden by default (and after being toggled remember the last state) that would maintain the previous behavior but make the context available to all users by default without requiring custom settings.

@50Wliu 50Wliu added the needs-review label Jun 3, 2017

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Jun 5, 2017

Contributor

Should the state for showing / hiding the context lines be added to FindOptions which would make the setting persistent?

Contributor

dirk-thomas commented Jun 5, 2017

Should the state for showing / hiding the context lines be added to FindOptions which would make the setting persistent?

@iolsen

This comment has been minimized.

Show comment
Hide comment
@iolsen

iolsen Jun 12, 2017

@maxbrunsfeld do you have some time to review this?

iolsen commented Jun 12, 2017

@maxbrunsfeld do you have some time to review this?

@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Jun 14, 2017

Contributor

@dirk-thomas Can you post a GIF of the interaction you've got working so far?

@simurai What direction would you like to see for this feature?

I could actually imagine a UI element that raises and lowers the number of context lines, like little + and - buttons with the word context between them.

Contributor

maxbrunsfeld commented Jun 14, 2017

@dirk-thomas Can you post a GIF of the interaction you've got working so far?

@simurai What direction would you like to see for this feature?

I could actually imagine a UI element that raises and lowers the number of context lines, like little + and - buttons with the word context between them.

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Jun 15, 2017

Contributor

Please see the following gif which uses the config settings of 3 lines of context before and after. Sorry for the bad resolution - I couldn't make the screen recorder work with higher resolutions:
find-and-replace_toggle-context-lines

I would be interested to make the following changes (in that order) if there is interest in that and pull request will be reviewed in a reasonable time frame:

  • update this PR:
    • to persist the toggle state, e.g. in the find options (see my pending question)
    • maintain a reasonable scrolling position when toggling (whatever that means)
  • separate PRs:
    • change the default setting to N context lines (e.g. 3) so that users can toggle the context lines without requiring custom settings
    • render leading whitespace correctly (if necessary add an option to enable/disable this behavior)
    • deduplicate lines in the search results (duplicate context lines as well as multiple matches per line)

I could actually imagine a UI element that raises and lowers the number of context lines, like little + and - buttons with the word context between them.

That would be an interesting feature to. The + command will currently be constraint by the number of context lines configure in the settings (since only that many context lines are being fetched). Otherwise when that threshold is exceeded the search query would need to be repeated with different arguments.

Contributor

dirk-thomas commented Jun 15, 2017

Please see the following gif which uses the config settings of 3 lines of context before and after. Sorry for the bad resolution - I couldn't make the screen recorder work with higher resolutions:
find-and-replace_toggle-context-lines

I would be interested to make the following changes (in that order) if there is interest in that and pull request will be reviewed in a reasonable time frame:

  • update this PR:
    • to persist the toggle state, e.g. in the find options (see my pending question)
    • maintain a reasonable scrolling position when toggling (whatever that means)
  • separate PRs:
    • change the default setting to N context lines (e.g. 3) so that users can toggle the context lines without requiring custom settings
    • render leading whitespace correctly (if necessary add an option to enable/disable this behavior)
    • deduplicate lines in the search results (duplicate context lines as well as multiple matches per line)

I could actually imagine a UI element that raises and lowers the number of context lines, like little + and - buttons with the word context between them.

That would be an interesting feature to. The + command will currently be constraint by the number of context lines configure in the settings (since only that many context lines are being fetched). Otherwise when that threshold is exceeded the search query would need to be repeated with different arguments.

@simurai

This comment has been minimized.

Show comment
Hide comment
@simurai

simurai Jun 15, 2017

Member

update this PR:

  • to persist the toggle state, e.g. in the find options (see my pending question)
  • maintain a reasonable scrolling position when toggling (whatever that means)

👍 I think that would be great.

Plus maybe move the button in its own btn-group so that it feels more like a separate toggle and not belonging to the collapse buttons. I can help out with that if you run into styling troubles.

screen shot 2017-06-15 at 5 05 58 pm

And change to a "Show Context" label. So it starts with a verb.


I could actually imagine a UI element that raises and lowers the number of context lines, like little + and - buttons with the word context between them.

Yeah, that would be similar to the Image View zoom. Although not sure if it's annoying having to click multiple times and you can't just expand straight to your prefered amount of lines. I think if you wanna seem more, you wanna see as much as possible (as set in the config).


Or another option: Expand context lines per match. Usually when I want to see more context it's not for every match, but just the one that I might be interested in. Maybe have an expand button and/or a keyboard shortcut like cmd-enter to toggle context only for that match.

screen shot 2017-06-15 at 5 04 44 pm

Anyways, that is outside of this PR's scope.

Member

simurai commented Jun 15, 2017

update this PR:

  • to persist the toggle state, e.g. in the find options (see my pending question)
  • maintain a reasonable scrolling position when toggling (whatever that means)

👍 I think that would be great.

Plus maybe move the button in its own btn-group so that it feels more like a separate toggle and not belonging to the collapse buttons. I can help out with that if you run into styling troubles.

screen shot 2017-06-15 at 5 05 58 pm

And change to a "Show Context" label. So it starts with a verb.


I could actually imagine a UI element that raises and lowers the number of context lines, like little + and - buttons with the word context between them.

Yeah, that would be similar to the Image View zoom. Although not sure if it's annoying having to click multiple times and you can't just expand straight to your prefered amount of lines. I think if you wanna seem more, you wanna see as much as possible (as set in the config).


Or another option: Expand context lines per match. Usually when I want to see more context it's not for every match, but just the one that I might be interested in. Maybe have an expand button and/or a keyboard shortcut like cmd-enter to toggle context only for that match.

screen shot 2017-06-15 at 5 04 44 pm

Anyways, that is outside of this PR's scope.

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Jun 16, 2017

Contributor

@simurai I moved the new button into a separate group, left of the existing buttons and updated the label (8ea4409). I had to use a wrapping btn-toolbar to visually separate the two groups. Was that how you imagined it?

find-and-replace_update-button

The per-match buttons might offer more flexibility but I would expect that it would still require a button to toggle the context lines for all matches for convenience. Having both might also make the UI more crowded than necessary. So I kept it with a single "global" button for now.

Contributor

dirk-thomas commented Jun 16, 2017

@simurai I moved the new button into a separate group, left of the existing buttons and updated the label (8ea4409). I had to use a wrapping btn-toolbar to visually separate the two groups. Was that how you imagined it?

find-and-replace_update-button

The per-match buttons might offer more flexibility but I would expect that it would still require a button to toggle the context lines for all matches for convenience. Having both might also make the UI more crowded than necessary. So I kept it with a single "global" button for now.

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Jun 16, 2017

Contributor

I still kind of need answers / comments regarding the two pending issues:

  • to persist the toggle state: Should I use the find options for this?
  • maintain a reasonable scrolling position when toggling: What exactly should than achieve? The first visible match should stay close to the top? Or any other criteria?
Contributor

dirk-thomas commented Jun 16, 2017

I still kind of need answers / comments regarding the two pending issues:

  • to persist the toggle state: Should I use the find options for this?
  • maintain a reasonable scrolling position when toggling: What exactly should than achieve? The first visible match should stay close to the top? Or any other criteria?
@simurai

This comment has been minimized.

Show comment
Hide comment
@simurai

simurai Jun 16, 2017

Member

maintain a reasonable scrolling position when toggling: What exactly should than achieve? The first visible match should stay close to the top? Or any other criteria?

Hmm.. This is probably super hard to get right. If you're scrolled down a bit and then click on "Show Context" and it jumps to the first match, you would have to scroll back to the position that you were before. Would some sort of "keep current scroll position" be possible? For example if the center of my current visible area is 66% of the total height, then when expanded also scroll to the center of 66% total expanded height.

Member

simurai commented Jun 16, 2017

maintain a reasonable scrolling position when toggling: What exactly should than achieve? The first visible match should stay close to the top? Or any other criteria?

Hmm.. This is probably super hard to get right. If you're scrolled down a bit and then click on "Show Context" and it jumps to the first match, you would have to scroll back to the position that you were before. Would some sort of "keep current scroll position" be possible? For example if the center of my current visible area is 66% of the total height, then when expanded also scroll to the center of 66% total expanded height.

@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Jun 16, 2017

Contributor

Storing the toggle state in the find options is fine.

Also, we probably shouldn't show this button if both searchContextLineCountBefore and searchContextLineCountAfter are set to zero, since it won't do anything.

Contributor

maxbrunsfeld commented Jun 16, 2017

Storing the toggle state in the find options is fine.

Also, we probably shouldn't show this button if both searchContextLineCountBefore and searchContextLineCountAfter are set to zero, since it won't do anything.

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Jun 16, 2017

Contributor

Storing the toggle state in the find options is fine.

I will try to update the patch with this soon.

Also, we probably shouldn't show this button if both searchContextLineCountBefore and searchContextLineCountAfter are set to zero, since it won't do anything.

This is already the case in the current patch.

Contributor

dirk-thomas commented Jun 16, 2017

Storing the toggle state in the find options is fine.

I will try to update the patch with this soon.

Also, we probably shouldn't show this button if both searchContextLineCountBefore and searchContextLineCountAfter are set to zero, since it won't do anything.

This is already the case in the current patch.

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Jun 17, 2017

Contributor

See edf279f for making the state to show context lines persistent via the FindOptions.

@simurai Regarding the idea of - and + buttons: for some use cases I can see that it would be nice to e.g. only show N lines of context after the match (but not any before). Also what label should be used for in-between the - and + buttons? It should probably indicate the number of currently shown lines as well as mention "Context" somehow. If we use two separate parts for before and after the label should also clarify which one it belongs to. If the user e.g. configures 0 as the max lines before but N > 0 as the maximum lines after only the later controls will be visible and should be clear enough (even when the controls for the previous lines are not around). Maybe create two new icons (for context lines before and after) would be best and the label would show those icons as well as the currently used number... So many ideas & options 😉 Any input on the UI part would be greatly appreciated - especially when considering custom icons I would probably need help from someone creating those...

Contributor

dirk-thomas commented Jun 17, 2017

See edf279f for making the state to show context lines persistent via the FindOptions.

@simurai Regarding the idea of - and + buttons: for some use cases I can see that it would be nice to e.g. only show N lines of context after the match (but not any before). Also what label should be used for in-between the - and + buttons? It should probably indicate the number of currently shown lines as well as mention "Context" somehow. If we use two separate parts for before and after the label should also clarify which one it belongs to. If the user e.g. configures 0 as the max lines before but N > 0 as the maximum lines after only the later controls will be visible and should be clear enough (even when the controls for the previous lines are not around). Maybe create two new icons (for context lines before and after) would be best and the label would show those icons as well as the currently used number... So many ideas & options 😉 Any input on the UI part would be greatly appreciated - especially when considering custom icons I would probably need help from someone creating those...

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Jun 17, 2017

Contributor

Regarding the scrolling behavior: the existing collapse / expand commands to hide/fold and show/unfold the matches per path avoid the same problem by simply always scrolling to the top when the commands are triggered. I would say this would be a "good enough" default behavior for the new show/hide context lines behavior too.

If we figure out a decent way how to behave in that case (which is reasonable to implement and doesn't lead to weird behavior in corner cases) it can be applied to all the options where the lengths of the search results view changes.

Does that sound like a viable option? I have implemented this behavior in 41faed0 for now.

Contributor

dirk-thomas commented Jun 17, 2017

Regarding the scrolling behavior: the existing collapse / expand commands to hide/fold and show/unfold the matches per path avoid the same problem by simply always scrolling to the top when the commands are triggered. I would say this would be a "good enough" default behavior for the new show/hide context lines behavior too.

If we figure out a decent way how to behave in that case (which is reasonable to implement and doesn't lead to weird behavior in corner cases) it can be applied to all the options where the lengths of the search results view changes.

Does that sound like a viable option? I have implemented this behavior in 41faed0 for now.

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Jun 19, 2017

Contributor

I also implemented the separate + and - buttons for leading as well as trailing context lines. Please the the following animation for how it looks and works (the flickering in the beginning is only visible in the screen recording 😉):

find-and-replace_dec-inc-sep-context-lines

I would replace the words "before" and "after" with an icon. Maybe even the line count could be removed from that button since it should be obvious from the results visible below.

What do you think?

Contributor

dirk-thomas commented Jun 19, 2017

I also implemented the separate + and - buttons for leading as well as trailing context lines. Please the the following animation for how it looks and works (the flickering in the beginning is only visible in the screen recording 😉):

find-and-replace_dec-inc-sep-context-lines

I would replace the words "before" and "after" with an icon. Maybe even the line count could be removed from that button since it should be obvious from the results visible below.

What do you think?

@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Jun 19, 2017

Contributor

Wow, that looks really cool @dirk-thomas, and the code looks great so far!

the existing collapse / expand commands to hide/fold and show/unfold the matches per path avoid the same problem by simply always scrolling to the top when the commands are triggered. I would say this would be a "good enough" default behavior for the new show/hide context lines behavior too.

I think scrolling to the top would be a bit more jarring for these options, because if you're clicking these buttons, there's probably a particular search result that you're looking at and wanting to see context for. What do you think about at least scrolling such that the same file is visible?

I'm going to defer to @simurai on some of the other UI design questions.

Contributor

maxbrunsfeld commented Jun 19, 2017

Wow, that looks really cool @dirk-thomas, and the code looks great so far!

the existing collapse / expand commands to hide/fold and show/unfold the matches per path avoid the same problem by simply always scrolling to the top when the commands are triggered. I would say this would be a "good enough" default behavior for the new show/hide context lines behavior too.

I think scrolling to the top would be a bit more jarring for these options, because if you're clicking these buttons, there's probably a particular search result that you're looking at and wanting to see context for. What do you think about at least scrolling such that the same file is visible?

I'm going to defer to @simurai on some of the other UI design questions.

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Jun 19, 2017

Contributor

Please see the latest commit using custom made icons instead:

find-and-replace_context-icons

Contributor

dirk-thomas commented Jun 19, 2017

Please see the latest commit using custom made icons instead:

find-and-replace_context-icons

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Jun 19, 2017

Contributor

I think scrolling to the top would be a bit more jarring for these options, because if you're clicking these buttons, there's probably a particular search result that you're looking at and wanting to see context for. What do you think about at least scrolling such that the same file is visible?

Well, I think "the same file is visible" is already a bit too fuzzy. Multiple files might be in the viewport. Which one should be picked in that case? I think it will be pretty complicated to handle this (however the intended behavior should be). So maybe keep this for a separate patch?

Contributor

dirk-thomas commented Jun 19, 2017

I think scrolling to the top would be a bit more jarring for these options, because if you're clicking these buttons, there's probably a particular search result that you're looking at and wanting to see context for. What do you think about at least scrolling such that the same file is visible?

Well, I think "the same file is visible" is already a bit too fuzzy. Multiple files might be in the viewport. Which one should be picked in that case? I think it will be pretty complicated to handle this (however the intended behavior should be). So maybe keep this for a separate patch?

@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Jun 19, 2017

Contributor

Well, I think "the same file is visible" is already a bit too fuzzy. Multiple files might be in the viewport. Which one should be picked in that case?

I think it will be pretty complicated to handle this

Ok sorry, I thought maybe leaving it semi-open ended would be helpful. How about this: after pushing one of those buttons, we should scroll to reveal the selected match. It shouldn't be complicated; similar functionality is already used for most of the existing commands. See the scrollToSelectedMatch method.

Contributor

maxbrunsfeld commented Jun 19, 2017

Well, I think "the same file is visible" is already a bit too fuzzy. Multiple files might be in the viewport. Which one should be picked in that case?

I think it will be pretty complicated to handle this

Ok sorry, I thought maybe leaving it semi-open ended would be helpful. How about this: after pushing one of those buttons, we should scroll to reveal the selected match. It shouldn't be complicated; similar functionality is already used for most of the existing commands. See the scrollToSelectedMatch method.

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Jun 19, 2017

Contributor

How about this: after pushing one of those buttons, we should scroll to reveal the selected match.

The downside of that heuristic is that the currently selected match might not even be in the visible area before the user clicks the button.

Contributor

dirk-thomas commented Jun 19, 2017

How about this: after pushing one of those buttons, we should scroll to reveal the selected match.

The downside of that heuristic is that the currently selected match might not even be in the visible area before the user clicks the button.

@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Jun 19, 2017

Contributor

That's true, but I think it's a reasonable behavior, and it's not obvious that it's any less desirable than scrolling to the currently visible match. It's also slightly easier, so I'm inclined to start with that.

Contributor

maxbrunsfeld commented Jun 19, 2017

That's true, but I think it's a reasonable behavior, and it's not obvious that it's any less desirable than scrolling to the currently visible match. It's also slightly easier, so I'm inclined to start with that.

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Jun 21, 2017

Contributor

... scrolling to the currently visible match

Changed in ddd2cf3.

Contributor

dirk-thomas commented Jun 21, 2017

... scrolling to the currently visible match

Changed in ddd2cf3.

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Jun 21, 2017

Contributor

I just found one problem which got introduced when I added the icons to the toggle buttons: I can only click the buttons on the "sides" (beside the icons). If I try to click the buttons in the middle (where the icons are) the clicked event doesn't fire. I must be doing something wrong with the icon. I found a wrong closing tag for my icons (fixed in 09fbc22) but the problem is still the same. I don't see what is different compared to the other icon. Any ideas?

Contributor

dirk-thomas commented Jun 21, 2017

I just found one problem which got introduced when I added the icons to the toggle buttons: I can only click the buttons on the "sides" (beside the icons). If I try to click the buttons in the middle (where the icons are) the clicked event doesn't fire. I must be doing something wrong with the icon. I found a wrong closing tag for my icons (fixed in 09fbc22) but the problem is still the same. I don't see what is different compared to the other icon. Any ideas?

@simurai

This comment has been minimized.

Show comment
Hide comment
@simurai

simurai Jun 22, 2017

Member

I can only click the buttons on the "sides" (beside the icons).

Should be fixed. Not sure why it works for the other icons. Maybe those buttons use some other event bubbling trick.

Member

simurai commented Jun 22, 2017

I can only click the buttons on the "sides" (beside the icons).

Should be fixed. Not sure why it works for the other icons. Maybe those buttons use some other event bubbling trick.

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Jun 22, 2017

Contributor

@simurai Thank you for the fix. I can confirm it works for me too.

@maxbrunsfeld Ready for another round of review I think.

Contributor

dirk-thomas commented Jun 22, 2017

@simurai Thank you for the fix. I can confirm it works for me too.

@maxbrunsfeld Ready for another round of review I think.

@simurai simurai referenced this pull request Jun 22, 2017

Merged

Update context icons #1

@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Jun 22, 2017

Contributor

@dirk-thomas The code looks very good. It looks like incrementLeadingContextLines and friends are not yet tested. Could you add another test that exercises these four methods? Maybe it could do a search with only one match so that it's really easy to reason about what matches are on-screen.

Other than that, I think I'm ready to merge this. Thanks so much for continuing this whole effort, I think it's now going to really make an impact.

Contributor

maxbrunsfeld commented Jun 22, 2017

@dirk-thomas The code looks very good. It looks like incrementLeadingContextLines and friends are not yet tested. Could you add another test that exercises these four methods? Maybe it could do a search with only one match so that it's really easy to reason about what matches are on-screen.

Other than that, I think I'm ready to merge this. Thanks so much for continuing this whole effort, I think it's now going to really make an impact.

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Jun 22, 2017

Contributor

Could you add another test that exercises these four methods?

I added more specs to use all the context lines function to increase/toogle/decrease leading/trailing lines: 61c0951

Maybe it could do a search with only one match so that it's really easy to reason about what matches are on-screen.

I created a function to extract the first match which gets rid of duplicate code. I still checked that the "right" lines are shown though and didn't rely only on the count which would be possible if there is a single match.

Contributor

dirk-thomas commented Jun 22, 2017

Could you add another test that exercises these four methods?

I added more specs to use all the context lines function to increase/toogle/decrease leading/trailing lines: 61c0951

Maybe it could do a search with only one match so that it's really easy to reason about what matches are on-screen.

I created a function to extract the first match which gets rid of duplicate code. I still checked that the "right" lines are shown though and didn't rely only on the count which would be possible if there is a single match.

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Jun 27, 2017

Contributor

@maxbrunsfeld Any update on reviewing / merging this?

Contributor

dirk-thomas commented Jun 27, 2017

@maxbrunsfeld Any update on reviewing / merging this?

@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Jun 30, 2017

Contributor

This is awesome.

Contributor

maxbrunsfeld commented Jun 30, 2017

This is awesome.

@maxbrunsfeld maxbrunsfeld merged commit fa44c22 into atom:master Jun 30, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Jun 30, 2017

Contributor

@dirk-thomas This has been incorporated into Atom master and will go out in Atom 1.20.

Contributor

maxbrunsfeld commented Jun 30, 2017

@dirk-thomas This has been incorporated into Atom master and will go out in Atom 1.20.

@dirk-thomas dirk-thomas deleted the dirk-thomas:toggle_context_lines branch Jun 30, 2017

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Jun 30, 2017

Contributor

Great, thank you. I already have the next patch ready for you: #919 😉

Contributor

dirk-thomas commented Jun 30, 2017

Great, thank you. I already have the next patch ready for you: #919 😉

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Jul 1, 2017

Contributor

Please see #920 which contains a fix for a regression introduced in this patch.

Contributor

dirk-thomas commented Jul 1, 2017

Please see #920 which contains a fix for a regression introduced in this patch.

@simurai simurai referenced this pull request Jul 12, 2017

Merged

Context lines styling #927

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