Package: emacs;
Reported by: Oleksandr Gavenko <gavenkoa <at> gmail.com>
Date: Sun, 14 Mar 2010 17:28:02 UTC
Severity: wishlist
Tags: fixed, patch
Fixed in version 26.1
Done: npostavs <at> users.sourceforge.net
Bug is archived. No further changes may be made.
Message #46 received at 5718 <at> debbugs.gnu.org (full text, mbox):
From: npostavs <at> users.sourceforge.net To: Eli Zaretskii <eliz <at> gnu.org> Cc: ahyatt <at> gmail.com, 5718 <at> debbugs.gnu.org, gavenkoa <at> gmail.com Subject: Re: bug#5718: scroll-margin in buffer with small line count. Date: Tue, 13 Sep 2016 22:40:29 -0400
Eli Zaretskii <eliz <at> gnu.org> writes:
>> 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.
>
> 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?
I'm not sure what you mean by "succeed" here. `next-line',
`previous-line', and `scroll-up-command' are able to keep point ouside
of the margin (thus keeping it centered); `scroll-down-command' leaves
it one line down from where it should be.
Actually, the above applies to windows with an odd number of lines, I
realized I didn't account for the case of an even number of lines, which
currently has 0 lines that are outside the margin. I need to change the
cap on `max-scroll-margin' to account for this.
>
> 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.
I hadn't; trying it out now, it seems to cause `next-line' to also have
the bad behaviour of `scroll-down-command' where point is one line too
far down.
>
> 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?
Yes, this seems to work:
@@ -16173,7 +16173,7 @@ redisplay_window (Lisp_Object window, bool just_this_one_p)
int centering_position = -1;
bool last_line_misfit = false;
ptrdiff_t beg_unchanged, end_unchanged;
- int frame_line_height;
+ int frame_line_height, margin;
bool use_desired_matrix;
void *itdata = NULL;
@@ -16203,6 +16203,8 @@ redisplay_window (Lisp_Object window, bool just_this_one_p)
restart:
reconsider_clip_changes (w);
frame_line_height = default_line_pixel_height (w);
+ margin = window_scroll_margin (w, MARGIN_IN_LINES);
+
/* Has the mode line to be updated? */
update_mode_line = (w->update_mode_line
@@ -16507,7 +16509,6 @@ 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 margin = window_scroll_margin (w, MARGIN_IN_LINES);
int pixel_margin = margin * frame_line_height;
bool header_line = WINDOW_WANTS_HEADER_LINE_P (w);
@@ -16792,7 +16793,6 @@ redisplay_window (Lisp_Object window, bool just_this_one_p)
it.current_y = it.last_visible_y;
if (centering_position < 0)
{
- int margin = window_scroll_margin (w, MARGIN_IN_LINES);
ptrdiff_t margin_pos = CHARPOS (startp);
Lisp_Object aggressive;
bool scrolling_up;
@@ -17036,7 +17036,6 @@ 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 = window_scroll_margin (w, MARGIN_IN_LINES);
bool move_down = w->cursor.vpos >= window_total_lines / 2;
move_it_by_lines (&it, move_down ? margin + 1 : -(margin + 1));
>
>
>> @@ -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).
These are 2 different functions: try_window and try_window_id.
>> + 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.)
Makes sense.
>
>> + 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.
Okay.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.