From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.bugs Subject: bug#5718: scroll-margin in buffer with small line count. Date: Mon, 12 Sep 2016 20:36:14 +0300 Message-ID: <834m5l9g1d.fsf@gnu.org> References: <4B9D1C61.70903@gmail.com> <87mvkjy0l5.fsf@users.sourceforge.net> <83fuqbfhpb.fsf@gnu.org> <87a8ggwcoo.fsf@users.sourceforge.net> <83inv4cc0s.fsf@gnu.org> <87d1ka17dr.fsf@users.sourceforge.net> Reply-To: Eli Zaretskii NNTP-Posting-Host: blaine.gmane.org X-Trace: blaine.gmane.org 1473701846 1131 195.159.176.226 (12 Sep 2016 17:37:26 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Mon, 12 Sep 2016 17:37:26 +0000 (UTC) Cc: ahyatt@gmail.com, 5718@debbugs.gnu.org, gavenkoa@gmail.com To: npostavs@users.sourceforge.net Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Mon Sep 12 19:37:20 2016 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bjVAQ-0007SZ-Uz for geb-bug-gnu-emacs@m.gmane.org; Mon, 12 Sep 2016 19:37:15 +0200 Original-Received: from localhost ([::1]:44363 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bjVAP-0007RD-4C for geb-bug-gnu-emacs@m.gmane.org; Mon, 12 Sep 2016 13:37:13 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:55929) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bjVAI-0007R1-Rw for bug-gnu-emacs@gnu.org; Mon, 12 Sep 2016 13:37:08 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bjVAD-0007I0-QL for bug-gnu-emacs@gnu.org; Mon, 12 Sep 2016 13:37:05 -0400 Original-Received: from debbugs.gnu.org ([208.118.235.43]:60178) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bjVAD-0007Hs-Mw for bug-gnu-emacs@gnu.org; Mon, 12 Sep 2016 13:37:01 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1bjVAD-00011v-Hc for bug-gnu-emacs@gnu.org; Mon, 12 Sep 2016 13:37:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Eli Zaretskii Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Mon, 12 Sep 2016 17:37:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 5718 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: Original-Received: via spool by 5718-submit@debbugs.gnu.org id=B5718.14737018113941 (code B ref 5718); Mon, 12 Sep 2016 17:37:01 +0000 Original-Received: (at 5718) by debbugs.gnu.org; 12 Sep 2016 17:36:51 +0000 Original-Received: from localhost ([127.0.0.1]:57890 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bjVA3-00011V-3D for submit@debbugs.gnu.org; Mon, 12 Sep 2016 13:36:51 -0400 Original-Received: from eggs.gnu.org ([208.118.235.92]:36651) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bjVA1-00011J-9G for 5718@debbugs.gnu.org; Mon, 12 Sep 2016 13:36:49 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bjV9u-0007B3-6J for 5718@debbugs.gnu.org; Mon, 12 Sep 2016 13:36:43 -0400 Original-Received: from fencepost.gnu.org ([2001:4830:134:3::e]:40401) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bjV9n-00077l-Kt; Mon, 12 Sep 2016 13:36:35 -0400 Original-Received: from 84.94.185.246.cable.012.net.il ([84.94.185.246]:1902 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_128_CBC_SHA1:128) (Exim 4.82) (envelope-from ) id 1bjV9h-00074M-5t; Mon, 12 Sep 2016 13:36:34 -0400 In-reply-to: <87d1ka17dr.fsf@users.sourceforge.net> (npostavs@users.sourceforge.net) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 208.118.235.43 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.org gmane.emacs.bugs:123227 Archived-At: > 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.