Fix bug that prevents helm from providing ESS starting directory

[dankessler]

When R is started "implicitly" (e.g., with C-RET on a line of code in an R buffer not associated with a session), the invocation of R gets wrapped in the ess--with-no-pop-to-buffer macro, which binds the display-buffer-ovverriding-action such that new buffers cannot be displayed. Although presumably well-intentioned, this has the unfortunate side-effect of screwing things up for helm users, since when/if ess tries to prompt the user for a directory, helm steps in and tries to display a new buffer in which the user can make the choice, but its efforts are thwarted by the wrapper.

This gives the Wrong type argument: window-live-p, nil error of #1074 because helm expects to get a (handle to a) buffer but instead gets nil when its attempts are squashed.

This commit fixes things in a hacky way but hopefully sufficiently surgical manner: it expands the let form that wraps ess-prompt-for-directory to rebind display-buffer-overriding-action to nil, which undoes (only for the purposes of prompting for a directory) the attempts of the ess--with-no-pop-to-buffer macro to prevent this kind of behavior.

The thread for #1074 is long and involved, but discussion relevant to this commit is in this comment.

[nettoyoussef]

Not sure if it affects any other function/process, but for the intended use case (i.e., use with helm), the solution works fine for me.

[vspinu]

This is simple and works but still might interfere with the user customization even for how helm buffer is displayed. I think the right thing is to get rid of ess--with-no-pop-to-buffer which is an ugly hack.

There are at least 2 related problems:

1. The process window is not shown till the very end. The process might be stuck for whatever reason and I don't think this is correct. The process buffer should be shown immediately.
2. On new process startup with C-c C-s *new* RET the buffer is not shown at all.

So I would propose to spend a bit more time on this one and nail all these problems once and for all. WDYT?

[dankessler]

This is simple and works but still might interfere with the user customization even for how helm buffer is displayed. I think the right is to get rid of ess--with-no-pop-to-buffer which is an ugly hack.

I'd be perfectly happy to just get rid of ess--with-no-pop-to-buffer but don't want to break something in the process. To be honest, I didn't really understand why it's there in the first place so I was just trying to work around it. What are the relevant user customization options here? The only thing I can find related to ESS and pop is ess-help-pop-to-buffer, which I don't think is relevant.

As far as I can tell, ess--with-no-pop-to-buffer is only used in two places (here and here); both are within the body of ess-request-a-process inside of ess-inf.el.

I tried simply getting rid of the two calls to ess--with-no-pop-to-buffer. However, this causes at least some funky behavior: implicitly starting a new session (e.g., open somefile.R buffer, do C-RET on a line, then specify the starting directory) results (for me) in having two windows both displaying the session (it clobbers the window with the somefile.R buffer). Perhaps this is what the macro was trying to fix?

1. The process window is not shown till the very end. The process might be stuck for whatever reason and I don't think this is correct. The process buffer should be shown immediately.

When you say "not until the very end", do you mean after the user is prompted for a starting directory? Similarly, when you say "immediately," what do you mean? Should the new buffer be displayed before R is even running inside of it, or is the goal for it to be displayed as soon as R is being launched (i.e., roughly immediately after the user provides the starting directory, if desired).

2. On new process startup with C-c C-s *new* RET the buffer is not shown at all.

Good catch! Getting rid of calls to ess--with-no-pop-to-buffer fixes this (but has the strange behavior discussed above).

So I would propose to spend a bit more time on this one and nail all these problems once and for all. WDYT?

So it seems that just killing the calls to the macro are probably not a satisfactory solution. Can you remember (or point me to) what the macro is trying to accomplish, and what user options needs to be preserved? I think I can get this fixed if I have a good mental model of what the desired behavior should be (e.g., should a new window always be dedicated to a newly launched R session, regardless of how it is invoked?).

[vspinu]

Those calls are there for a reason but not the best way to go about that. Maybe we could wrap that code into our own dynamic variable and behave accordingly just before the actual display. We can still bind the display buffer alist but for a much narrowed scope. Currently the brush is too wide.

[dankessler]

So I would propose to spend a bit more time on this one and nail all these problems once and for all. WDYT?

My impetus for submitting this PR in the first place was just to narrowly fix the specific bug RE helm (which probably bites a lot of users), although I admit that my fix is shallow.

I agree that now is a natural time to try to fix things properly and am happy to be a part of that, but since AFAICT my fix doesn't make things any worse while un-breaking things for helm users, perhaps we could just accept this PR and then discuss the desired design/fix for suppressing display's more thoughtfully in a fresh issue in the hope that that can lead to a more comprehensive PR?

In the meantime, I'm trying to wrap my head around how things are supposed to work and/or what the macro is trying to accomplish toward that goal (other than its literal behavior of clobbering the display of any new windows within the wrapped code). I've tried blaming which led me to 21cb413184765ca86138b69f8a3e806a4421bdfe and 0ee75821d653ac362c72dbdb219b29244211118f, which offer some clues and ultimately led me to #987. I think fundamentally the goal is to appropriately support the kind of behavior described in the ESS manual: 3.5 Controlling buffer display, so perhaps I can go read that carefully along with #987 and think about how to more carefully wield a finer brush here, but I hope we'll consider just rolling out the hacky helm fix in the meantime.

[mmaechler]

Merging for now... even though it adds a hack on top of another hack ... which has not been addressed yet

[mmaechler]

Sorry guys, I seem unable to deal with this pull request properly. ... doing it manually instead