helm-split-window-preferred-function and helm-display-function

[Ambrevar]

I have been tinkering around helm-split-window-preferred-function. Which is a fantastic customization possibility! But things went wrong...

After some debugging, it turns out that helm-split-window-preferred-function gets called twice in helm-default-display-buffer if

• helm-full-frame is nil
• helm-split-window-default-side is not 'same or one-window-p is nil.

Which happens with common configurations including the default one.

This is because helm-default-display-buffer calls both helm-split-window-preferred-function and display-buffer, the latter calling split-window-preferred-function which was locally bound to helm-split-window-preferred-function in the caller.

Calling the function twice is a major issue because the result of the second call will depend on the result of the first call, which makes it extremely convoluted to design any splitting rules. That the default function works is mostly a lucky shot I believe.

To make my helm-split-window-preferred-function work I had to replace helm-display-function:

(defun helm-custom-display-buffer (buffer)
  "Custom function to display `helm-buffer' BUFFER."
  (display-buffer
   buffer `(nil . ((window-height . ,helm-display-buffer-default-height)
                   (window-width  . ,helm-display-buffer-default-width))))
  (helm-log-run-hook 'helm-window-configuration-hook))

(setq helm-display-function 'helm-custom-display-buffer)

This does not seem very productive and I think this calls for some improvements:

1. Let's fix the documentation. helm-split-window-preferred-function should link to the documentation of helm-default-display. helm-display-buffer and helm-default-display-buffer too, and it should say it uses it.
   All functions should explain how they relate to each other. Some configuration variables remain buried deep within the code. It took me a while to realize helm-full-frame existed!

2. helm-display-buffer and helm-default-display-buffer should be revised to not call helm-split-window-preferred-function twice.

3. More generally, what about having only one callback, that is, either helm-display-function or helm-split-window-preferred-function? And leave all configuration variables to this callback instead of being scattered around 3 functions (more?). Namely those variables:

helm-full-frame
helm-split-window-default-side
helm-always-two-windows
helm-reuse-last-window-split-state
helm-split-window-in-side-p ; Also fix this typo: it's "inside", not "in-side"

;; Merge those callbacks into one:
helm-split-window-preferred-function
helm-display-function

[Ambrevar]

Thanks for the update, that seems to do it.

I wrote the following split function:

(defun helm-split-window-combined-fn (window)
  "Helm window splitting that combined most standard features.

- With C-u, split inside. With C-u C-u, use same window.
- Else use biggest other window when available.
- Else split horizontally if width>height, vertically otherwise."
  (cond
   ((or (minibufferp helm-current-buffer)
        (and
         (not (one-window-p t))
         (not (equal current-prefix-arg '(4)))
         (not (equal current-prefix-arg '(16)))))
    ;; Find biggest window.
    (let (biggest (maxarea 0))
      (dolist (w (window-list))
        (unless (eq w (selected-window))
          (let ((area (* (window-pixel-width w) (window-pixel-height w))))
            (when (> area maxarea)
              (setq maxarea area
                    biggest w)))))
      biggest))
   ((equal current-prefix-arg '(16))
    ;; Same window.
    (selected-window))
   (t
    ;; If split inside or if unique window.
    (split-window (selected-window) nil
                  (if (> (window-pixel-width) (window-pixel-height))
                      'right
                    'below)))))

What I found frustrating with the default function is that you could not change the behaviour "live", i.e. choose between "other window", "inside", or "same".

So what do think about my last point, merging the display and the split functions? I'm not sure of the semantic difference between the two when it comes to Emacs.