Add configurable pane direction.

[JaredReisinger]

Addresses #99 and part of #328. Does not address any kind of "opposite" setting.

Migrates existing true/false (pane to the right or not) config settings into "right"/"none" equivalent value.

Note: In order to migrate an exiting setting, we have to run code very early when the module loads, before it activates. This is unfortunate from a performance standpoint. If we're willing to simply lose any previous configuration setting (falling back to the default, "right"), we can avoid the perf impact of doing work during module load. I'm completely open to the wisdom of the Atom community here; I just wanted to start with an already-backward-compatible option.

[JaredReisinger]

Not sure why travis timed out. I do get two errors from apm test, but I also get those two errors on master; they weren't introduced by the PR.

[winstliu]

Yes, master is currently failing due to language-javascript changes. Unfortunately I can't keep both Atom core and markdown-preview green (until the JS changes land in a released Atom version)...it's either one or the other 😦.

[acontreras89]

An alternative would be to keep the flag and add the enumeration, so that false never splits and true splits in splitPaneDirection (or whatever you call the new config option.)

And just to be clear, I'm not saying it would be best, just wanted bring an alternative to discussion.

On the other hand, there's one more thing I wanted to bring to the table. This functionality currently relies on the split parameter of Workspace's open method. However, this method will not create new panes when the direction is left or up, as seen here.

[JaredReisinger]

@acontreras89 : Does that mean that the documented behavior is incorrect?

Regardless, the 95+% case (and the one I personally want) is simply to add 'bottom' as an option; dropping the functionality down to just none/right/bottom is totally acceptable to me, and doesn't prevent left/top from being added in the future. Also, leaving the flag in place and only checking the string/enum when true is a reasonable user experience, it just felt a little redundant to me to have a superfluous enum when the flag was false... but that's a very subjective thing. @50Wliu, can you add some insight and/or tribal wisdom here? (For example, is it generally better for package configurations to be changed in "simply additive" ways?)

[acontreras89]

The documented behaviour doesn't give any details as to what happens when no pane is available in the given direction. @lee-dohm, maybe it should state that for down and right, a new pane will be created when none is available for reuse (or the opposite, as the term split already implies creating a new pane.)

It also says

split Either left, right, top or bottom.

although the values to obtain a vertical splits are up and down.

With regard to the config. options, I can see your point @JaredReisinger. I personally prefer to take the approach that better fits my mental model, in which (1) deciding whether to use the current pane or a new one is a boolean thing, no matter what comes after that choice, and (2) having directions and a nil value grouped together doesn't quite feel as good (readability- and maintainability-wise). This is a matter of personal preference, though.

I thought it was worth proposing this alternative as it would also avoid migrating the configuration.

Despite this, if I could have a dropdown with all five options and two underlying setting values a boolean and a direction,) I'd go with that. It seems like a better user experience to just have one config option.

[lee-dohm]

I made the doc changes that @acontreras89 pointed out. They're on master though, so they won't show up in the API docs until v1.7.0 😦

[JaredReisinger]

@acontreras89 : All good points, and I really hate the config migration code. I'll put this back to flag+enum, and reduce the available enum options to "right" and "down", so it actually works. ;-)

[acontreras89]

I'm glad we agree 😄 but keep in mind I'm a regular contributor, just like you. Wouldn't mind asking for a second opinion on this before making any changes.

For now, we could add some specs, as updating them later on shouldn't be a problem.

[lee-dohm]

Because Atom doesn't have a mechanism for one setting disabling another, such as the checkbox for on/off and dropdown for direction, this tends to be confusing for people. Because of that, the dropdown with all five options is much preferred 😀

[acontreras89]

I see your point @lee-dohm. Off topic, do you think that kind of behaviour would be nice to have?

[JaredReisinger]

Hah! I pushed my change before seeing @lee-dohm's comment. I'll revert the commit, but then still drop the list from 5 values to 3 (none, right, and down).

[JaredReisinger]

Okay... we're back to a single openPreviewInSplitPane value, one of none, right, or down, with a default of right for consistency with previous behavior. Neither left nor up are supported, as the underlying pane implementation doesn't support this yet. Any previous boolean openPreviewInSplitPane value is not migrated to the new enum value, in order to avoid running migration code at module-load time. The downside of this is that anyone who's explicitly turned off the split pane will have to do so again after updating the package.

Finally, the unit tests were modified to check the PaneAxis orientation, with new tests to explicitly check that right results in a horizontal orientation, and down results in a vertical orientation. We don't check the relative position of the panes, in part because that amounts to testing atom.workspace.open() rather than the preview code itself. (Perhaps listening to atom.workspace.onDidAddPane() would be better, but the current test is probably good enough for now.) Two tests will fail, but this is a known, unrelated issue.

[maxbrunsfeld]

Thanks for the very clear update @JaredReisinger. Apologies that nobody from the core team has looked at this for the last couple of weeks.

I am ok with the decision not to migrate the old setting. I also think that since we aren't trying to remain backwards-compatibility in that respect, we should rename the setting. The name openPreviewInSplitPane makes it sound like a boolean to me. I would suggest splitPaneDirection, as @acontreras89 said.

[JaredReisinger]

@maxbrunsfeld : Sounds good. (And apparently I'm not much better at following up on this PR either! Having a local build—love the link to local package feature!—means I've been running with "preview below" on my vertical monitor, so I'm a happy camper even if this take a while to get in.)

Same deal as before for the two failing tests: it's a known issue, and unrelated to the change/PR.

[JaredReisinger]

@maxbrunsfeld: After much delay (sadly), I happened to check back and merge in all of the changes that have happened in the interim. The great news is that the (unrelated) previously failing tests have been fixed, and this PR now looks better than it did before, check-wise. If there's anything I can do to help get this reviewed, please don't hesitate to ask. Thanks!

[walles]

@JaredReisinger, is the merge conflict in README.md blocking getting this reviewed?

[JaredReisinger]

@walles: Possibly. I simply hadn't checked back on this PR for ages, and didn't see it now had a merge conflict. It was an easy fix, though, so it should no longer be a blocker.

[JaredReisinger]

Is it just me, or does the recent AppVeyor failure have absolutely nothing to do with this change? It's failing to retrieve the build package from GitHub for the beta x64 build. (It doesn't even get to the tests.) The other AppVeyor builds complete successfully.

[winstliu]

Don't worry about CI failures. They don't block the merge and if there's a legitimate failure we'll tell you.

[JaredReisinger]

Good to know! Thanks, @50Wliu!

[JaredReisinger]

@rafeca : This PR has been languishing for over three years since I first offered it, but I still find the idea useful since I tend to run my Atom windows "tall and narrow", so an up/down split for Markdown previews is desirable. Would you consider merging this (once the merge conflicts are resolved, or perhaps if I close this and create an all-new PR)? If you don't think the feature is useful, I'll just close this and live with moving the panes by hand. Thanks!

[rsese]

Really sorry for the delay here @JaredReisinger:

Would you consider merging this (once the merge conflicts are resolved, or perhaps if I close this and create an all-new PR)? If you don't think the feature is useful, I'll just close this and live with moving the panes by hand.

I'll bring this up for you with the other maintainers and we'll followup as soon as we can.

[rsese]

Just wanted to let you know that we chatted about your PR @JaredReisinger and are 👍 on reviewing it if you can go ahead and resolve the merge conflicts.

Thanks very much for checking in with us about this and sorry again for the delay.