[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#5718: scroll-margin in buffer with small line count.
From: |
Eli Zaretskii |
Subject: |
bug#5718: scroll-margin in buffer with small line count. |
Date: |
Mon, 12 Sep 2016 20:36:14 +0300 |
> From: npostavs@users.sourceforge.net
> Cc: 5718@debbugs.gnu.org, ahyatt@gmail.com, gavenkoa@gmail.com
> Date: Sun, 11 Sep 2016 16:58:08 -0400
>
> >> >> this_scroll_margin = max (0, scroll_margin);
> >> >> this_scroll_margin
> >> >> = min (this_scroll_margin, window_total_lines / 4);
> >> >
> >> > Which reveals a subtle bug: the actual scroll margin should be 1 for 7
> >> > lines, 2 for 11, etc. The problem is that the value of
> >> > window_total_lines includes the mode line, which it shouldn't. Maybe
> >> > this should be fixed.
>
> I have a patch set for fixing this and allowing the user to change the
> maximum margin from 0.25. The latter doesn't quite work perfectly, for
> some reason when setting the maximum margin to 0.5 and scroll-margin to
> 100, `scroll-down-command' doesn't keep point centered in the window,
> even though other commands (e.g. `scroll-up-command') do.
Thanks, LGTM.
However, I think we need to solve those glitches as part of
introducing the feature. Setting a margin to half the window size
makes centering point difficult, but since some commands do succeed,
I'm guessing that those commands which succeed have a bug, i.e. they
leave point inside the margin. Is that indeed so?
Also, did you test these changes with scroll-conservatively set to
101? If not, please do, as that setting activates some code parts
that no other option does.
A few comments below.
> @@ -16527,10 +16507,8 @@ redisplay_window (Lisp_Object window, bool
> just_this_one_p)
> /* Some people insist on not letting point enter the scroll
> margin, even though this part handles windows that didn't
> scroll at all. */
> - int window_total_lines
> - = WINDOW_TOTAL_LINES (w) * FRAME_LINE_HEIGHT (f) /
> frame_line_height;
> - int margin = min (scroll_margin, window_total_lines / 4);
> - int pixel_margin = margin * frame_line_height;
> + int margin = window_scroll_margin (w, MARGIN_IN_LINES);
> + int pixel_margin = margin * frame_line_height;
> bool header_line = WINDOW_WANTS_HEADER_LINE_P (w);
> /* Note: We add an extra FRAME_LINE_HEIGHT, because the loop
> @@ -16814,12 +16792,7 @@ redisplay_window (Lisp_Object window, bool
> just_this_one_p)
> it.current_y = it.last_visible_y;
> if (centering_position < 0)
> {
> - int window_total_lines
> - = WINDOW_TOTAL_LINES (w) * FRAME_LINE_HEIGHT (f) / frame_line_height;
> - int margin
> - = scroll_margin > 0
> - ? min (scroll_margin, window_total_lines / 4)
> - : 0;
> + int margin = window_scroll_margin (w, MARGIN_IN_LINES);
> ptrdiff_t margin_pos = CHARPOS (startp);
> Lisp_Object aggressive;
> bool scrolling_up;
> @@ -17063,10 +17036,7 @@ redisplay_window (Lisp_Object window, bool
> just_this_one_p)
> {
> int window_total_lines
> = WINDOW_TOTAL_LINES (w) * FRAME_LINE_HEIGHT (f) /
> frame_line_height;
> - int margin =
> - scroll_margin > 0
> - ? min (scroll_margin, window_total_lines / 4)
> - : 0;
> + int margin = window_scroll_margin (w, MARGIN_IN_LINES);
> bool move_down = w->cursor.vpos >= window_total_lines / 2;
Here you call window_scroll_margin 3 times in the same function. Any
way of doing that only once and reusing the result?
> @@ -17298,17 +17267,7 @@ try_window (Lisp_Object window, struct text_pos pos,
> int flags)
> if ((flags & TRY_WINDOW_CHECK_MARGINS)
> && !MINI_WINDOW_P (w))
> {
> - int this_scroll_margin;
> - int window_total_lines
> - = WINDOW_TOTAL_LINES (w) * FRAME_LINE_HEIGHT (f) / frame_line_height;
> -
> - if (scroll_margin > 0)
> - {
> - this_scroll_margin = min (scroll_margin, window_total_lines / 4);
> - this_scroll_margin *= frame_line_height;
> - }
> - else
> - this_scroll_margin = 0;
> + int this_scroll_margin = window_scroll_margin (w, MARGIN_IN_PIXELS);
> if ((w->cursor.y >= 0 /* not vscrolled */
> && w->cursor.y < this_scroll_margin
> @@ -18592,15 +18551,8 @@ try_window_id (struct window *w)
> /* Don't let the cursor end in the scroll margins. */
> {
> - int this_scroll_margin, cursor_height;
> - int frame_line_height = default_line_pixel_height (w);
> - int window_total_lines
> - = WINDOW_TOTAL_LINES (w) * FRAME_LINE_HEIGHT (it.f) /
> frame_line_height;
> -
> - this_scroll_margin =
> - max (0, min (scroll_margin, window_total_lines / 4));
> - this_scroll_margin *= frame_line_height;
> - cursor_height = MATRIX_ROW (w->desired_matrix, w->cursor.vpos)->height;
> + int this_scroll_margin = window_scroll_margin (w, MARGIN_IN_PIXELS);
> + int cursor_height = MATRIX_ROW (w->desired_matrix,
> w->cursor.vpos)->height;
> if ((w->cursor.y < this_scroll_margin
> && CHARPOS (start) > BEGV)
Same here (in another function).
> diff --git a/src/window.c b/src/window.c
> index dbda435..20a7f3a 100644
> --- a/src/window.c
> +++ b/src/window.c
> @@ -4803,7 +4803,18 @@ window_scroll_margin (struct window *window, enum
> margin_unit unit)
> = (window->total_lines * WINDOW_FRAME_LINE_HEIGHT (window)
> - WINDOW_MODE_LINE_HEIGHT (window))
> / frame_line_height;
> - int margin = min (scroll_margin, window_total_lines / 4);
> +
> + int margin, max_margin;
> + double ratio = 0.25;
> + if (FLOATP (Vmaximum_scroll_margin))
> + {
> + ratio = XFLOAT_DATA (Vmaximum_scroll_margin);
> + ratio = max (0.0, ratio);
> + ratio = min (ratio, 0.5);
> + }
> + max_margin = (int) (window_total_lines * ratio);
> + margin = max (0, scroll_margin);
> + margin = min (scroll_margin, max_margin);
> if (unit == MARGIN_IN_PIXELS)
> return margin * frame_line_height;
> else
> diff --git a/src/xdisp.c b/src/xdisp.c
> index 3602025..b22242a 100644
> --- a/src/xdisp.c
> +++ b/src/xdisp.c
> @@ -31451,6 +31451,11 @@ Recenter the window whenever point gets within this
> many lines
> of the top or bottom of the window. */);
> scroll_margin = 0;
>
> + DEFVAR_LISP ("maximum-scroll-margin", Vmaximum_scroll_margin,
> + doc: /* Maximum effective value of `scroll-margin'.
> +Given as a fraction of the current window's lines. */);
"as a fraction of the current window's height" sounds better, I
think. (It doesn't matter if the height is in lines or in pixels,
for this purpose.)
> + Vmaximum_scroll_margin = make_float (0.25);
> +
We usually call such variables "max-SOMETHING", not
"maximum-SOMETHING".
Also, the actual value is limited by 0.5, but the doc string doesn't
tell that. It also doesn't say that any non-float value is ignored.
Finally, the new variable needs to be documented in the user manual
and in NEWS.
Thanks.