From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Ihor Radchenko Newsgroups: gmane.emacs.bugs Subject: bug#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update) Date: Fri, 14 Jul 2023 13:48:43 +0000 Message-ID: <87351qioxw.fsf@localhost> References: <877cr4nez9.fsf@localhost> <83v8eo3pfv.fsf@gnu.org> <87bkgeiuap.fsf@localhost> <83fs5qfznv.fsf@gnu.org> Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="18454"; mail-complaints-to="usenet@ciao.gmane.io" Cc: monnier@iro.umontreal.ca, 64596@debbugs.gnu.org To: Eli Zaretskii Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Fri Jul 14 15:49:28 2023 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1qKJAm-0004Zc-BM for geb-bug-gnu-emacs@m.gmane-mx.org; Fri, 14 Jul 2023 15:49:28 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1qKJAO-00005F-DM; Fri, 14 Jul 2023 09:49:04 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qKJAM-00004q-Ge for bug-gnu-emacs@gnu.org; Fri, 14 Jul 2023 09:49:02 -0400 Original-Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1qKJAM-00028j-7u for bug-gnu-emacs@gnu.org; Fri, 14 Jul 2023 09:49:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1qKJAL-0001SX-Ow for bug-gnu-emacs@gnu.org; Fri, 14 Jul 2023 09:49:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Ihor Radchenko Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Fri, 14 Jul 2023 13:49:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 64596 X-GNU-PR-Package: emacs Original-Received: via spool by 64596-submit@debbugs.gnu.org id=B64596.16893425275578 (code B ref 64596); Fri, 14 Jul 2023 13:49:01 +0000 Original-Received: (at 64596) by debbugs.gnu.org; 14 Jul 2023 13:48:47 +0000 Original-Received: from localhost ([127.0.0.1]:41739 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qKJA6-0001Ru-Dh for submit@debbugs.gnu.org; Fri, 14 Jul 2023 09:48:46 -0400 Original-Received: from mout01.posteo.de ([185.67.36.65]:34397) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qKJA3-0001RZ-Sb for 64596@debbugs.gnu.org; Fri, 14 Jul 2023 09:48:45 -0400 Original-Received: from submission (posteo.de [185.67.36.169]) by mout01.posteo.de (Postfix) with ESMTPS id 9BBAD240028 for <64596@debbugs.gnu.org>; Fri, 14 Jul 2023 15:48:37 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.net; s=2017; t=1689342517; bh=/QXU+HukibJSAEYIWAQxNhcC6nF2Fz8F2IgMzE318t8=; h=From:To:Cc:Subject:Date:Message-ID:MIME-Version:From; b=LbtfwjtjG9eItxgXTY1YycBi+V4OLXKpwv9dp0aFtDoHtA/k/rnG7jLula5Uxe5sU EJd9ZS78bD1SUxZM0OzF1iDht+x8XDoT2lI/ny/QwMysSFP2pcexsEGha475rx9NFh J82y6amIJkXkzwSpZhIvfXEDmGlPL8FkhQWgRxc3b2lvrbQtTAS9ULUgy7RGGJY9dt aeie8CIfm1dP2SP9YJize5yAT1R5ACKYKb05JAZEzG0EBcAAVai3/Dqu7tkcUIZ7jC c2qpWx5iLG8jeSje11hrgDMyj1POqdiY4VSqSLwz2pg0HkmAew0tu2tnKIwpe/ZjZ6 5JfbI7nLmIwqg== Original-Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4R2Xr43H5Cz6twZ; Fri, 14 Jul 2023 15:48:36 +0200 (CEST) In-Reply-To: <83fs5qfznv.fsf@gnu.org> X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list 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-mx.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:265091 Archived-At: Eli Zaretskii writes: >> I'm afraid that the very existence of prevent_redisplay_optimizations_p >> flag is a mistake hiding bugs with redisplay optimizations logic. > > I think you should avoid making such comments until you have a better > understanding of the redisplay logic, or else I will stop taking your > posts in this matter seriously. Your answer indicates that I missed something important after reading the top comment in xdisp.c, redisplay_internal, redisplay_window, and skimmed through a couple of other functions. But I still cannot figure out what. Note that I did not mean that previous committers had bad intentions. I just think that the idea of having "disable all optimizations" flag is causing the code quality that could be otherwise somewhat better (at least, more explicit about why redisplay should take long path). I think that replacing all the instances of setting this flag to more explicit alternatives would improve the display performance and also readability. >> Currently, redisplay_internal has a number of conditions used to >> determine if one or another optimization is safe to use + assertion that >> !prevent_redisplay_optimizations_p. >> >> When some code outside xdisp.c sets this flag, it is nothing but a >> statement: "I was lazy enough to update xdisp.c properly, so I just >> force bypassing optimizations". > > No. The optimization in question, which is disabled when > prevent_redisplay_optimizations_p flag is set for the current buffer, > is described in the comment before the condition: > > /* Optimize the case that only the line containing the cursor in the > selected window has changed. > > The prevent_redisplay_optimizations_p flag says, among other things, > that we are not in that case, and it is tested early enough in order > to prevent us from examining additional conditions which are just > waste of CPU cycles (and some of them are relatively expensive). Sure, but it is a very generic way to say this. It provides no detailed reason what exactly changed in the buffer/window/frame that makes us bypass this optimization. Is it not always possible to use the other, more specific, existing flags for redisplay to indicate that something important has been changed? >> So, I see not why calling bset_update_mode_line is not sufficient to >> force mode line update in all windows associated with a single buffer. > > The mode line displays quite a lot of information. It could also > display information we don't know about (on the level of the redisplay > code), because mode-line-format supports :eval, which can execute any > Lisp. Therefore, there could be changes to the buffer that affect the > mode line indirectly. But isn't :eval processed by display_mode_lines? Once we take care to call bset_update_mode_line, display_mode_lines is guaranteed to be executed, AFAIU. > One problem of this kind which we had relatively recently is when the > changes in mode-line-format or some of its :eval forms yields a mode > line that is significantly taller or smaller than the previously > displayed one. Such changes in the mode-line height generally affect > the window(s) as well, not just the mode line itself. Sure, and redisplay_window accounts for this when calling display_mode_lines: display_mode_lines (w); unbind_to (count1, Qnil); /* If mode line height has changed, arrange for a thorough immediate redisplay using the correct mode line height. */ if (window_wants_mode_line (w) && CURRENT_MODE_LINE_HEIGHT (w) != DESIRED_MODE_LINE_HEIGHT (w)) { f->fonts_changed = true; w->mode_line_height = -1; MATRIX_MODE_LINE_ROW (w->current_matrix)->height = DESIRED_MODE_LINE_HEIGHT (w); } > Moreover, you will see in the wild that force-mode-line-update is used > not just to update the mode line, but also to force a more thorough > redisplay of one or more windows. Why not `force-window-update'? > Thus, this is not as simple a problem as you seem to think, and we > need deeper analysis and more significant changes than simply deleting > the code that you didn't understand and think is redundant. Please > keep in mind that people who wrote that code (and I don't mean myself) > were not stupid at all, and generally knew what they were doing! I understand, which is why I looked into git history and found that the code in question is carried over during refactoring. Into a new function with different meaning. And I now looked deeper into the code, and see no obvious downsides of removing current_buffer->prevent_redisplay_optimizations_p = true; from force-mode-line-update specifically. `set-buffer-modified-p' may need to be re-considered though as it is the original place where setting prevent_redisplay_optimizations_p was done (for different reasons). >> > The purpose of force-mode-line-update is to do what its name says, >> ... >> Then, why do we need to call Fforce_mode_line_update in >> set-buffer-modified-p? Wouldn't calling bset_redisplay be better? > > You have read the large comment there which attempts to answer this > question, didn't you? Yup, I did. I thought bset_redisplay as an alternative, because it is smarter than bset_update_mode_line when the buffer is displayed in selected window (it is not necessary to assign DISPLAY_SOME flags). However, I did miss the docstring that explicitly says that `set-buffer-modified-p' forces mode line update. So, forcing mode line update here is simply following the docstring and should not be disputed. -- Ihor Radchenko // yantar92, Org mode contributor, Learn more about Org mode at . Support Org development at , or support my work at