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

Already on GitHub? Sign in to your account

Adds support for multiple wrap guide lines. #56

Merged
merged 2 commits into from Jan 14, 2019

Conversation

Projects
None yet
9 participants
@lexicalunit
Copy link

lexicalunit commented Aug 11, 2016

This patch creates guides at the columns listed in wrap-guide.guides.
Unfortunately wrap-guide.columns is a deprecated setting and I want to
avoid any potential conflicts.

It also creates a distinction between element .wrap-guide and a new
element, .wrap-guide-container. The container class now contains
multiple child nodes of class .wrap-guide. Tests updated accordingly.

Configuring guide styles will still work as indicated in README.md.

Potential weirdnesses:

  • The wrap-guide.guides setting is not language-specific.
  • Soft wrap is still governed by editor.preferredLineLength, which is
    language-specific. This can differ with the setting for the right
    most guide, causing dissonance.

Closes #14.

@lexicalunit

This comment has been minimized.

Copy link
Author

lexicalunit commented Aug 11, 2016

WIP because I have yet to add new spec tests to test multiple wrap guides specifically.

@lexicalunit lexicalunit force-pushed the lexicalunit:lexicalunit/mulit-wrap-guide branch from 3d043d8 to f60c5b4 Aug 11, 2016

@lexicalunit

This comment has been minimized.

Copy link
Author

lexicalunit commented Aug 11, 2016

I would also love to get some feedback on the best way to resolve the potential weirdnesses listed above.

The way I resolved it in lexicalunit/multi-wrap-guide was to always treat the right most guide as the editor. preferredLineLength, meaning:

  1. If wrap-guide.guides changes, this entails updating editor.preferredLineLength to the right most column.
  2. Make wrap-guide.guides support the language-specific config API.

Potential weirdness in this situation would be if the user has a wrap-guide.guides set to something and then manually updates their editor.preferredLineLength. Do we update the right most column to match the new setting? Do we just ignore the update to editor.preferredLineLength and re-set it back to the right most column in wrap-guide.guides? Or...?

@lee-dohm

This comment has been minimized.

Copy link
Member

lee-dohm commented Aug 11, 2016

If we're going to make the right-most guide equivalent to preferredLineLength in one direction, the same should be true in the other direction. So I would suggest that if someone updates preferredLineLength it should update the wrap-guide.guides setting as well.

This also means that wrap-guide.guides would need to support the language-specific config API.

@lexicalunit lexicalunit force-pushed the lexicalunit:lexicalunit/mulit-wrap-guide branch from f60c5b4 to 4cf77e2 Aug 12, 2016

@lexicalunit

This comment has been minimized.

Copy link
Author

lexicalunit commented Aug 12, 2016

Ok, I've added a number of spec tests to verify the multiple wrap guide feature.

Commit message updated:

Soft wrap is still governed by editor.preferredLineLength, which is
language-specific. wrap-guide.guides is also language-specific and
the right-most guide defines the preferred line length. Updates to
one will change the other, and vice-versa.

PR is no longer a work-in-progress :)

@lexicalunit lexicalunit changed the title WIP: Adds support for multiple wrap guide lines. Adds support for multiple wrap guide lines. Aug 12, 2016

@lexicalunit lexicalunit force-pushed the lexicalunit:lexicalunit/mulit-wrap-guide branch 2 times, most recently from cc5efd4 to 2b8e702 Aug 12, 2016

@lexicalunit

This comment has been minimized.

Copy link
Author

lexicalunit commented Aug 17, 2016

@50Wliu no longer work-in-progress :)

@lexicalunit lexicalunit force-pushed the lexicalunit:lexicalunit/mulit-wrap-guide branch from 2b8e702 to feee7a9 Aug 20, 2016

@MikeRatcliffe

This comment has been minimized.

Copy link

MikeRatcliffe commented Oct 8, 2016

Been using this for a few weeks now and haven't found any issues.

@50Wliu 50Wliu removed the needs-testing label Oct 8, 2016

@lexicalunit lexicalunit force-pushed the lexicalunit:lexicalunit/mulit-wrap-guide branch 2 times, most recently from 3fcd284 to cc2a859 Mar 3, 2017

@lexicalunit

This comment has been minimized.

Copy link
Author

lexicalunit commented Mar 3, 2017

PR updated following the recent merge of #65.

@50Wliu

This comment has been minimized.

Copy link
Member

50Wliu commented Mar 6, 2017

Thanks @lexicalunit, and sorry for the delay in reviewing. I'll try to get someone to review it soon.

@piranna

This comment has been minimized.

Copy link

piranna commented Mar 23, 2017

Any update on this? Could someone merge that or tell me how to use it myself?

@lexicalunit lexicalunit force-pushed the lexicalunit:lexicalunit/mulit-wrap-guide branch from cc2a859 to 3d057f0 Mar 23, 2017

@lexicalunit

This comment has been minimized.

Copy link
Author

lexicalunit commented Mar 23, 2017

@piranna you can currently get multiple wrap guides a number of ways:

  1. You can manually install wrap-guide and then pull in my changes.
