bug#38828: 26.3; Customized mode line breaks height of vertical scroll bar

[martin rudalics <rudalics@gmx.at>]

[-- Attachment #1: Type: text/plain, Size: 2941 bytes --]

 > AFAICT, when the ml_row->mode_line_p flag is reset, ml_row->height is
 > also zero, so disregarding the flag would not have helped us in this
 > case.

Right, I've been there initially.

 >> For the record: Rewriting 'window_box_height' as
 >>
 >> int
 >> window_box_height (struct window *w)
 >> {
 >>     return WINDOW_BOX_TEXT_HEIGHT (w);
 >> }
 >>
 >> fixes the problem here.
 >
 > Thanks, but that's not an idea I would like entertaining.

Neither do I.

 > As much as
 > window_box_text_height

"window_box_height" presumably, also everywhere below.

 > and WINDOW_BOX_TEXT_HEIGHT look similar,
 > there's a subtle difference between them.  The latter is supposed not
 > to be called during redisplay, because window's mode_line_height field
 > is not guaranteed to be accurate then,

Your patch does use it though.

 > and the fallback is the same
 > estimate_mode_line_height which doesn't support non-character display
 > elements.  It is generally okay to use WINDOW_BOX_TEXT_HEIGHT in
 > window.c,

window.c doesn't use it.  It's used in xdisp.c only.  And I'm sure it
can return negative values, sometimes.  So I presume we can do away
with it then?

 > because that code runs outside of redisplay, and the window
 > object should be up-to-date.  But window_box_text_height is called
 > _a_lot_ from within redisplay itself.  And here we get to another
 > subtlety: window_box_text_height uses the window's current_matrix,
 > which might not be up-to-date yet, depending on where it is called,
 > because we only update it in update_frame, and set_vertical_scroll_bar
 > is called long before that.
 >
 > When a window is resized, as happens in this scenario when the
 > echo-area message is cleared, the mode-line row changes to a different
 > one (because it is always the last row of the window), and the new row
 > didn't yet get updated.  We have the correct data in the
 > desired_matrix, but not in the current_matrix.  I considered using the
 > desired_matrix in this case, but that is also somewhat scary: the
 > desired matrix could be very sparse under some redisplay
 > optimizations, so we'd need some logic to verify the data is valid
 > before using it.

I've tried via

	  w->mode_line_height
	    = MATRIX_MODE_LINE_ROW (w->current_matrix)->height
	    = DESIRED_MODE_LINE_HEIGHT (w);

after displaying the mode lines without great success (likely because
window_box_height didn't go for it then).

 > Eventually, a simpler solution is just to fall back
 > to the window's mode_line_height field, before falling back to
 > estimate_mode_line_height, because when the mode-line height changes,
 > we schedule an immediate thorough redisplay of the window, and
 > invalidate that window's mode_line_height field, to be recomputed by
 > the rescheduled redisplay.  See the proposed patch below.

This fails here when loading the attached test-popup-2.el and typing,
for example, F2 <up> F3 <up> F2 <up>.

martin

[-- Attachment #2: test-popup-2.el --]
[-- Type: text/plain, Size: 1490 bytes --]

(defvar old-mode-line-format mode-line-format)

(defun foo-message (&optional newline)
  (message
   (concat (if newline "\n" "")
	   "total: %s, body: %s, mode: %s")
   (window-pixel-height) (window-body-height nil t)
   (window-mode-line-height)))

(defun foo ()
  (interactive)
  (foo-message))

(global-set-key [f1] 'foo)

(defun test-popup ()
  (interactive)
;;   (set-face-attribute 'mode-line nil :height 200)
;;   (set-face-attribute 'mode-line-inactive nil :height 200)
  (setq
   mode-line-format
   (cons (propertize " " 'display
                     (create-image "/* XPM */ static char * image[] = {
\"3 60 1 1\",
\"0 c #00aaff\",
\"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
\"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
\"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
\"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
\"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
\"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
\"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
\"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
\"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
\"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
\"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\",
\"000\",\n\"000\",\n\"000\",\n\"000\",\n\"000\"
};" 'xpm t :ascent 'center))
	 mode-line-format))
  (foo-message t))

(global-set-key [f2] 'test-popup)

(defun bar ()
  (interactive)
  (setq mode-line-format old-mode-line-format))

(global-set-key [f3] 'bar)