owickstrom / gi-gtk-declarative

Declarative GTK+ programming in Haskell
https://owickstrom.github.io/gi-gtk-declarative/
288 stars 35 forks source link

Do not ignore exception on main loop #78

Closed guibou closed 4 years ago

guibou commented 4 years ago

This closes #77.

In the previous implementation, any exception in runLoop was ignored until Gtk.main ends. This resulted to weird behaviors where user update / view functions where ignored, but the application was still running and primitive gtk events (such as style change on mouse overlays) were still running, resulting in an apparently alive but unresponsive application. The exception was also not printed.

With this change, the application now quit if any exception is raised in the view and update user function. User are in charge of setting an exception handler if they want a different behavior.

Dretch commented 4 years ago

I think this totally makes sense. I will attempt to write an automated test for this feature, and then I think I have permission to merge the code (might need to fix Travis somehow too).

guibou commented 4 years ago

@Dretch Thank you for adding a test and taking care of this PR. Do you think you can extend your test with a testcase for an error in the view method too?

Dretch commented 4 years ago

@guibou

@Dretch Thank you for adding a test and taking care of this PR. Do you think you can extend your test with a testcase for an error in the view method too?

Good idea. I have pushed a test. It is currently failing, however! I think this is related to the view function being evaluated on the Gtk+ UI thread.

guibou commented 4 years ago

@Dretch Thank you. Let me update the PR with the fix.

guibou commented 4 years ago

@Dretch it was a bit more painful than expected.

I addressed view and pipeline exception. And also, i realized that any exception in update deeply inside the transition type may not be caught too.

It is a bit of a mess. I tested by running all the examples.

owickstrom commented 4 years ago

Good stuff! 👍

guibou commented 4 years ago

@Dretch I think I fixed the space leak issue. I also added a test for the Transition exception you depicted. It is a nightmare because there is 3 places where a transition may raise an exception (Transition _ undefined, Transition _ (pure undefined), Transition _ (pure (Just undefined))), and the three are not handled the same way ;)

Dretch commented 4 years ago

Looks great @guibou

guibou commented 4 years ago

@Dretch with some mistake (removing your commits, now fixed ;)), I rebased on top of master with the hope that the new nix environment will help the success of tests.

Dretch commented 4 years ago

@Dretch with some mistake (removing your commits, now fixed ;)), I rebased on top of master with the hope that the new nix environment will help the success of tests.

I think the gi-gtk-declarative-app-simple tests are failing because they run Gtk, which needs a framebuffer and dbus. The gi-gtk-declarative tests were already setup with these in Nix, so I have setup the app-simple tests the same way (your review is welcome).

guibou commented 4 years ago

@Dretch looks fine. The code you moved may be improved a bit (for example, I think that addBuildTool is more adapted than addBuildDepend), but that's orthogonal to this PR.

guibou commented 4 years ago

And good job to the original author (@owickstrom, I guess) for the wrapper which allows to run a GTK application in a test. I will use that.

Dretch commented 4 years ago

Thanks @guibou :tada: