Crash on resize on master

[jtdaugherty]

@iphydf I'm not sure which of your recent changes could have impacted this, but when I attempt a resize I get this crash pretty consistently. Would you be willing to look into this?

matterhorn: vty internal failure: this value should not propagate to users
CallStack (from HasCallStack):
  error, called at src/Graphics/Vty/Input.hs:161:21 in vty-5.34-inplace:Graphics.Vty.Input

Here's some relevant code, but I haven't looked into how this might have arisen as a consequence of your changes:

      let pokeIO = Catch $ do
              let e = error "vty internal failure: this value should not propagate to users"
              setAttrs
              atomically $ writeTChan (input^.eventChannel) (EvResize e e)
      _ <- installHandler windowChange pokeIO Nothing
      _ <- installHandler continueProcess pokeIO Nothing

[iphydf]

Will take a look at that now.

[iphydf]

Ah, I think I know: EvResize is a strict struct now. The fix would be to either explicitly make EvResize fields lazy or ensure that the EvResize e e expression itself is lazy. I'll see if I can do the latter, otherwise I'll make a PR for the former.

[iphydf]

Or alternatively maybe I can avoid the bottom value altogether. I'll have a look at the code.

[jtdaugherty]

I guess my reaction to the code above is, why is a lazy EvResize being written to that channel in the first place? Is there a better way to deal with whatever that code is for?

[iphydf]

I agree. I'm trying to figure out where that event goes and where it's being evaluated.

[jtdaugherty]

I wonder if it's just a dummy event to get the internal reader to wake up and redraw..

[jtdaugherty]

(I.e. the initial draw)

[iphydf]

Sending any other event works too (e.g. EvGainedFocus). I don't know if it always works, though.

[iphydf]

Not sending any event does not work (resize becomes delayed until the next real event happens), so yeah that seems reasonable.

[jtdaugherty]

Looking at the code, if this is just for recovery after signals, then would it be possible to provide the actual window dimensions in the EvResize event that the handler is producing? That seems like a way to make this behavior legitimate.

[iphydf]

It is received here, ignoring the bottom values in the constructor arguments: https://github.com/jtdaugherty/vty/blob/e4fb394cdf9565673697af6aba9c6427337a9b05/src/Graphics/Vty.hs#L215

and then it's fixed up to become a real resize event with the new bounds. It doesn't seem like we can access the bounds in the sender context. This also means that sending anything other than EvResize bot bot will not be quite correct (even though it does seem to work - I'm not sure when it would not).

[iphydf]

Also correct would be to send EvResize 0 0 or anything else, but the current code would catch the introduction of a bug where that 0 value accidentally ends up in user code. The current code is better than sending 0 0.

[jtdaugherty]

Okay, that makes sense. Thank you for looking into this! I'll merge the PR you created, but I also want to think about improving the internal event-handling so that a constructor for this specific case could be added without polluting the event type that applications handle. I don't know how much of an overhaul that would require, but I'd like to have the cake and eat it too (make resizes strict, and not use dummy resize constructors as they are used here, overloading the Event type in this way).

[jtdaugherty]

I had some time and this turned out to be pretty straightforward. Let me know what you think, @iphydf -

https://github.com/jtdaugherty/vty/commit/2cd98a862c88069e3863ad0ad989dfa0e9081d29

[jtdaugherty]

(I didn't make the EvResize fields strict in that branch yet, but will if these changes are otherwise reasonable.)

[jtdaugherty]

Also, a note on the API breakage: I very much doubt that the majority of Vty users are in the habit of customizing their Inputs, and since that's the only spot where the new type affects the user-facing API, I suspect that the risk of this causing anyone any trouble is very low.

[iphydf]

LGTM. Yeah, that's a good way of doing it. Thanks for the better fix :).

[jtdaugherty]

Okay, thanks for taking a look. It's now merged, and I'll remove the laziness annotations from EvResize shortly.