bug-gnu-emacs
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

bug#56682: feature/improved-locked-narrowing 9dee6df39c: Reworked locked


From: Gregory Heytings
Subject: bug#56682: feature/improved-locked-narrowing 9dee6df39c: Reworked locked narrowing.
Date: Sat, 01 Apr 2023 14:26:50 +0000


Thanks for your review!


They also change the code in non-trivial ways, and I'm not thrilled about changing code in those parts at this late stage.


Two commits do change the code, yes, to fix (cursor motion command with C-n / C-p) bugs. That code is only used for buffers with long lines. It would be most regrettable to release Emacs 29 without these bugs fixed.


Specific comments:

afc2c6c13c:

. Misses an optimization opportunity: where pos == BEGV, you can use BEGV_BYTE instead of calling CHAR_TO_BYTE.


Okay, thanks, added in dce08cf05c.


. I'm not sure I'm happy about calling find_newline in a loop where previous code just made a trivial calculation. Imagine a buffer with a lot of short lines. What problem exactly is being solved here, and how does this change solve it?


The point is to find the buffer position of BOL. But you're right, there is a missed optimization here, which I just added (also in dce08cf05c). Now the code searches for that position in [pos-500..pos], [pos-5500..pos-500], [pos-55500..pos-5500], [pos-555500..pos-55500], in that order, and buffers with lots of short lines (or more precisely: buffers with lots of short lines _and_ one or more long lines) are not negatively affected by that code anymore.


2093e010dc:

. Why such a strange method of finding out whether we are on a TTY frame? The usual method is FRAME_WINDOW_P (XFRAME (w->frame)).


That's what I've been using since that function was introduced six months ago or so. I admit I don't remember why that's what I chose. If you tell me that using FRAME_WINDOW_P (XFRAME (w->frame)) has the same effect as terminal-live-p == t (and indeed after looking at the code ISTM that that's the case), I'll replace that.


. The continuation glyph can be present not only on TTY frames, but also on GUI frames when one or both of the fringes are disabled, so the test needs to be augmented. I think you need to use WINDOW_LEFT_FRINGE_WIDTH and WINDOW_RIGHT_FRINGE_WIDTH.


Thanks, I did not realize that. I just did that (again in dce08cf05c), but I'm not sure how WINDOW_LEFT_FRINGE_WIDTH should be used. Using WINDOW_RIGHT_FRINGE_WIDTH seems enough, but I'm probably missing something.






reply via email to

[Prev in Thread] Current Thread [Next in Thread]