Re: Window tree and window's internal height

[Eli Zaretskii]

> Date: Sat, 17 Nov 2018 19:39:36 +0100
> From: martin rudalics <address@hidden>
> CC: address@hidden
> 
>  > First, we have this in the ELisp manual:
>  >
>  >       A minibuffer window (*note Minibuffer Windows::) is not part of its
>  >    frame’s window tree unless the frame is a minibuffer-only frame.
>  >    Nonetheless, most of the functions in this section accept the minibuffer
>  >    window as an argument.  Also, the function ‘window-tree’ described at
>  >    the end of this section lists the minibuffer window alongside the actual
>  >    window tree.
>  >
>  > The first sentence is incorrect, isn't it?  Because Emacs disagrees:
>  >
>  >    emacs -Q
>  >    M-: (window-next-sibling) RET
>  >     => #<window 4 on  *Minibuf-0*>
>  >
>  > And if that sentence is incorrect, we don't need the next two
>  > "exceptions", right?
> 
> Right, graph theoretically.  For a long time, however, the Elisp manual
> has used the term "root", for example, in Emacs 22 as follows
> 
>       The return value is a list of the form `(ROOT MINI)', where ROOT
>       represents the window tree of the frame's root window, and MINI is
>       the frame's minibuffer window.
> 
> which implies that Emacs' window tree is not free but a "rooted tree"
> and in a rooted tree every node except the root must have a unique
> parent node.  But the minibuffer window does not have a parent so we
> would have two nodes without parent.

So maybe we should simply say that the mini-window doesn't have a
parent, instead of what we say now?

> Which means that our window tree concept per se is valid but using
> 'window-next-sibling' to get the minibuffer window from the root
> window is an incorrect naming convention.  It's simply there because
> 'window-next-sibling' is a 1-to-1 reflection of the w->next field of
> the window structure in window.h.

Looking at w->next _was_ what tripped me in the first place: I didn't
expect a sole window on a TTY frame to have a non-nil object pointed
by its 'next' pointer.  I don't think we should assume that the ELisp
manual is only for people who work on the Lisp level.

>  >    int
>  >    window_internal_height (struct window *w)
>  >    {
>  >      int ht = w->total_lines;
>  >
>  >      if (!MINI_WINDOW_P (w))
>  >        {
>  >    if (!NILP (w->parent)
>  >        || WINDOWP (w->contents)
>  >        || !NILP (w->next)
>  >        || !NILP (w->prev)
>  >        || window_wants_mode_line (w))
>  >      --ht;
>  >
>  >    if (window_wants_header_line (w))
>  >      --ht;
>  >        }
>  >
>  >      return ht;
>  >    }
>  >
>  > I don't understand any of the conditions except window_wants_mode_line
>  > when we decide whether to decrease the height due to the mode line.
>  > What do the other conditions have to do with the window's height?  I
>  > could perhaps understand the "WINDOWP (w->contents)" part, as some
>  > kind of optimization for non-leaf windows, which assumes the default
>  > of having the mode line, but the rest seem simply wrong, don't they?
>  > (The WINDOWP condition is not really interesting, as I don't see any
>  > call of this function for a non-leaf window.)
> 
> Whatever these three conditions were meant for is beyond my
> imagination.

Well, I guess that answers my question: it's simply a very old bug.

> window_internal_height was always off-limit for me.  I
> wonder how these miscalculations would affect window_scroll_line_based

I don't think they do, because the result is either used as a limit to
clip some value, or for computing half the window-full, which errs by
half a line half of the time anyway (due to integer division).

> (I hardly ever use modeline-less windows) though.

Sounds like almost no one does, or we would have heard about bug#33363
and its ilk a long time ago.

>  > This is the immediate cause of bug#33363, which I could fix either by
>  > a local change in try_window_id, or by modifying
>  > window_internal_height to leave only the window_wants_mode_line
>  > condition there.  If the latter might be unsafe, maybe I should do the
>  > former on the release branch and the latter on master.  WDYT?
> 
> I would call window_body_height on the release branch and on master.

window_body_height attempts to calculate the mode-line and header-line
heights in pixels, which might be expensive and if so entirely a waste
of CPU cycles on TTY frames.  But I guess this indirectly answers my
question, because my proposed change in window_internal_height will
make it the exact equivalent of window_body_height for TTY frames.  So
I think I will make that change on the release branch.

> And I would use window_body_height in window_scroll_line_based too.
> It should then take care of dividers and horizontal scroll bars on GUI
> windows as well.

window_scroll_line_based is never used on GUI frames, and dividers and
scroll bars always have exactly zero size on TTY frames.  Right?

I see only one GUI client of window_internal_height: ns-win.el, where
it calls compute-motion.  But it calls that function in a way that
doesn't trigger the call to window_internal_height, so I think we are
safe.  However, maybe ns-win.el should switch to using
window-body-height instead.

Thanks.