$ git clone git@github.com:atom/wrap-guide.git
$ cd wrap-guide
$ git pull git@github.com:lexicalunit/wrap-guide.git lexicalunit/mulit-wrap-guide
$ npm install
$ apm link

Of course you'll need to have set up git, npm, and apm for this option to work. And you'll need to keep around the repository and manually update it yourself using git.

  1. Alternatively, there are packages that add support for multiple wrap guides. Such as my own multi-wrap-guide.

  2. And there are other packages out there that offer similar functionality. For example omni-ruler, but I don't use those so I can't vouch for them personally.

@piranna

This comment has been minimized.

Copy link

piranna commented Mar 23, 2017

Alternatively, there are packages that add support for multiple wrap guides. Such as my own multi-wrap-guide.

Forked on this one, and with horizontal rules too, you rocks! :-D

@lexicalunit lexicalunit force-pushed the lexicalunit:lexicalunit/mulit-wrap-guide branch from 3d057f0 to cee646b Apr 19, 2017

@lexicalunit

This comment has been minimized.

Copy link
Author

lexicalunit commented Apr 19, 2017

Welp. I rebased on the latest from master and this completely breaks all spec tests. I've tested it in my editor and it still works just fine. But since we now attach to .scroll-view instead of .lines something has gone horribly wrong. This used to work:

    getWrapGuides  = ->
      wrapGuides = []
      atom.workspace.getTextEditors().forEach (editor) ->
        guides = editor.getElement().rootElement.querySelectorAll(".wrap-guide")
        wrapGuides.push(guides) if guides
      wrapGuides
      expect(atom.workspace.getTextEditors().length).toBe 1
      expect(getWrapGuides().length).toBe 1

But now the expectation fails. I have no idea why. Seems like it should work just fine. Oh well. I'm going to continue to maintain the package multi-wrap-guide and abandon this PR. Someone else can take it over if they want.

@lexicalunit

This comment has been minimized.

Copy link
Author

lexicalunit commented Apr 19, 2017

Hrm. Weird. The spec tests passed on Travis CI. They fail completely for me using Atom 1.17beta on my local machine though.

@lexicalunit

This comment has been minimized.

Copy link
Author

lexicalunit commented Sep 24, 2017

@50Wliu Sorry, I missed the message about adding the config schema. There wasn't previously a schema for wrap-guide.columns nor wrap-guide.enabled, so I wasn't sure if it would make sense to add one for just wrap-guide.guides.

I added a config schema for guides and enabled and left columns undocumented as it seems like it's a deprecated feature. Possibly, if it is deprecated, we could remove it at this time and rename wrap-guide.guides to wrap-guides.columns 🤷‍♀️


updateGuidesCallback = (args) =>
# ensure that multiple guides stay sorted in ascending order
guides = uniq(args.newValue)

This comment has been minimized.

@50Wliu

50Wliu Sep 25, 2017

Member

To avoid bringing in a new dependency, how about the following instead?

guides = args.newValue.filter((item, index) -> args.newValue.indexOf(item) is index)

It's unclear to me if uniq sorts the array. Nevertheless, that can also be done easily: guides.sort((a, b) -> a - b)

This comment has been minimized.

@lexicalunit

lexicalunit Sep 26, 2017

Author

Yep, makes sense to me. I wasn't totally sure if we wanted to bring in the dependency or just roll our own solution here. PR updated. I also made the function use more clear by naming it uniqueAscending().

@lexicalunit lexicalunit force-pushed the lexicalunit:lexicalunit/mulit-wrap-guide branch from 4967bf7 to e4fa639 Sep 26, 2017

@50Wliu

This comment has been minimized.

Copy link
Member

50Wliu commented Nov 28, 2017

@lexicalunit is there ever a use case where you might want a guide that's past the editor.preferredLineLength setting? For example #14 (comment).

@lexicalunit

This comment has been minimized.

Copy link
Author

lexicalunit commented Nov 28, 2017

@50Wliu I have a similar setup to the one you linked to. For me it's the following:

  • 72: Your git commit message is too long!
  • 80: Generally the size of a terminal
  • 100: Lots of style guides use this
  • 120: My personal preference

As far as I know, the only thing editor.preferredLineLength is used for is to indicate where soft wrap should happen.

So in my setup, soft wrap doesn't kick in until we get to the final guide. The other guides are there for me to know when I need to insert newlines if, for example, I'm writing a git commit message in Atom. Note that I also have it so when I right click on a wrap guide I can pick "Line Break at this Guide" and have the line-length-break package automatically hard wrap my text at that guide if I ever need to do something like that.

So no, I haven't found a reason to have soft wrap kick in before the last guide except for the case where the edge of the Atom window comes before the wrap guide. I don't think @amazingant is talking about having editor.preferredLineLength be less than the last wrap guide in their comment.

I'm trying to imagine why someone might want to do it though. If they did, the guides past the preferred line length would be of no use since no text would ever be able to reach them.

@amazingant

This comment has been minimized.

Copy link

amazingant commented Nov 28, 2017

