From 76c476f2dcfa3bfb5b65ef0a811b6c0326f9dcbf Mon Sep 17 00:00:00 2001 From: whitequark Date: Thu, 19 Jul 2018 21:54:05 +0000 Subject: [PATCH] Eliminate blocking in Error() and Message() calls. This serves two purposes. First, we want to (some day) convert these messages into a less obtrustive form, something like toaster notifications, such that they don't interrupt workflow as harshly. That would, of course, be nonblocking. Second, some platforms, like Emscripten, do not support nested event loops, and it's not possible to display a modal dialog on them synchronously. When making this commit, I've reviewed all Error() and Message() calls to ensure that only some of the following is true for all of them: * The call is followed a break or return statement that exits an UI entry point (e.g. an MenuX function); * The call is followed by cleanup (in fact, in this case the new behavior is better, since even with a synchronous modal dialog we have to be reentrant); * The message is an informational message only and nothing unexpected will happen if the operation proceeds in background. In general, all Error() calls already satisfied the above conditions, although in some cases I changed control flow aroudn them to more clearly show that. The Message() calls that didn't satisfy these conditions were reworked into an asynchronous form. There are three explicit RunModal() calls left that need to be reworked into an async form. --- src/confscreen.cpp | 6 +++--- src/file.cpp | 1 + src/graphicswin.cpp | 32 +++++++++++++++----------------- src/importdxf.cpp | 1 + src/modify.cpp | 1 + src/platform/gui.h | 10 +++++++++- src/platform/guigtk.cpp | 34 ++++++++++++++++++++++++++-------- src/solvespace.cpp | 2 ++ src/solvespace.h | 1 + src/util.cpp | 20 +++++++++++++++++--- 10 files changed, 76 insertions(+), 32 deletions(-) diff --git a/src/confscreen.cpp b/src/confscreen.cpp index 026a70e37..7c8d54b2a 100644 --- a/src/confscreen.cpp +++ b/src/confscreen.cpp @@ -331,10 +331,10 @@ bool TextWindow::EditControlDoneForConfiguration(const std::string &s) { double x, y, z; if(sscanf(s.c_str(), "%lf, %lf, %lf", &x, &y, &z)==3) { SS.lightDir[edit.i] = Vector::From(x, y, z); + SS.GW.Invalidate(); } else { Error(_("Bad format: specify coordinates as x, y, z")); } - SS.GW.Invalidate(); break; } case Edit::COLOR: { @@ -367,11 +367,11 @@ bool TextWindow::EditControlDoneForConfiguration(const std::string &s) { } case Edit::CAMERA_TANGENT: { SS.cameraTangent = (min(2.0, max(0.0, atof(s.c_str()))))/1000.0; + SS.GW.Invalidate(); if(!SS.usePerspectiveProj) { Message(_("The perspective factor will have no effect until you " "enable View -> Use Perspective Projection.")); } - SS.GW.Invalidate(); break; } case Edit::GRID_SPACING: { @@ -385,8 +385,8 @@ bool TextWindow::EditControlDoneForConfiguration(const std::string &s) { Error(_("Specify between 0 and 8 digits after the decimal.")); } else { SS.SetUnitDigitsAfterDecimal(v); + SS.GW.Invalidate(); } - SS.GW.Invalidate(); break; } case Edit::EXPORT_SCALE: { diff --git a/src/file.cpp b/src/file.cpp index c3c8c29e4..23e8c81cc 100644 --- a/src/file.cpp +++ b/src/file.cpp @@ -844,6 +844,7 @@ static Platform::MessageDialog::Response LocateImportedFile(const Platform::Path dialog->AddButton(C_("button", "&Cancel"), MessageDialog::Response::CANCEL); } + // FIXME(async): asyncify this call return dialog->RunModal(); } diff --git a/src/graphicswin.cpp b/src/graphicswin.cpp index b44f1a96a..a6d623e73 100644 --- a/src/graphicswin.cpp +++ b/src/graphicswin.cpp @@ -678,15 +678,17 @@ void GraphicsWindow::MenuView(Command id) { case Command::SHOW_GRID: SS.GW.showSnapGrid = !SS.GW.showSnapGrid; + SS.GW.EnsureValidActives(); + SS.GW.Invalidate(); if(SS.GW.showSnapGrid && !SS.GW.LockedInWorkplane()) { Message(_("No workplane is active, so the grid will not appear.")); } - SS.GW.EnsureValidActives(); - SS.GW.Invalidate(); break; case Command::PERSPECTIVE_PROJ: SS.usePerspectiveProj = !SS.usePerspectiveProj; + SS.GW.EnsureValidActives(); + SS.GW.Invalidate(); if(SS.cameraTangent < 1e-6) { Error(_("The perspective factor is set to zero, so the view will " "always be a parallel projection.\n\n" @@ -694,8 +696,6 @@ void GraphicsWindow::MenuView(Command id) { "factor in the configuration screen. A value around 0.3 " "is typical.")); } - SS.GW.EnsureValidActives(); - SS.GW.Invalidate(); break; case Command::ONTO_WORKPLANE: @@ -1050,11 +1050,11 @@ void GraphicsWindow::MenuEdit(Command id) { } } } while(didSomething); + SS.GW.Invalidate(); + SS.ScheduleShowTW(); if(newlySelected == 0) { Error(_("No additional entities share endpoints with the selected entities.")); } - SS.GW.Invalidate(); - SS.ScheduleShowTW(); break; } @@ -1077,7 +1077,6 @@ void GraphicsWindow::MenuEdit(Command id) { break; } - SS.UndoRemember(); // Rotate by ninety degrees about the coordinate axis closest // to the screen normal. @@ -1168,20 +1167,18 @@ void GraphicsWindow::MenuRequest(Command id) { } else if(g->type == Group::Type::DRAWING_WORKPLANE) { // The group's default workplane g->activeWorkplane = g->h.entity(0); - Message(_("No workplane selected. Activating default workplane " - "for this group.")); - } - - if(!SS.GW.LockedInWorkplane()) { + MessageAndRun([] { + // Align the view with the selected workplane + SS.GW.ClearSuper(); + SS.GW.AnimateOntoWorkplane(); + }, _("No workplane selected. Activating default workplane " + "for this group.")); + } else { Error(_("No workplane is selected, and the active group does " "not have a default workplane. Try selecting a " "workplane, or activating a sketch-in-new-workplane " "group.")); - break; } - // Align the view with the selected workplane - SS.GW.ClearSuper(); - SS.GW.AnimateOntoWorkplane(); break; } case Command::FREE_IN_3D: @@ -1232,12 +1229,13 @@ void GraphicsWindow::MenuRequest(Command id) { break; case Command::CONSTRUCTION: { - SS.UndoRemember(); SS.GW.GroupSelection(); if(SS.GW.gs.entities == 0) { Error(_("No entities are selected. Select entities before " "trying to toggle their construction state.")); + break; } + SS.UndoRemember(); int i; for(i = 0; i < SS.GW.gs.entities; i++) { hEntity he = SS.GW.gs.entity[i]; diff --git a/src/importdxf.cpp b/src/importdxf.cpp index 7eb361683..a6bb305d1 100644 --- a/src/importdxf.cpp +++ b/src/importdxf.cpp @@ -1100,6 +1100,7 @@ static void ImportDwgDxf(const Platform::Path &filename, importer.clearBlockTransform(); if(!read(data, &importer)) { Error("Corrupted %s file.", fileType.c_str()); + return; } if(importer.unknownEntities > 0) { Message("%u %s entities of unknown type were ignored.", diff --git a/src/modify.cpp b/src/modify.cpp index a9ce872fc..539ac43ef 100644 --- a/src/modify.cpp +++ b/src/modify.cpp @@ -711,6 +711,7 @@ void GraphicsWindow::SplitLinesOrCurves() { } } else { Error(_("Can't split; no intersection found.")); + return; } // All done, clean up and regenerate. diff --git a/src/platform/gui.h b/src/platform/gui.h index 0d8c6b580..74e7ee3bf 100644 --- a/src/platform/gui.h +++ b/src/platform/gui.h @@ -213,7 +213,7 @@ class Window { std::function onClose; std::function onFullScreen; std::function onMouseEvent; - std::function onSixDofEvent; + std::function onSixDofEvent; std::function onKeyboardEvent; std::function onEditingDone; std::function onScrollbarAdjusted; @@ -293,6 +293,8 @@ class MessageDialog { CANCEL }; + std::function onResponse; + virtual ~MessageDialog() {} virtual void SetType(Type type) = 0; @@ -303,6 +305,12 @@ class MessageDialog { virtual void AddButton(std::string name, Response response, bool isDefault = false) = 0; virtual Response RunModal() = 0; + virtual void ShowModal() { + Response response = RunModal(); + if(onResponse) { + onResponse(response); + } + } }; typedef std::shared_ptr MessageDialogRef; diff --git a/src/platform/guigtk.cpp b/src/platform/guigtk.cpp index 7d06ddf04..2c78edc00 100644 --- a/src/platform/guigtk.cpp +++ b/src/platform/guigtk.cpp @@ -1078,7 +1078,11 @@ class MessageDialogImplGtk final : public MessageDialog { : gtkDialog(parent, "", /*use_markup=*/false, Gtk::MESSAGE_INFO, Gtk::BUTTONS_NONE, /*modal=*/true) { - SetTitle("Message"); + SetTitle("Message"); + + gtkDialog.signal_response().connect([this](int gtkResponse) { + ProcessResponse(gtkResponse); + }); } void SetType(Type type) override { @@ -1129,21 +1133,35 @@ class MessageDialogImplGtk final : public MessageDialog { } } - Response RunModal() override { - switch(gtkDialog.run()) { - case Gtk::RESPONSE_OK: return Response::OK; break; - case Gtk::RESPONSE_YES: return Response::YES; break; - case Gtk::RESPONSE_NO: return Response::NO; break; - case Gtk::RESPONSE_CANCEL: return Response::CANCEL; break; + Response ProcessResponse(int gtkResponse) { + Response response; + switch(gtkResponse) { + case Gtk::RESPONSE_OK: response = Response::OK; break; + case Gtk::RESPONSE_YES: response = Response::YES; break; + case Gtk::RESPONSE_NO: response = Response::NO; break; + case Gtk::RESPONSE_CANCEL: response = Response::CANCEL; break; case Gtk::RESPONSE_NONE: case Gtk::RESPONSE_CLOSE: case Gtk::RESPONSE_DELETE_EVENT: - return Response::NONE; + response = Response::NONE; break; default: ssassert(false, "Unexpected response"); } + + if(onResponse) { + onResponse(response); + } + return response; + } + + void ShowModal() override { + gtkDialog.show(); + } + + Response RunModal() override { + return gtkDialog.run(); } }; diff --git a/src/solvespace.cpp b/src/solvespace.cpp index cd533dfdc..7e643646b 100644 --- a/src/solvespace.cpp +++ b/src/solvespace.cpp @@ -161,6 +161,7 @@ bool SolveSpaceUI::LoadAutosaveFor(const Platform::Path &filename) { /*isDefault=*/true); dialog->AddButton(C_("button", "Do&n't Load"), MessageDialog::Response::NO); + // FIXME(async): asyncify this call if(dialog->RunModal() == MessageDialog::Response::YES) { unsaved = true; return LoadFromFile(autosaveFile, /*canCancel=*/true); @@ -463,6 +464,7 @@ bool SolveSpaceUI::OkayToStartNewFile() { dialog->AddButton(C_("button", "Do&n't Save"), MessageDialog::Response::NO); dialog->AddButton(C_("button", "&Cancel"), MessageDialog::Response::CANCEL); + // FIXME(async): asyncify this call switch(dialog->RunModal()) { case MessageDialog::Response::YES: return GetFilenameAndSave(/*saveAs=*/false); diff --git a/src/solvespace.h b/src/solvespace.h index ee4605a2c..395adff7f 100644 --- a/src/solvespace.h +++ b/src/solvespace.h @@ -235,6 +235,7 @@ void MultMatrix(double *mata, double *matb, double *matr); int64_t GetMilliseconds(); void Message(const char *fmt, ...); +void MessageAndRun(std::function onDismiss, const char *fmt, ...); void Error(const char *fmt, ...); class System { diff --git a/src/util.cpp b/src/util.cpp index 07aa90f94..b0dde5479 100644 --- a/src/util.cpp +++ b/src/util.cpp @@ -100,10 +100,11 @@ void SolveSpace::MultMatrix(double *mata, double *matb, double *matr) { } //----------------------------------------------------------------------------- -// Word-wrap the string for our message box appropriately, and then display +// Format the string for our message box appropriately, and then display // that string. //----------------------------------------------------------------------------- -static void MessageBox(const char *fmt, va_list va, bool error) +static void MessageBox(const char *fmt, va_list va, bool error, + std::function onDismiss = std::function()) { #ifndef LIBRARY va_list va_size; @@ -156,7 +157,13 @@ static void MessageBox(const char *fmt, va_list va, bool error) } dialog->AddButton(C_("button", "&OK"), MessageDialog::Response::OK, /*isDefault=*/true); - dialog->RunModal(); + + dialog->onResponse = [=](MessageDialog::Response _response) { + if(onDismiss) { + onDismiss(); + } + }; + dialog->ShowModal(); #endif } void SolveSpace::Error(const char *fmt, ...) @@ -173,6 +180,13 @@ void SolveSpace::Message(const char *fmt, ...) MessageBox(fmt, f, /*error=*/false); va_end(f); } +void SolveSpace::MessageAndRun(std::function onDismiss, const char *fmt, ...) +{ + va_list f; + va_start(f, fmt); + MessageBox(fmt, f, /*error=*/false, onDismiss); + va_end(f); +} //----------------------------------------------------------------------------- // Solve a mostly banded matrix. In a given row, there are LEFT_OF_DIAG