Thanks Alex Schroeder for reporting this crash. The scenario:
Before this commit these steps yielded the following crash:
Error: bad argument #1 to 'len' (string expected, got nil) text.lua:626: in function 'pos_at_end_of_screen_line' edit.lua:298: in function 'mouse_press'
There were two distinct problems here:
How did these issues get missed for years?
Between these 3 patterns, the odds of running into a drawing as the first or bottom-most line on the screen were fairly small. And then I had to interact with it. I suspect I tend to interact with drawings after centering them vertically.
Bug #1 in particular has some interesting past history.
Near the start of the project, when I implemented line-wrapping I
started saving screen_bottom, the bottom-most line displayed on
screen. I did this so I could scroll down easily just by assigning
screen_top = screen_bottom
. (On the other hand, scrolling up still
required some work. I should perhaps get rid of it and just compute
scrolls from scratch each time.)
Also near the start of the project, I supported selecting text by a complex state machine spanning keypress, mouse press and mouse release: mouse click (press and immediate release) moves cursor mouse drag (press and much later release) creates selection shift-click selects from current cursor to click location shift-movement creates/grows a selection
On 2023-06-01, inscript reported a bug. Opening a window with just a little bit of text (lots of unused space in the window), selecting all the text and then clicking below all the text would crash the editor.
To fix this I added code at the bottom of edit.mouse_press which computed the final visible line+pos location and used that in the cursor-move/text-selection state machine. It did this computation based on.. screen_bottom. But I didn't notice that screen_bottom could be a drawing (which has no pos). This commit's bug/regression was created.
On 2023-09-20, Matt Wynne encountered a crash which got me to realize I need code at the bottom of edit.mouse_release symmetric to the code at the bottom of edit.mouse_press. I still didn't notice that screen_bottom could be a drawing.
So in fixing inscript's bug report, I introduced (at least) 2 regressions, because I either had no idea or quickly forgot that screen_bottom could point at a drawing.
While I created regressions, the underlying mental bug feels new. I just never focused on the fact that screen_bottom could point at a drawing.
This past history makes me suspicious of my mouse_press/mouse_release code. I think I'm going to get rid of screen_bottom entirely as a concept. I'll still have to be careful though about the remaining locations and which of them are allowed to point at drawings:
I sometimes copy between these 4 location variables. Auditing shows no gaps where cursor could ever end up pointing at a drawing. It's just when I started using screen_bottom for a whole new purpose (in the mouse_press/release state machine) that I went wrong.
I should also try getting rid of starty entirely. Is it really needed for a responsive editor? I think I introduced it back when I didn't know what I was doing with LÖVE and was profligately creating text objects willy-nilly just to compute widths.
Getting rid of these two fairly global bits of mutable state will hopefully make lines much more robust when the next person tries it out in 6 months :-/ X-(
Thanks everyone for the conversation around this bug: https://merveilles.town/@akkartik/112567862542495637
Bug #2 has some complexity as well, and might lead to some follow-on cleanup.
When I click on the button to insert a new drawing, the mouse_release hook triggers and moves the cursor below the new drawing. This is desirable, but I'd never noticed this happy accident. It stops working when I invalidate starty for all lines (which gets recomputed and cached for all visible lines on every frame).
Fixing this caused a couple of unit tests start crashing for 2 reasons that required their own minor fixes:
My emulated mouse press and release didn't have an intervening frame and so mouse_release no longer receives starty. Now I've added a call to edit.draw() between press and release.
This might actually bite someone for real someday, if they're running on a slow computer or something like that. I've tried to click really fast but I can't seem to put mouse_press and release in the same frame (assuming 30 frames per second)
My tests' window dimensions often violate my constraint that the screen always have one line of text for showing the cursor. They're unrealistically small or have a really wide aspect ratio (width 2x of height). I suspect lines.love will itself crash in those situations, but hopefully they're unrealistic. Hmm, I wonder what would happen if someone maximized in a 16:9 screen, that's almost 2x.. Anyways, I've cleaned a couple of tests up, but might need to fix up others at some point. I'd have to rejigger all my brittle line-wrapping tests if I modify the screen width :-/ X-(
QAMVLUK22RP5RBDTDV5XVPQCSJUWDWESV4TRCUTNUM46E26BH2AQC
656FM555BRGLHJ7PTIZXD2IY5T7PGFEYSHG2T3Z7WNX6QZ5KROSAC
DSLD74DK3P6J2VAFCYF5BGTHZ637QTW3PDHOUHFACDZU66YNM3IAC
LXTTOB33N2HCUZFIUDRQGGBVHK2HODRG4NBLH6RXRQZDCHF27BSAC
CNCYMM6ABOXCRI2IP5A4T2OGBO5FQ7GWBXBP2OQYL4YET5BLJCGQC
6XCJX4DZB6UEAXEXXUGVVPBCF5SNDYOJGU3Q6BPB4ZMI5DZH4MYQC
BULPIBEGL7TMK6CVIE7IS7WGAHGOSUJBGJSFQK542MOWGHP2ADQQC
MXA3RZYKUI4UF2ISY7JEF6VKX6NOPZMZH5SLLCZHRJKFIXXXDPSAC
LF7BWEG4DKQI7NMXMZC4LC2BE5PB42HK5PD6OYBNIDMAZBJASOKQC
2L5MEZV344TOZLVY3432RHJFIRVXFD6O3GWLL5O4CV66BGAFTURQC
TGHAJBESCIEGWUE2D3FGLNOIAYT4D2IRGZKRXRMTUFW7QZETC7OAC
WJBZZQE4A4KLYGS2KA254I6VN2DVXDY4XKCNAE76GTMLLQGYCUOQC
KYNGDE2CKNOKUC2XMAS5MEU6YT2C3IW5SIZLOJE64G3ERT7BSWFAC
5UKUADTWMNWPOPBBTXUXY7UNFW64DWANI2RQHKSCSZNWHTQM4GUAC
7CLGG7J277QZGMNOVFEXBX6DRETCVK7GH66HJ7BYOBMPHTJCDFMAC
K2X6G75Z6XBC4DVIRWC5HC7XA3A2SKOM3MWSQTCFEYWIJL7LME2QC
IDGP4BJZTKAD6ZO4RLAWYVN6IFCMIM76G6HJGPTE27K4D6CDBUHQC
ZTMRQZSWUL6FJRI4C4H37MR2IMV22DB6KRGEOUNYRWW5CTAVQFKAC
FS2ITYYHBLFT66YUC3ENPFYI2HOYHOVEPQIN7NQR6KF5MEK4NKZAC
function Text.final_text_loc_on_screen(State)
if State.lines[State.screen_bottom1.line].mode == 'text' then
return {
line=State.screen_bottom1.line,
pos=Text.pos_at_end_of_screen_line(State, State.screen_bottom1),
}
end
local loc2 = Text.to2(State, State.screen_bottom1)
while true do
if State.lines[loc2.line].mode == 'text' then break end
assert(loc2.line > 1 or loc2.screen_line > 1 and loc2.screen_pos > 1) -- elsewhere we're making sure there's always at least one text line on screen
loc2 = Text.previous_screen_line(State, loc2)
end
local result = Text.to1(State, loc2)
result.pos = Text.pos_at_end_of_screen_line(State, result)
return result
end