johnnovak / illwill

A curses inspired simple cross-platform console library for Nim
Do What The F*ck You Want To Public License
398 stars 27 forks source link

change gBoxCharsUnicode to a const array #52

Closed jrfondren closed 3 months ago

jrfondren commented 3 months ago

... from a threadvar that's only initialized on the main thread, leading to zero-length toUTF8String results, and IndexDefect on .runeAt(0)

That error can be seen by taking the examples/keycodes.nim and running it in another thread:

--- examples/keycodes.nim       2024-05-04 21:18:51.932685246 -0500
+++ keycodes2.nim   2024-05-04 21:02:55.339472718 -0500
@@ -49,7 +49,7 @@
   tb.write(tx+14, ty, $gLastKeyPressed)

-proc main() =
+proc main() {.thread.} =
   illwillInit(fullscreen=true)
   setControlCHook(exitProc)
   hideCursor()
@@ -68,5 +68,6 @@

     sleep(50)

-main()
-
+var t: Thread[void]
+createThread t, main
+joinThread t

My use-case is making a TUI that's controlled by Erlang, and implemented by a Nim DLL. Since the limited NIF interface is used, the Nim DLL can't maintain a UI loop except in a separate thread.

johnnovak commented 3 months ago

Yeah, I don't remember why I needed a {.threadvar.}. Well, this is meant to be used from the main thread, and there are other threadvars in the code, and I don't have time or inclination to figure this out in the foreseeable future.

So please just fork it and use it like that.

jrfondren commented 3 months ago

The problem isn't that this is a threadvar, but that it's a threadvar that's initialized by toplevel code that only runs once, leaving the threadvar with only default initialization in other threads. You don't have any other threadvars like this.

You likely don't remember why you needed {.threadvar.} because those were added by earlier PRs, #19 and #29.

johnnovak commented 3 months ago

Indeed, I don't remember because it wasn't me, apparently 😄 My intention was to keep illwill simple, so yeah, no thread safety, just use it from the main thread.

But your PR doesn't seem to cause any harm, so merging, after all 😄 Thanks!

johnnovak commented 3 months ago

Bumped version to v0.4.1 @jrfondren