From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.ciao.gmane.io!not-for-mail From: martin rudalics Newsgroups: gmane.emacs.bugs Subject: bug#38181: Actual height of mode-line not taken into account Date: Thu, 7 May 2020 10:34:53 +0200 Message-ID: <6713049b-49e2-64ca-902d-1f972952f590@gmx.at> References: <87eeyd3ul0.fsf@bernoul.li> <83d0dt2qt6.fsf@gnu.org> <83r2290w24.fsf@gnu.org> <83pnhs6wwp.fsf@gnu.org> <83k1806qca.fsf@gnu.org> <8336en7giv.fsf@gnu.org> <81264049-4f88-fae7-6448-e0ac5d977268@gmx.at> <83a78u5s8y.fsf@gnu.org> <01cee2f7-aeb5-4eb1-b2d5-e056c91eab8b@gmx.at> <83wo5rolsc.fsf@gnu.org> <4682372c-e25a-dc62-842f-c3971f79bb16@gmx.at> <838si6mnsy.fsf@gnu.org> <86491f07-ed68-2880-17a0-a60ed40d5059@gmx.at> <83ftcdktrm.fsf@gnu.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Injection-Info: ciao.gmane.io; posting-host="ciao.gmane.io:159.69.161.202"; logging-data="108967"; mail-complaints-to="usenet@ciao.gmane.io" Cc: jonas@bernoul.li, 38181@debbugs.gnu.org To: Eli Zaretskii Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Thu May 07 10:37:45 2020 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 1jWc2K-000SCn-WF for geb-bug-gnu-emacs@m.gmane-mx.org; Thu, 07 May 2020 10:37:45 +0200 Original-Received: from localhost ([::1]:43450 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jWc2J-00069p-EY for geb-bug-gnu-emacs@m.gmane-mx.org; Thu, 07 May 2020 04:37:43 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:50260) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1jWc0g-00054q-Gu for bug-gnu-emacs@gnu.org; Thu, 07 May 2020 04:36:03 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:58719) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1jWc0g-0004xq-6x for bug-gnu-emacs@gnu.org; Thu, 07 May 2020 04:36:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1jWc0g-0002lA-3s for bug-gnu-emacs@gnu.org; Thu, 07 May 2020 04:36:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: martin rudalics Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Thu, 07 May 2020 08:36:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 38181 X-GNU-PR-Package: emacs Original-Received: via spool by 38181-submit@debbugs.gnu.org id=B38181.158884050310486 (code B ref 38181); Thu, 07 May 2020 08:36:02 +0000 Original-Received: (at 38181) by debbugs.gnu.org; 7 May 2020 08:35:03 +0000 Original-Received: from localhost ([127.0.0.1]:42031 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1jWbzi-0002iq-Tt for submit@debbugs.gnu.org; Thu, 07 May 2020 04:35:03 -0400 Original-Received: from mout.gmx.net ([212.227.15.15]:41295) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1jWbzg-0002iA-5L for 38181@debbugs.gnu.org; Thu, 07 May 2020 04:35:00 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=gmx.net; s=badeba3b8450; t=1588840493; bh=MOTGxhTUx6U+IZdtGEghtMpIbRH258bseDvprT776Jw=; h=X-UI-Sender-Class:Subject:To:Cc:References:From:Date:In-Reply-To; b=c/Wago0Jwj6G8P+4L0huyKO10M42Nwko17fde/mlhgWPhfm5fXysHFw5lVsa1U/Hn c0zeRau2yylG/+hcKdvCpqzDnpE5r3T7QY6olI9FqTGHZvtcHhpMfvxWWSTGTY4NXR L2Y8SQnxHLL36tD4g2sHOCAXMUDgkRqy4SBPVGx4= X-UI-Sender-Class: 01bb95c1-4bf8-414a-932a-4f6e2808ef9c Original-Received: from [192.168.1.101] ([212.95.5.89]) by mail.gmx.com (mrgmx005 [212.227.17.190]) with ESMTPSA (Nemesis) id 1MmDIo-1ioVnZ2lzk-00iEiW; Thu, 07 May 2020 10:34:53 +0200 In-Reply-To: <83ftcdktrm.fsf@gnu.org> Content-Language: en-US X-Provags-ID: V03:K1:iHiC1EofanmQ1vTFXcd+B5yW4MX5PsgWHtf+K1gruBs92YEp99f Vx3Gr6VNPW3MG6qXPZZgO1RZQzaWCq4mrtVvoN8I7NDsR1ilOHpTyzKAi+J1zE4o/tzzwIX cyrOPoSxpVCGGs6WvkXemaAZg531+3xJXP6xADxy/rAQiN+mpRBk4ONE8cWF/WgEHKUfJoz eE7zrukJWttD/irrO9NoA== X-UI-Out-Filterresults: notjunk:1;V03:K0:02eakfRHTgQ=:gsAUCbJF1QmjCPTCJpgRKr UbD4mHFSZyrknKLtj8MJxCDIhAl4lm32uPrw/Fh+TYPiA9EhZ68vxGDQ0zlKgBGOoMRye+G6n agqHehy7Iuwy0e91mZZKQTqJXkF/KmIRbF4ubWqlYR5rr4p6Bi+c5VO7g9wfcfk5335cxVriY Dx9agLvjGktwZc0RzfxdFV5Hbu+V78jV7H/D/bjYlcvPx7U3dbUzMhioIKLXmwiLXNUUMLobL QAK3WZIWngxqYIJ41m1C8plk3VfR0+pR9/Ds1yjKoYGHyIqDZeL7pf7HL7aQCa3KnZC2TTjHQ L1/Gy/GOjH1dJKu6kMDkmG4O2yBrJ/U4aDsqYNHUzjjYLIp+iQuJv6b/l53C6+XOiDEIPNqin ez3mIEXlwL3pLmSsA7V42l+ZFSZ454/ovMtVKOGsSzI5s+xwttmROagalyEvRkOl1WoJlSLVH STZkT6jZxzvxiNtBE/rw9RVE1CTiks28v63PYKbkDOfGVuMpui/IiRU5Rfxdv1PvuEKX/giwt Jxwe7f9cfJyvK98mIyF3AQO1XAtWRuAtbmTkr8Td1GHJ1LBvW7WjsBXskayQUZ8uXwkxVBJb5 HJNgNqniqF7yS0ioRJD7GvZ9s5+pBmOac363RfyOx1FK/pQCbAdfKaeI/VliJF+6OkT+pwYzP wOYwZhs3X2nBdCkVc9ZikOWTtHra6tVxL7NSbhYJ0cS31/a2cy1XTRXbYIiJdOxVz0CmUuEGP q+C7Vum1xZ4OMD6LWN5tDaBXlys8OKcX6PyJ4kT88Vyy8IWtV9Iq9K7heROG7mYmFBo5z9WF 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" Xref: news.gmane.io gmane.emacs.bugs:179867 Archived-At: >> With Bug#40639 and Bug#41071 the situation is simple: The internal >> border changed face but when we enter init_iterator >> inhibit_free_realized_faces is true and we don't handle it. > > Can you show the backtrace from this call to init_iterator? I run emacs -Q and evaluate there the following form (progn (set-face-background 'internal-border "red") (select-window (display-buffer-in-child-frame (get-buffer-create "*scratch*") '((child-frame-parameters . ((left . 100) (top . 100) (height . 10) (width . 20) (minibuffer . nil) (internal-border-width . 50))))))) Now _if I remove the following two lines_ if ((IT)->glyph_row != NULL) \ inhibit_free_realized_faces = true; \ from PRODUCE_GLYPHS, evaluating the form above does pick up the face triggered by XFRAME (w->frame)->face_change true as #0 0x0000000000463cbf in init_iterator (it=0x7fffffff8c60, w=0x193e060, charpos=1, bytepos=1, row=0x1947f90, base_face_id=DEFAULT_FACE_ID) at ../../src/xdisp.c:2975 #1 0x00000000004652e8 in start_display (it=0x7fffffff8c60, w=0x193e060, pos=...) at ../../src/xdisp.c:3298 #2 0x0000000000497468 in try_window (window=XIL(0x193e065), pos=..., flags=1) at ../../src/xdisp.c:19120 #3 0x00000000004941e9 in redisplay_window (window=XIL(0x193e065), just_this_one_p=false) at ../../src/xdisp.c:18544 #4 0x000000000048be75 in redisplay_window_0 (window=XIL(0x193e065)) at ../../src/xdisp.c:16258 #5 0x00000000007af751 in internal_condition_case_1 (bfun=0x48be33 , arg=XIL(0x193e065), handlers=XIL(0x7ffff3e98ee3), hfun=0x48bdfb ) at ../../src/eval.c:1379 #6 0x000000000048bdcd in redisplay_windows (window=XIL(0x193e065)) at ../../src/xdisp.c:16238 #7 0x000000000048a7fb in redisplay_internal () at ../../src/xdisp.c:15706 #8 0x00000000004883f9 in redisplay () at ../../src/xdisp.c:14933 #9 0x000000000064edbd in read_char (commandflag=1, map=XIL(0x17e4f93), prev_event=XIL(0), used_mouse_menu=0x7fffffffe0ff, end_time=0x0) at ../../src/keyboard.c:2493 #10 0x0000000000661ce5 in read_key_sequence (keybuf=0x7fffffffe290, prompt=XIL(0), dont_downcase_last=false, can_return_switch_frame=true, fix_current_buffer=true, prevent_redisplay=false) at ../../src/keyboard.c:9553 #11 0x000000000064b283 in command_loop_1 () at ../../src/keyboard.c:1350 #12 0x00000000007af676 in internal_condition_case (bfun=0x64ae07 , handlers=XIL(0x90), hfun=0x64a416 ) at ../../src/eval.c:1355 #13 0x000000000064a9ec in command_loop_2 (ignore=XIL(0)) at ../../src/keyboard.c:1091 #14 0x00000000007aeb2a in internal_catch (tag=XIL(0xd110), func=0x64a9bf , arg=XIL(0)) at ../../src/eval.c:1116 #15 0x000000000064a98a in command_loop () at ../../src/keyboard.c:1070 #16 0x0000000000649efd in recursive_edit_1 () at ../../src/keyboard.c:714 #17 0x000000000064a0f5 in Frecursive_edit () at ../../src/keyboard.c:786 #18 0x00000000006404fe in main (argc=4, argv=0x7fffffffe788) at ../../src/emacs.c:2054 Lisp Backtrace: "redisplay_internal (C function)" (0x0) (gdb) c With PRODUCE_GLYPHS unaltered, that breakpoint does _not_ trigger. Only if I set frame titles for child frames (as I do on master now) I can get a backtrace like the below #0 0x00000000004821dc in init_iterator (it=0x7fffffffb4a0, w=0x1a4b800, charpos=-1, bytepos=-1, row=0x0, base_face_id=DEFAULT_FACE_ID) at ../../src/xdisp.c:2988 #1 0x00000000004a575b in gui_consider_frame_title (frame=XIL(0x1a5e8f5)) at ../../src/xdisp.c:12447 #2 0x00000000004a5c07 in prepare_menu_bars () at ../../src/xdisp.c:12544 #3 0x00000000004aaeaf in redisplay_internal () at ../../src/xdisp.c:15392 #4 0x00000000004a9aeb in redisplay () at ../../src/xdisp.c:14977 #5 0x0000000000768eb6 in read_char (commandflag=1, map=XIL(0x1c5c3d3), prev_event=XIL(0), used_mouse_menu=0x7fffffffe10f, end_time=0x0) at ../../src/keyboard.c:2493 #6 0x00000000007800e3 in read_key_sequence (keybuf=0x7fffffffe2a0, prompt=XIL(0), dont_downcase_last=false, can_return_switch_frame=true, fix_current_buffer=true, prevent_redisplay=false) at ../../src/keyboard.c:9547 #7 0x000000000076549a in command_loop_1 () at ../../src/keyboard.c:1350 #8 0x000000000084133d in internal_condition_case (bfun=0x76501e , handlers=XIL(0x90), hfun=0x76462d ) at ../../src/eval.c:1355 #9 0x0000000000764c03 in command_loop_2 (ignore=XIL(0)) at ../../src/keyboard.c:1091 #10 0x00000000008407f1 in internal_catch (tag=XIL(0xd200), func=0x764bd6 , arg=XIL(0)) at ../../src/eval.c:1116 #11 0x0000000000764ba1 in command_loop () at ../../src/keyboard.c:1070 #12 0x0000000000764114 in recursive_edit_1 () at ../../src/keyboard.c:714 #13 0x000000000076430c in Frecursive_edit () at ../../src/keyboard.c:786 #14 0x000000000076048f in main (argc=3, argv=0x7fffffffe798) at ../../src/emacs.c:2035 which also picks up the change of the internal border's background. With an unaltered release I can't get init_iterator to notice the face change without, in some clumsy way, switching to that child frame with the mouse or something the like. Disclaimer: In all those runs I do not know whether the (set-face-background 'internal-border "red") set the face_change flag or something else did. >> if (face_change) >> { >> face_change = false; >> XFRAME (w->frame)->face_change = 0; >> free_all_realized_faces (Qnil); >> } >> else >> { >> XFRAME (w->frame)->face_change = 0; >> free_all_realized_faces (w->frame); >> } > > You don't really need to free the faces everywhere here, only for the > window W's frame, right? Right. > I think we shouldn't throw away face caches > of other frames in this situation, it's too radical. Agreed. > I think the only situation where it can hurt is when this code is > called while redisplay_internal runs. IOW, we need to test the value > of redisplaying_p. OK. >> At the very least I probably should set windows_or_buffers_changed. >> There is this comment in redisplay_internal >> >> /* If face_change, init_iterator will free all realized faces, which >> includes the faces referenced from current matrices. So, we >> can't reuse current matrices in this case. */ >> if (face_change) >> windows_or_buffers_changed = 47; >> >> which hints at that (without considering that f->face_change is not >> handled there). > > The call to free_all_realized_faces invalidates the current matrices > of all the windows on the frame, so the stale face IDs will not be > used after that on that frame, and therefore nothing else needs to be > done in that case. If _all_ the face caches are discarded on all > frames, we need windows_or_buffers_changed to force redisplay_internal > consider more than just the selected frame. So if you modify your > code to free faces only on the frame of the window for which you > compute the mode-line height, the problem with > windows_or_buffers_changed will disappear. OK. >> Nevertheless, the fact that inhibit_free_realized_faces is true when >> entering the iterator after a face change is IMO a bug. We apparently >> always run the iterator with the old faces first. Only after we have >> (incidentally?) detected some change that forces us to "retry", we have >> redisplay set inhibit_free_realized_faces to false itself and the face >> change will get applied in the next iteration. If so, the outcome of >> the previous iterations gets lost if a face changed there. Does such a >> strategy give us any gains? > > I don't think I follow. redisplay_internal resets the > inhibit_free_realized_faces flag to zero near its beginning. It is > true that we also reset it when we retry, but the first try doesn't > bypass this resetting. Am I missing something? It might have been a wrong conclusion on my side. Maybe the problem is caused by the fact that face_change is not set by 'set-face-background' and the face change that triggered the backtraces above came from somewhere else. >> Again, the question I tried to ask earlier was: Does a current matrix in >> between two redisplays rely on the old realized faces? > > Of course it does. The glyph matrices don't hold any face > information, they only hold the ID of each face, and the ID is just > the index of the face's cache slot. If the face cache is thrown away, > we will not be able to use the current matrix. So if I set `inhibit-free-realized-faces' to nil at some arbitrary time things may go wrong. > The current matrix is used for the following purposes: > > . outside of the redisplay cycle, for redisplaying portions of > windows when we get expose events -- in this case we simply write > to the glass the relevant portions of the matrix, without > reproducing it > > . during a redisplay cycle, for decisions about redisplay > optimizations > > . at the end of a redisplay cycle, for comparison with the desired > matrix, which is needed for figuring out what to write to the > glass > > For the first purpose, we have the following defenses: > > . expose_frame: > > if (FRAME_FACE_CACHE (f) == NULL > || FRAME_FACE_CACHE (f)->used < BASIC_FACE_ID_SENTINEL) > { > redisplay_trace ("expose_frame no faces\n"); > return; > } > > . expose_window (called by expose_frame): > > if (w->current_matrix == NULL) > return false; > > So this case is covered, i.e. you can call ... from Elisp, at any time, I presume ... > free_all_realized_faces, > and the next expose event will be handled correctly. > The other two are the reason why we set the > inhibit_free_realized_faces in PRODUCE_GLYPHS: the flag must be set > during redisplay, until update_frame finished its job. Otherwise we > will sooner or later crash. I'm running without this for a couple of days now and it did not crash so far. Sheer luck, I presume. Couldn't PRODUCE_GLYPHS set inhibit_free_realized_faces iff redisplaying_p is true? > So I think each function that might need to notice face changes as > soon as they happened, should be able to reset > inhibit_free_realized_faces, provided that (a) it makes sure > redisplaying_p is zero, and (b) it only does that if necessary and for > a single frame. The latter part is because clearing the face cache > will cause all the move_it_* functions to work harder, because they > will have to recompute all the faces. So it's not so entirely trivial to do that in Elisp. >> If so, the answer is that inhibit_free_realized_faces should be >> always true but for the small window within retrying in >> redisplay_internal. > > I don't think I agree, but maybe I'm missing something. OK. But IIUC so far we do not allow inhibit_free_realized_faces nil outside of redisplay_internal. Unless someone sets 'inhibit-free-realized-faces' which is, in general, to recommended. Right? >> In either case, it strikes me as a strange idea that redisplay_internal >> saves and restores the value of a variable which is apparently always >> true when it is entered (I suppose redisplay_internal is not re-entrant >> given >> >> /* I don't think this happens but let's be paranoid. */ >> if (redisplaying_p) >> return; > > redisplay_internal is non-reentrant, but I see no harm in restoring > the value on exit. If someone sets it to nil now, observing the precautions you gave above, things change radically. We may restore the value to nil, which was hardly the case before. martin