I'm not sure this is very useful. I had an initial idea to stop using
screen_bottom1 in final_text_loc_on_screen, by starting from screen_top1
rather than screen_bottom1. But that changes the direction in which we
scan for the text line in situations where there is somehow no text on
screen (something that should never happen but I have zero confidence in
that).
Still, it doesn't seem like a bad thing to drastically reduce the
lifetime of some derived state.
Really what I need to do is throw this whole UX out and allow the cursor
to be on a drawing as a whole. So up arrow or left arrow below a drawing
would focus the whole drawing in a red border, and another up arrow and
left arrow would skip the drawing and continue upward. I think that
change to the UX will eliminate a whole class of special cases in the
code.
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-(
scenario:
- create a long wrapping line
- tap past end of first screen line
Before this commit the cursor would be positioned not quite at the end
of the screen line but one character before. In effect there was no way
to position cursor at end of a wrapping line.
I'm not sure how this bug has lasted so long. It was introduced in
commit 8d3adfa36 back in June 2022, which was itself billed as a bugfix
for "clicking past end of screen line". But when I go back to it this
bug exists even back then. How did I miss it?! I wrote a test back then
-- and the test was wrong, has always been wrong.
- source editor always expects relative paths
- refresh mocked data
There's still one issue after this: the font size saved in the config
file is the one we use in tests. More broadly, Editor_state is
completely wrong.
Ideally I'd just not save any settings for the source editor if the
tests fail.
Scenario:
modify a test to fail in the source editor
delete any settings in the 'config' file in the save dir
start lines.love
press C-e to switch to source editor
Before this commit, this scenario led to the following events:
the C-e keypress invokes App.run_tests_and_initialize()
the failing test results in a call to error()
the call to error() is trapped by the xpcall around the event handler in love.run
handle_error runs
Current_app is 'source', so love.event.quit() is triggered
love.quit() is invoked
source.settings() is invoked
App.screen.position() is invoked, which calls the test mock
Since App.screen.move was never invoked, App.screen.position() returns nil
The 'config' file is written without values for source.x and source.y
As a result, future runs fail to open.
This is likely a corner case only I will ever run into, since I'm
careful to never commit failing unit tests. Still, I spent some time
trying to figure out the best place to fix this. Options:
* don't write config if Error_message is set
but we do want config written in this scenario:
* we hit an error, source editor opens
* we spend some time debugging and don't immediately fix the issue
* we quit, with some new files opened in various places
* hardcode source.settings() to call love.window.getPosition() rather
than App.screen.position().
drawback: weird special case
* clean up test mocks before aborting
this seems like something we always want
I'm not very sure of my choice.
This bug doesn't leave me feeling very great about my whole app.
Arguably everything I've done is bullshit hacks piled on hacks.
Perhaps the issue is:
- naked error() in LÖVE apps never invokes love.quit(), but
- an unhandled error within my handle_error invokes love.quit() (via
love.event.quit)
Perhaps LÖVE should provide a way to abort without invoking the quit
handler. There's literally no other way in LÖVE to request a quit.
scenario: run without config file, quit, run again
expected: font size remains the same on second run
Before this commit it was increasing on each run.
It turns out the font height that you pass into love.graphics.newFont()
is not the result of font:getHeight().
Renamed from the 'error' state.
Now we no longer overload Error_message; it's only used for actual
errors that trigger opening the source editor.
I was tempted to hide Skip_rest_of_key_events inside the 'warning' state
as well, but that isn't right. It applies to all Current_app
transitions, not just those in and out of 'warning'.
I'm starting to feel better after replacing 1 line with 20 and 2 new
bits of global state. I'm now handling two scenarios more explicitly:
* If I change Current_app within key_press, the corresponding text_input
and key_release events go to the new app. If it's an editor it might
insert the key, which is undesirable. Putting such handlers in
key_release now feels overly clever, particularly since it took me
forever to realize why I was getting stuck in an infinite loop.
* Both 'run' and 'source' can hit the version check, so we need to be
able to transition from the 'error' app to either. Which
necessitates yet another global bit of state: Next_app.
When I stopped running the version check before the tests I also stopped
initializing Version, which can be used in tests to watch out for font
changes across versions. As a result I started seeing a test failure
with LÖVE v12.
It looks like all manual tests pass now. And we're also printing the
warning about version checks before running tests, which can come in
handy if a new version ever causes test failures. The only thing that
makes me unhappy is the fact that we're calling the version check twice.
And oh, the fact that this part around initialization and version
management is clearly still immature.
I'll capture some desires and fragmentary thought processes around them:
* If there's an error, go to the source editor.
* But oh, don't go to source editor on some unactionable errors, so we
include a new `Current_app` mode for them:
* Unsupported version requires an expert. Just muddle through if you
can or give a warning someone can send me.
* A failing test might be spurious depending on the platform and font
rendering scheme. So again just provide a warning someone can send
me.
[Source editor can be confusing for errors. Also an editor! But not
showing the file you asked for!]
* But our framework clears the warning after running tests:
* If someone is deep in developing a new feature and quits -> restore
back in the source editor.
[Perhaps `Current_app` is the wrong place for this third hacky mode,
since we actually want to continue running. Perhaps it's orthogonal to
`Current_app`.]
[Ideally I wouldn't run the tests after the version check. I'd pause,
wait for a key and then resume tests? "Muddle through" is a pain to
orchestrate.]
* We store `Current_app` in settings. But we don't really intend to
persist a `Current_app` of 'error'. Only the main app or 'source'
editor.
[Another vote against storing 'error' in `Current_app`.]
* So we need to rerun the version check after running tests to actually
show the warning.
[Perhaps I need to separate out the side-effect of setting `Version`
from the side-effect of changing `Current_app`. But that's not right
either, because I do still want to raise an error message if the
version check fails before running tests. Which brings us back to
wanting to run the tests after raising the version check..]
One good thing: none of the bugs so far have been about silently
ignoring test failures. I thought that might be the case for a bit,
which was unnerving.
I grew similar muddiness in Mu's bootstrap system over time, with
several surrounding modes around the core program that interacted poorly
or at least unsatisfyingly with each other. On one level it just feels
like this outer layer reflects muddy constraints in the real world. But
perhaps there's some skill I still need to learn here..
Why am I even displaying this error if we're going to try to muddle
through anyway? In (vain) hopes that someone will send me that
information. It's not terribly actionable even to me. But it's really
intended for when making changes. If a test fails then, you want to
know.
The code would be cleaner if I just threw an unrecoverable error from
the version check. Historically, the way I arrived at this solution was:
* I used the default love.errorhandler for a while
* I added xpcall and error recovery, but now I have situations where I
would rather fall back on love.errorhandler. How to tell xpcall
that?
But no, this whole line of thought is wrong. LÖVE has a precedent for
trying to muddle through on an unexpected version. And spurious test
failures don't merit a hard crash. There's some irreducible requirement
here. No point making the code simplistic when the world is complex.
Perhaps I should stop caching Version and just recompute it each time.
It's only used once so far, hardly seems worth the global.
We have two bits of irreducible complexity here:
* If tests fail it might be a real failure, or it might not.
* Even if it's an unexpected version, everything might be fine.
And the major remaining problem happens at the intersection of these two
bits. What if we get an unexpected version with some difference that
causes tests to fail? But this is a hypothetical and not worth thinking
about since I'll update the app fairly quickly in response to new
versions.
We'll end up calling Text.redraw_all anyway, which will clear starty and
much more besides.
We'll still conservatively continue clearing starty in a few places
where there's a possibility that Text.redraw_all may not be called. This
change is riskier than most.
Just checking mouse.isDown works if the editor is the entirety of the
app, as is true in this fork. However, we often want to introduce other
widgets. We'd like tapping on them to not cause the selection to flash:
https://news.ycombinator.com/context?id=38404923&submission=38397715
The right architecture to enforce this is: have each layer of the UI
maintain its own state machine between mouse_press and mouse_release
events. And only check the state machine in the next level down rather
than lower layers or the bottommost layer of raw LÖVE.
I'm not sure this can trigger everywhere (I've only been able to
exercise it in Lua Carousel), but it seems like a safety net worth
having against future modifications by anybody.
This commit doesn't guarantee we'll always catch it. But if this
invariant is violated, things can get quite difficult to debug. I found
in the Lua Carousel fork that all the xpcalls I keep around were
actively hindering my ability to notice this invariant being violated.
This came up when trying to integrate my apps with the vudu debugger
(https://github.com/deltadaedalus/vudu). In general, it's a subtle part
of LÖVE's semantics that you can modify event handlers any time and your
modifications will get picked up. Now my Freewheeling Apps will follow
this norm as well.