@lexicalunit: Yes, my guides are for the same mental reminder of "you should probably add a line break here for content type X".

The only place where I have a hard wrap before the guide is in my vim config when I'm editing a git commit message. I have a guide line in vim at 80, but the first line only of the commit message hard wraps at 72. I wouldn't immediately think to use atom's editor.preferredLineLength for that kind of context-sensitive hard wrapping anyway.

@lexicalunit lexicalunit force-pushed the lexicalunit:lexicalunit/mulit-wrap-guide branch from e4fa639 to f7b9f03 Nov 28, 2017

@lexicalunit

This comment has been minimized.

Copy link
Author

lexicalunit commented Nov 28, 2017

@50Wliu: I just rebased on master to resolve conflicts. Since wrap-guide.columns is now gone, maybe now is the time to s/guides/columns/g in this PR? 🤷‍♀️

@50Wliu

This comment has been minimized.

Copy link
Member

50Wliu commented Nov 28, 2017

@lexicalunit @amazingant thanks for the explanations :).

I will discuss with the team about whether it's safe or not to replace the removed columns setting with this one.

@lexicalunit lexicalunit force-pushed the lexicalunit:lexicalunit/mulit-wrap-guide branch from f7b9f03 to 90ffcf5 Nov 28, 2017

@lee-dohm lee-dohm added the triaged label Nov 19, 2018

@smashwilson

This comment has been minimized.

Copy link
Member

smashwilson commented Jan 14, 2019

I will discuss with the team about whether it's safe or not to replace the removed columns setting with this one.

I'm 👍 on using wrap-guide.columns here. The old wrap-guide.columns setting had a pretty different schema, so pre-existing settings lurking in peoples' config files will be disregarded quietly and overwritten rather than, for example, raising an exception somewhere.

Ping me when you're ready and I'll be happy to merge and 🚢 😄

@lexicalunit

This comment has been minimized.

Copy link
Author

lexicalunit commented Jan 14, 2019

In that case, shall I update the PR to use wrap-guide.columns now?

@smashwilson

This comment has been minimized.

Copy link
Member

smashwilson commented Jan 14, 2019

Yes please 🙇

lexicalunit added some commits Aug 11, 2016

lexicalunit
Adds support for multiple wrap guide lines.
This patch creates guides at the columns listed in `wrap-guide.guides`.
Unfortunately `wrap-guide.columns` is a deprecated setting and I want to
avoid any potential conflicts.

It also creates a distinction between element `.wrap-guide` and a new
element, `.wrap-guide-container`. The container class now contains
multiple child nodes of class `.wrap-guide`. Tests updated accordingly.

Configuring guide styles will still work as indicated in `README.md`.

Soft wrap is still governed by `editor.preferredLineLength`, which is
language-specific. `wrap-guide.guides` is also language-specific and
the right-most guide defines the preferred line length. Updates to
one will change the other, and vice-versa.

Closes #14.

@lexicalunit lexicalunit force-pushed the lexicalunit:lexicalunit/mulit-wrap-guide branch from 90ffcf5 to e44889d Jan 14, 2019

@lexicalunit

This comment has been minimized.

Copy link
Author

lexicalunit commented Jan 14, 2019

Done, and rebased on master too.

@smashwilson

This comment has been minimized.

Copy link
Member

smashwilson commented Jan 14, 2019

Excellent, thanks!

@smashwilson smashwilson merged commit ff2cb69 into atom:master Jan 14, 2019

2 checks passed

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

@lexicalunit lexicalunit deleted the lexicalunit:lexicalunit/mulit-wrap-guide branch Jan 14, 2019

@piranna

This comment has been minimized.

Copy link

piranna commented Jan 14, 2019

Does this one have horizontal rules? If so, multiple ones as I requested in the other repo?

@lexicalunit

This comment has been minimized.

Copy link
Author

lexicalunit commented Jan 14, 2019

Sorry, this PR was just for vertical columns.

@piranna

This comment has been minimized.

Copy link

piranna commented Jan 14, 2019

Should I create a new one here, or is enough with the one in the other repo as reminder? :-)

@JosephTLyons

This comment has been minimized.

Copy link

JosephTLyons commented Jan 14, 2019

When will this feature ship?

@daviwil

This comment has been minimized.

Copy link
Member

daviwil commented Jan 14, 2019

@JosephTLyons I'll be publishing an update to wrap-guide and then update it on Atom master shortly, so it'll show up in tomorrow's Nightly release or the next Atom Beta.

@JosephTLyons

This comment has been minimized.

Copy link

JosephTLyons commented Jan 19, 2019

Amazing! I’ve been wanting this feature natively for so long! I didn’t care for the multi line package or hacking it, this will be a great addition!

@JosephTLyons

This comment has been minimized.

Copy link

JosephTLyons commented Mar 12, 2019

So this will be showing up in the Atom 1.36 then?

@lee-dohm

This comment has been minimized.

Copy link
Member

lee-dohm commented Mar 12, 2019

Yes, it is currently scheduled to be released with v1.36.

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