From 9501f01ca0a8ddcf54e12584f79833c884fde939 Mon Sep 17 00:00:00 2001 From: "Kartik K. Agaram" Date: Sun, 9 Jun 2024 13:17:55 -0700 Subject: [PATCH] fix a crash involving mouse and drawings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Thanks Alex Schroeder for reporting this crash. The scenario: * Edit a file like say this repo's Readme. * The second line is empty and there's a '+' to insert a drawing. Click on that. * Resize the window so just the first line of text and the drawing are visible. * Close the window. * Reopen lines.love, it will reopen the same file. * Click on the left margin to the left of the drawing. 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: 1. State.screen_bottom1 is not required to point to a text line, it could just as well be a drawing. I have been sloppy in handling that. 2. The bug was partially masked (the need to close and reopen the window) by a second bug: inserting a drawing was not invalidating the cache I save of starty coordinates for each line. (I've inserted and deleted starty invalidations a few times in the past, but it looks like I'd never had one in this particular location edit.draw before.) How did these issues get missed for years? - Even though I use lines.love on a daily basis, it turns out I don't actually create line drawings all that often. - When I do, I'm still living in files that are mostly text with only an occasional drawing. - I keep my windows fairly large. 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: - cursor and selection are not allowed to point at drawings - screen_top and screen_bottom 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-( --- README.md | 5 +++++ drawing_tests.lua | 2 +- edit.lua | 13 +++++++------ text.lua | 19 +++++++++++++++++++ text_tests.lua | 2 +- 5 files changed, 33 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 1a27ac7..690d94c 100644 --- a/README.md +++ b/README.md @@ -72,6 +72,11 @@ found anything amiss: http://akkartik.name/contact * No clipping yet for drawings. In particular, circles/squares/rectangles and point labels can overflow a drawing. +* If you ever see a crash when clicking on the mouse, it might be because a + mouse press and release need to happen in separate frames. Try pressing and + releasing more slowly and let me know if that helps or not. This is klunky, + sorry. + * Touchpads can drag the mouse pointer using a light touch or a heavy click. On Linux, drags using the light touch get interrupted when a key is pressed. You'll have to press down to drag. diff --git a/drawing_tests.lua b/drawing_tests.lua index ede25f0..64188df 100644 --- a/drawing_tests.lua +++ b/drawing_tests.lua @@ -3,7 +3,7 @@ -- of specific shapes. In particular, no tests of freehand drawings. function test_creating_drawing_saves() - App.screen.init{width=120, height=60} + App.screen.init{width=800, height=600} Editor_state = edit.initialize_test_state() Editor_state.filename = 'foo' Editor_state.lines = load_array{} diff --git a/edit.lua b/edit.lua index b096274..42309df 100644 --- a/edit.lua +++ b/edit.lua @@ -66,8 +66,10 @@ function edit.initialize_state(top, left, right, font, font_height, line_height) -- -- Make sure these coordinates are never aliased, so that changing one causes -- action at a distance. + -- + -- On lines that are drawings, pos will be nil. screen_top1 = {line=1, pos=1}, -- position of start of screen line at top of screen - cursor1 = {line=1, pos=1}, -- position of cursor + cursor1 = {line=1, pos=1}, -- position of cursor; must be on a text line screen_bottom1 = {line=1, pos=1}, -- position of start of screen line at bottom of screen selection1 = {}, @@ -185,6 +187,7 @@ function edit.draw(State) Drawing.before = snapshot(State, line_index-1, line_index) table.insert(State.lines, line_index, {mode='drawing', y=y, h=256/2, points={}, shapes={}, pending={}}) table.insert(State.line_cache, line_index, {}) + for _,line_cache in ipairs(State.line_cache) do line_cache.starty = nil end if State.cursor1.line >= line_index then State.cursor1.line = State.cursor1.line+1 end @@ -292,10 +295,7 @@ function edit.mouse_press(State, x,y, mouse_button) State.old_cursor1 = State.cursor1 State.old_selection1 = State.selection1 State.mousepress_shift = App.shift_down() - State.selection1 = { - line=State.screen_bottom1.line, - pos=Text.pos_at_end_of_screen_line(State, State.screen_bottom1), - } + State.selection1 = Text.final_text_loc_on_screen(State) end function edit.mouse_release(State, x,y, mouse_button) @@ -333,7 +333,7 @@ function edit.mouse_release(State, x,y, mouse_button) end -- still here? mouse release is below all screen lines - State.cursor1.line, State.cursor1.pos = State.screen_bottom1.line, Text.pos_at_end_of_screen_line(State, State.screen_bottom1) + State.cursor1 = Text.final_text_loc_on_screen(State) edit.clean_up_mouse_press(State) --? print_and_log(('edit.mouse_release: finally selection %s,%s cursor %d,%d'):format(tostring(State.selection1.line), tostring(State.selection1.pos), State.cursor1.line, State.cursor1.pos)) end @@ -596,6 +596,7 @@ end function edit.run_after_mouse_click(State, x,y, mouse_button) App.fake_mouse_press(x,y, mouse_button) edit.mouse_press(State, x,y, mouse_button) + edit.draw(State) App.fake_mouse_release(x,y, mouse_button) edit.mouse_release(State, x,y, mouse_button) App.screen.contents = {} diff --git a/text.lua b/text.lua index 9c27bde..d9e7653 100644 --- a/text.lua +++ b/text.lua @@ -621,6 +621,7 @@ function Text.pos_at_start_of_screen_line(State, loc1) end function Text.pos_at_end_of_screen_line(State, loc1) + assert(State.lines[loc1.line].mode == 'text') Text.populate_screen_line_starting_pos(State, loc1.line) local line_cache = State.line_cache[loc1.line] local most_recent_final_pos = utf8.len(State.lines[loc1.line].data)+1 @@ -634,6 +635,24 @@ function Text.pos_at_end_of_screen_line(State, loc1) assert(false, ('invalid pos %d'):format(loc1.pos)) end +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 + function Text.cursor_at_final_screen_line(State) Text.populate_screen_line_starting_pos(State, State.cursor1.line) local screen_lines = State.line_cache[State.cursor1.line].screen_line_starting_pos diff --git a/text_tests.lua b/text_tests.lua index b8f89db..44cdafc 100644 --- a/text_tests.lua +++ b/text_tests.lua @@ -16,7 +16,7 @@ function test_initial_state() end function test_click_to_create_drawing() - App.screen.init{width=120, height=60} + App.screen.init{width=800, height=600} Editor_state = edit.initialize_test_state() Editor_state.lines = load_array{} Text.redraw_all(Editor_state)