Fix window configurations not getting restored due to broken split-window call

[caadr]

visual-fill-column-split-window was added as a window parameter to make splitting possible with wide margins.

It gets called by split-window with 3 args

    (throw 'done (funcall function window size side))
    ;; Head:     86d8d76aa3 ; lisp/ldefs-boot.el: Update.
    ;; Tag:      emacs-27.1
    ;; emacs/lisp/window.el

but has an excess arg in its signature that gets passed back to split-window as nil

    (defun visual-fill-column-split-window (&optional window size side pixelwise)
      ...
      (setq new (split-window window size side pixelwise)) ;; pixelwise => nil

Thus, window sizes are not calculated correctly and “Window too small to split” errors occur that block window configurations from getting restored with window-state-put. It also messes with the window dimensions around the split-window call. The problem is masked by a (condition-case nil... (error nil)).
This affects switching doom emacs workspaces / persp-mode perspectives when the selected window has visual-fill-column mode turned on.

Fixes:
Bad-ptr/persp-mode.el#114
Bad-ptr/persp-mode.el#110
joostkremers/writeroom-mode#54

[joostkremers]

I notice you also pass t for the argument pixelwise when visual-fill-column-split-window calls split-window. That makes me a little hesitant to adopt this solution, because what would happen if a Lisp program calls split-window with an explicit nil argument for pixelwise? I realise that it would be dropped in split-window, but I'm afraid that it might cause cases where everything works now to go wrong...

I can't find anything in the Elisp manual about the rationale of not passing the pixelwise argument on to the function in the split-window parameter (it is mentioned, but not explained), so I'd need a bit more background to be sure this is the right fix.

BTW, the better fix would probably be to use the min-margins window parameter, which was added specifically for this type of thing is Emacs 25, but I never got round to implementing that.

[caadr]

You are absolutely right! I did some more digging:

Adding pixelwise to the funcall in split-window seems to fix the problem “cleanly”… maybe a case for the mailing list? Looking at the git blame, the person who wrote visual-fill-column-split-window and the funcall is the same (Martin Rudalics).

Otherwise I could not think of a non-hacky way to prevent a conflict here. When split-window gets called with pixelwise = t it will get niled on the round trip through visual-fill-column-split-window.

Until the better fix with min-margins is implemented, I can only come up with

• cleaning up the signature to avoid confusion, but not hardcoding t
• removing the window parameter when pixelwise should be t, either
  • somewhere in the call chain, e.g. in persp-window-state-put-function
  • or more generally with something like:

(defadvice! fix/reset-split-window-param-maybe-a
  (&optional window _size _side pixelwise)
  :before #'split-window
  (and pixelwise
       (eq (window-parameter window 'split-window)
           #'visual-fill-column-split-window)
       (set-window-parameter window 'split-window nil)))

With this, manual splitting works for me, as well as persp switching. The window param seems to get restored to visual-fill-column-split-window through the window-size-change-functions hook.

EDIT: looking at this, I guess one could also create a version of visual-fill-column-split-window in the advice that passes the correct pixelwise value.

;; EDIT: the pixelwise paramter got removed from `visual-fill-column-split-window' recently.
;; For this hack to work `visual-fill-column' has to be pinned in packages.el
;;
;; (package! visual-fill-column :pin "598bc992f050575c48db6fb9ea50794a5ce5d065")
;;
;; Dont forget to run
;; $ doom sync -u
;; after pinning.
;;
;; * sorry for the doomisms
(defadvice! fix/visual-fill-column-split-window-dispatch-a
  (&optional window _size _side pixelwise)
  "Fixes custom split function bound to window parameter not being able to
pass the value of `pixelwise' back to the `split-window' function."
  :before #'split-window
  (and (eq (window-parameter window 'split-window)
           #'visual-fill-column-split-window)
       (set-window-parameter window 'split-window
                             (lambda (win size side)
                               (visual-fill-column-split-window
                                win size side pixelwise)))))

In that case, the function signature has to keep pixelwise of course.
I think this is fine until a better solution comes around.

[caadr]

Reverted the changes, only updating the docstring for now.

[joostkremers]

Adding pixelwise to the funcall in split-window seems to fix the problem “cleanly”… maybe a case for the mailing list? Looking at the git blame, the person who wrote visual-fill-column-split-window and the funcall is the same (Martin Rudalics).

Yes, but the addition of the argument pixelwise to visual-fill-column-split-window was done by me. Unaware, obviously, that it isn't even passed along. 😞

Besides, Martin said that the solution with visual-fill-column-split-window is fragile, so it shouldn't be a surprise that we're running into issues like this.

Otherwise I could not think of a non-hacky way to prevent a conflict here. When split-window gets called with pixelwise = t it will get niled on the round trip through visual-fill-column-split-window.

Until the better fix with min-margins is implemented, I can only come up with

* ~cleaning up the signature to avoid confusion, but~ not hardcoding `t`

* removing the window parameter when pixelwise should be `t`, either

[...]

With this, manual splitting works for me, as well as persp switching. The window param seems to get restored to visual-fill-column-split-window through the window-size-change-functions hook.

I guess that still doesn't guarantee there won't be edge cases, though. 😞

EDIT: looking at this, I guess one could also create a version of visual-fill-column-split-window in the advice that passes the correct pixelwise value.

(defadvice! fix/visual-fill-column-split-window-pixelwise-a
  (&optional window _size _side pixelwise)
  "Fixes custom split function that is bound as window
parameter not getting called with the argument PIXELWISE from
`split-window'."
  :before #'split-window
  (and (eq (window-parameter window 'split-window)
           #'visual-fill-column-split-window)
       (set-window-parameter window 'split-window
                             (lambda (win size side)
                               (visual-fill-column-split-window
                                win size side pixelwise)))))

In that case, the function signature has to keep pixelwise of course.
I think this is fine until a better solution comes around.

Yes, that looks like it would work.

Still, I plan to implement the min-margins solution ASAP, which will hopefully make all this trickery unnecessary.

[joostkremers]

I implemented the solution with the window parameter min-margins here, but there seems to be a bug in Emacs that keeps it from working properly. There's a fix (see the thread here), but I don't know if it's gonna be in Emacs 27 or 28.

[joostkremers]

The fix has been include into Emacs 27 here. I assume it'll be included in Emacs 27.2. With that fix and an updated visual-fill-column, the split-window window parameter isn't necessary and the bug you ran into shouldn't be a problem any more.

[caadr]

Great! Thanks for looking into that. 27.2. should be around the corner I guess & I'll just use the workaround till then. PR can be closed :)