revery-ui / revery

:zap: Native, high-performance, cross-platform desktop apps - built with Reason!
https://www.outrunlabs.com/revery/
MIT License
8.07k stars 196 forks source link

onBeforeQuit: allow subscribers to prevent immediate quit #1053

Closed timbertson closed 3 years ago

timbertson commented 3 years ago

Implements the API change discussed in https://github.com/onivim/oni2/issues/3270

I decided to make a Fanout submodule of Event to represent "events with return values", but am very open to alternative structures. It seemed unnecessary to add another type paramater for Event.t when it's almost always going to be unit.

Since this changes the onBeforeQuit signature anyway, I included the exit code since it seems like that might be useful, but I don't have a specific use case in mind.

I didn't make it backwards compatible. It would be possible by instead adding a new onBeforeQuitFanout (or something) function that exposes the ability to suppress the response, but I don't know what your plans are in general for backwards compat, so I went with the simplest version first.

bryphe commented 3 years ago

Since this changes the onBeforeQuit signature anyway, I included the exit code since it seems like that might be useful, but I don't have a specific use case in mind.

Sounds reasonable to me. I could potentially see cases where, whether the exit code is success or failure, we may want to handle it differently.

It would be possible by instead adding a new onBeforeQuitFanout (or something) function that exposes the ability to suppress the response, but I don't know what your plans are in general for backwards compat, so I went with the simplest version first.

For this API, I'm OK breaking backwards compat - AFAIK, Onivim is the only consumer of this API. So the simpler approach sounds good - thank you for thinking about this and checking @timbertson !

timbertson commented 3 years ago

yay! 🥳

I've raised a proper PR in Oni2: https://github.com/onivim/oni2/pull/3326

bryphe commented 3 years ago

I've raised a proper PR in Oni2: onivim/oni2#3326

Nice! Thank you @timbertson - merging now 😄