Re: [PATCH setup] Make sure that fatal error messages are visible
On 2017-12-20 10:09, Ken Brown wrote: > On 12/20/2017 11:19 AM, Jon Turney wrote: >> On 19/12/2017 00:53, Ken Brown wrote: >>> The message box produced by TOPLEVEL_CATCH could be hidden by whatever >>> window was previously being displayed, so that setup appeared to hang. >>> Fix this by giving fatal error message boxes type MB_SETFOREGROUND. >> >> This is good as far as it goes, but is kind of working around the fact that >> fatal() is being called with an NULL owner HWND. >> >> This is not idea because I guess it means that propsheet window is still >> activate-able when this messagebox is displayed (MB_APPMODAL doesn't apply)? > > It turns out that MB_SYSTEMMODAL did the job. I tried MB_APPLMODAL and > MB_TASKMODAL also, but both of those still allowed me to activate the > propsheet > window. Is it really a problem if users can look at other windows when there is an error? It is often useful to be able to look at your inputs to see if they played a role in causing the error, or it is some external issue. -- Take care. Thanks, Brian Inglis, Calgary, Alberta, Canada
Re: [PATCH setup] Make sure that fatal error messages are visible
On 12/20/2017 11:19 AM, Jon Turney wrote:
On 19/12/2017 00:53, Ken Brown wrote:
The message box produced by TOPLEVEL_CATCH could be hidden by whatever
window was previously being displayed, so that setup appeared to hang.
Fix this by giving fatal error message boxes type MB_SETFOREGROUND.
This is good as far as it goes, but is kind of working around the fact
that fatal() is being called with an NULL owner HWND.
This is not idea because I guess it means that propsheet window is still
activate-able when this messagebox is displayed (MB_APPMODAL doesn't
apply)?
It turns out that MB_SYSTEMMODAL did the job. I tried MB_APPLMODAL and
MB_TASKMODAL also, but both of those still allowed me to activate the
propsheet window.
Revised patch attached.
Ken
From 1f99ac4cc6c2b6c0b39aa84d80985cb21438a242 Mon Sep 17 00:00:00 2001
From: Ken Brown
Date: Mon, 18 Dec 2017 19:53:00 -0500
Subject: [PATCH setup v2 1/2] Make sure that fatal error messages are visible
The message box produced by TOPLEVEL_CATCH could be hidden by whatever
window was previously being displayed, so that setup appeared to hang.
Fix this by giving fatal error message boxes type MB_SYSTEMMODAL.
This also prevents the user from activating the property sheet
window while the fatal message box is being displayed.
---
msg.cc | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/msg.cc b/msg.cc
index 403e78a..92c9675 100644
--- a/msg.cc
+++ b/msg.cc
@@ -83,7 +83,7 @@ fatal (HWND owner, int id, ...)
{
va_list args;
va_start (args, id);
- mbox (owner, "fatal", 0, id, args);
+ mbox (owner, "fatal", MB_SYSTEMMODAL, id, args);
Logger ().exit (1);
}
--
2.15.1
Re: [PATCH setup] Throw exceptions instead of exiting with error codes
On 12/20/2017 11:06 AM, Jon Turney wrote:
On 19/12/2017 17:20, Ken Brown wrote:
---
Exception.h | 1 +
PickView.cc | 11 +++
choose.cc | 7 +--
3 files changed, 13 insertions(+), 6 deletions(-)
Yeah, silently dying seems like a bad idea.
Hmmm... does this not need a corresponding use of TOPLEVEL_CATCH to
transform that exception into a fatal() ?
It's already caught by a TOPLEVEL_CATCH.
In any case, some commentary in the commit about how these exceptions
are expected to be handled would good.
Revised patch attached.
Ken
From 7e41d3bcd3994394b775744b3c53b7a8f54b2282 Mon Sep 17 00:00:00 2001
From: Ken Brown
Date: Tue, 19 Dec 2017 11:10:22 -0500
Subject: [PATCH setup v2] Throw exceptions instead of exiting with error codes
There were two places in PickView.cc and one in choose.cc where setup
would silently die with an error code. Change this so that setup now
throws exceptions. The exceptions are caught by
TOPLEVEL_CATCH("DialogProc") in PropertyPage::DialogProc().
---
Exception.h | 1 +
PickView.cc | 11 +++
choose.cc | 7 +--
3 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/Exception.h b/Exception.h
index 7b16612..15a145e 100644
--- a/Exception.h
+++ b/Exception.h
@@ -36,6 +36,7 @@ private:
#define APPERR_CORRUPT_PACKAGE 1
#define APPERR_IO_ERROR2
#define APPERR_LOGIC_ERROR 3
+#define APPERR_WINDOW_ERROR4
#define TOPLEVEL_CATCH(threadname) \
catch (Exception *e) \
diff --git a/PickView.cc b/PickView.cc
index d24866c..2e1beda 100644
--- a/PickView.cc
+++ b/PickView.cc
@@ -27,6 +27,7 @@
/* For 'source' */
#include "state.h"
#include "LogSingleton.h"
+#include "Exception.h"
using namespace std;
@@ -539,8 +540,9 @@ PickView::init(views _mode)
HDS_HORZ, 0, 0, 0, 0, GetHWND(),
(HMENU) IDC_CHOOSE_LISTHEADER, hinstance,
(LPVOID) NULL)) == NULL)
-// FIXME: throw an exception
-exit (10);
+throw new Exception (TOSTRING(__LINE__) " " __FILE__,
+"Unable to create list header window",
+APPERR_WINDOW_ERROR);
// Retrieve the bounding rectangle of the parent window's
// client area, and then request size and position values
@@ -550,8 +552,9 @@ PickView::init(views _mode)
hdl.prc = &rcParent;
hdl.pwpos = ℘
if (!SendMessage (listheader, HDM_LAYOUT, 0, (LPARAM) & hdl))
-// FIXME: throw an exception
-exit (11);
+throw new Exception (TOSTRING(__LINE__) " " __FILE__,
+"Unable to get size and position of rectangle",
+APPERR_WINDOW_ERROR);
// Set the font of the listheader, but don't redraw, because its not shown
// yet.This message does not return a value, so we are not checking it as we
diff --git a/choose.cc b/choose.cc
index c78f55d..32600c8 100644
--- a/choose.cc
+++ b/choose.cc
@@ -57,6 +57,8 @@
#include "UserSettings.h"
+#include "Exception.h"
+
#include "getopt++/BoolOption.h"
static BoolOption UpgradeAlsoOption (false, 'g', "upgrade-also", "also upgrade
installed packages");
static BoolOption CleanOrphansOption (false, 'o', "delete-orphans", "remove
orphaned packages");
@@ -143,8 +145,9 @@ ChooserPage::createListview ()
chooser = new PickView (cat);
RECT r = getDefaultListViewSize();
if (!chooser->Create(this, WS_CHILD | WS_HSCROLL | WS_VSCROLL |
WS_VISIBLE,&r))
-// TODO throw exception
-exit (11);
+throw new Exception (TOSTRING(__LINE__) " " __FILE__,
+"Unable to create chooser list window",
+APPERR_WINDOW_ERROR);
chooser->init(PickView::views::Category);
chooser->Show(SW_SHOW);
chooser->setViewMode (!is_new_install || UpgradeAlsoOption ||
hasManualSelections ?
--
2.15.1
Re: [PATCH setup] Make sure that fatal error messages are visible
On 19/12/2017 00:53, Ken Brown wrote:
The message box produced by TOPLEVEL_CATCH could be hidden by whatever
window was previously being displayed, so that setup appeared to hang.
Fix this by giving fatal error message boxes type MB_SETFOREGROUND.
This is good as far as it goes, but is kind of working around the fact
that fatal() is being called with an NULL owner HWND.
This is not idea because I guess it means that propsheet window is still
activate-able when this messagebox is displayed (MB_APPMODAL doesn't apply)?
---
msg.cc | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/msg.cc b/msg.cc
index 403e78a..0ba4839 100644
--- a/msg.cc
+++ b/msg.cc
@@ -83,7 +83,7 @@ fatal (HWND owner, int id, ...)
{
va_list args;
va_start (args, id);
- mbox (owner, "fatal", 0, id, args);
+ mbox (owner, "fatal", MB_SETFOREGROUND, id, args);
Logger ().exit (1);
}
Re: [PATCH setup] Throw exceptions instead of exiting with error codes
On 19/12/2017 17:20, Ken Brown wrote: --- Exception.h | 1 + PickView.cc | 11 +++ choose.cc | 7 +-- 3 files changed, 13 insertions(+), 6 deletions(-) Yeah, silently dying seems like a bad idea. Hmmm... does this not need a corresponding use of TOPLEVEL_CATCH to transform that exception into a fatal() ? Or does the default handling of the exception by the runtime report the error? In any case, some commentary in the commit about how these exceptions are expected to be handled would good.
