sleeping editor does not set `with-editor-previous-winconf`
dankessler opened this issue · 4 comments
I had noticed some subtle odd behavior when doing git commit using magit while on tramp which I think I've tracked down to with-editor.
Committing works fine for me via tramp, but after I finished my commit message with C-c C-c, rather than going back to the window configuration I had prior to starting the commit, things would go back to some other, messier state. In vanilla emacs, it's not terribly noticeable: the biggest consequence is that I'm left with a buffer hanging around that has a preview of the commit's diff, but in spacemacs it's a bit uglier: there's some additional config in spacemacs that causes the window configuration to change slightly when preparing the commit message buffer, and then when that buffer dies after C-c C-c I fall back into an uglier state where I no longer have the status buffer up, which is a minor pain if I'm planning to do e.g., a sequence of many small commits, as I have to get the status back up, stage, commit, then find my way back to the status buffer, etc. Over time, I noticed the pattern and decided to do some digging to try to figure out what was going on.
In brief, I think the problem is that the previous window configuration is not being stored when the "sleeping editor" is used.
with-editor advises server-switch-buffer in such a way that if we switch to a buffer using server-switch-buffer (and when appropriate conditions are satisfied) the current window configuration will be stored in with-editor-previous-winconf; this happens here. The local variable with-editor-previous-winconf is then used by with-editor-return when tidying up after itself, but of course if this variable is not set, it can't make use of it.
As far as I can tell the "sleeping editor" filter (defined in with-editor-sleeping-editor-filter) does not use server-switch-buffer, but instead calls either (i) whatever is returned by with-editor-server-window or (ii) switch-to-buffer, neither of which will trigger the advised function, and so when on tramp (or any other time that the "sleeping editor" is used), the previous window configuration doesn't get stored.
I can verify this by setting up a commit window and checking the value of with-editor-previous-winconf: if I'm working locally, then I can see that it is set to something sensible, if I'm working over tramp then it's unset (nil).
As a crude hack, I just inserted
(setq with-editor-previous-winconf
(current-window-configuration))immediately before this line. This both succeeds diagnostically (i.e., with-editor-previous-winconf is now defined as a local variable in COMMIT MSG buffer's whether local or on tramp) and functionally (i.e., my window configuration gets tidied back up to the state it was in prior to me starting the commit process).
Prior to this change, I also get an error message (pasted below) if I cancel (with C-c C-k) a commit message, but with this tweak it now cancels cleanly.
There was a problem with the editor 'sh -c 'printf "\nWITH-EDITOR: $$ OPEN $0\037 IN $(pwd)\n"; sleep 604800 & sleep=$!; trap "kill $sleep; exit 0" USR1; trap "kill $sleep; exit 1" USR2; wait $sleep''. ... [Hit $ to see buffer magit-process: rns for details]
and here are the details from the process buffer
1 git … commit --allow-empty
hint: Waiting for your editor to close the file...
WITH-EDITOR: 7611 OPEN /home/kesslerd/repos/rns/.git/COMMIT_EDITMSG� IN /home/kesslerd/repos/rns
error: There was a problem with the editor 'sh -c 'printf "\nWITH-EDITOR: $$ OPEN $0\037 IN $(pwd)\n"; sleep 604800 & sleep=$!; trap "kill $sleep; exit 0" USR1; trap "kill $sleep; exit 1" USR2; wait $sleep''.
Please supply the message using either -m or -F option.
I'd of course be happy to submit this as a PR, but with-editor is a sufficiently nuanced package that I'm not confident that (a) there isn't a better way to do this, or (b) this is the desired behavior for reasons I don't understand (e.g., there's some business with with-editor-server-window-alist which seems like perhaps it's intended to remedy this issue but I don't really understand it).
I haven't looked at this myself again yet and it might be a while until I get around to that. It seems you have taken a careful look and I would recommend you implement what seems right to you now, before too much time has passed and you have to familiarize yourself with the code again.
I do vaguely remember that I made a conscious choice to not ensure identical behavior, but not whether I was just being lazy or if I encountered some great obstacle.
Sure! I've been using the "crude hack" that I described for several days now and it has indeed improved the experience when committing on tramp and I haven't noticed any regressions.
It feels like there should be a more elegant way to do it but I still haven't thought of anything, and perhaps that's what stopped you earlier. I'll take another careful look to see if I can't think of something better, and then submit a PR with wherever I land.
I've been living with this small patch for a while now and haven't yet encountered any ill effects, so I've submitted it as a PR.