[PATCH setup 0/5] Improve behavior after download error, v2

[PATCH setup 0/5] Improve behavior after download error, v2

[Ken Brown]

Currently setup goes back to the mirror selection page after a
download error if the user answers "Yes" to "Download incomplete.  Try
again?".  The same happens in unattended mode until the retries have
been exhausted.

And if the user answers "No", then installation continues, even though
this can damage the user's installation.  For example, if a package is
selected for reinstall but cannot be downloaded, it will be
uninstalled.

This series of patches changes the behavior as follows:

 - In unattended mode, simply retry the download.

 - In interactive mode, pop up a dialog showing which packages had
   download errors and giving the user the following options:
    - Retry (retries download)
    - Back (return to the package selection page)
    - Continue, with a warning
    - Cancel (exit)

Ken Brown (5):
  Just retry download after error in unattended mode
  Fix off-by-one error in download retry report
  Remove "Try again?" from exit message.
  Query user after download error in interactive mode
  Limit the number of packages shown in the IDD_DOWNLOAD_ERROR listbox

 download.cc | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++---------
 res.rc      | 23 ++++++++++++++
 resource.h  |  3 ++
 3 files changed, 111 insertions(+), 14 deletions(-)

-- 
2.15.0

[PATCH setup 3/5] Remove "Try again?" from exit message.

[Ken Brown]

---
 download.cc | 2 +-
 res.rc      | 1 +
 resource.h  | 1 +
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/download.cc b/download.cc
index a430f7f..841f680 100644
--- a/download.cc
+++ b/download.cc
@@ -266,7 +266,7 @@ do_download_thread (HINSTANCE h, HWND owner)
   if (source == IDC_SOURCE_DOWNLOAD)
     {
       if (errors)
-	Logger ().setExitMsg (IDS_DOWNLOAD_INCOMPLETE);
+	Logger ().setExitMsg (IDS_DOWNLOAD_INCOMPLETE_EXIT);
       else if (!unattended_mode)
 	Logger ().setExitMsg (IDS_DOWNLOAD_COMPLETE);
       return IDD_DESKTOP;
diff --git a/res.rc b/res.rc
index 76a871f..d1f0871 100644
--- a/res.rc
+++ b/res.rc
@@ -526,6 +526,7 @@ BEGIN
     IDS_ERR_CHDIR           "Could not change dir to %s: %s [%.8x]"
     IDS_OLD_SETUP_VERSION   "This setup is version %s, but setup.ini claims version %s is available.\nYou might want to upgrade to get the latest features and bug fixes."
     IDS_DOWNLOAD_INCOMPLETE "Download Incomplete.  Try again?"
+    IDS_DOWNLOAD_INCOMPLETE_EXIT  "Download incomplete.  Check %s for details"
     IDS_INSTALL_ERROR	    "Installation error (%s), Continue with other packages?"
     IDS_INSTALL_INCOMPLETE  "Installation incomplete.  Check %s for details"
     IDS_CORRUPT_PACKAGE     "Package file %s has a corrupt local copy, please remove and retry."
diff --git a/resource.h b/resource.h
index 172b2c8..98a4a0f 100644
--- a/resource.h
+++ b/resource.h
@@ -40,6 +40,7 @@
 #define IDS_ELEVATED			  139
 #define IDS_INSTALLEDB_VERSION            140
 #define IDS_TRUSTSYNC_TOOLTIP             141
+#define IDS_DOWNLOAD_INCOMPLETE_EXIT      142
 
 // Dialogs
 
-- 
2.15.0

[PATCH setup 5/5] Limit the number of packages shown in the IDD_DOWNLOAD_ERROR listbox

[Ken Brown]

---
 download.cc | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/download.cc b/download.cc
index b059bf5..6e6d6e8 100644
--- a/download.cc
+++ b/download.cc
@@ -188,6 +188,7 @@ download_one (packagesource & pkgsource, HWND owner)
 
 static std::vector <packageversion> download_failures;
 static std::string download_warn_pkgs;
+static const int max_pkgs = 20;
 
 static INT_PTR CALLBACK
 download_error_proc (HWND h, UINT message, WPARAM wParam, LPARAM lParam)
@@ -224,12 +225,19 @@ query_download_errors (HINSTANCE h, HWND owner)
 {
   download_warn_pkgs = "";
   Log (LOG_PLAIN) << "The following package(s) had download errors:" << endLog;
+  int count = 0;
   for (std::vector <packageversion>::const_iterator i = download_failures.begin (); i != download_failures.end (); i++)
     {
       packageversion pv = *i;
       std::string pvs = pv.Name () + "-" + pv.Canonical_version ();
       Log (LOG_PLAIN) << "  " << pvs << endLog;
-      download_warn_pkgs += pvs + "\r\n";
+      if (count < max_pkgs)
+	download_warn_pkgs += pvs + "\r\n";
+      else if (count == max_pkgs)
+	download_warn_pkgs += "...and "
+	  + std::to_string (download_failures.size () - max_pkgs)
+	  + " more.";
+      count++;
     }
   return DialogBox (h, MAKEINTRESOURCE (IDD_DOWNLOAD_ERROR), owner,
 		    download_error_proc);
-- 
2.15.0

[PATCH setup 4/5] Query user after download error in interactive mode

[Ken Brown]

Instead of just giving the user a "Try again?" Yes/No choice that goes
to IDD_SITE on Yes, create a dialog IDD_DOWNLOAD_ERROR with the
following choices: 'Retry' (retry the download), 'Back' (return to
IDD_CHOOSE), 'Continue' (ignore the errors), or 'Cancel' (exit).

The dialog lists the packages that had download errors so that the
user can make an informed choice.

Users who liked the old behavior (IDD_SITE) can select Back twice.
---
 download.cc | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 res.rc      | 22 ++++++++++++++++++
 resource.h  |  2 ++
 3 files changed, 95 insertions(+), 5 deletions(-)

diff --git a/download.cc b/download.cc
index 841f680..b059bf5 100644
--- a/download.cc
+++ b/download.cc
@@ -25,6 +25,7 @@
 #include <stdio.h>
 #include <unistd.h>
 #include <process.h>
+#include <vector>
 
 #include "resource.h"
 #include "msg.h"
@@ -182,16 +183,65 @@ download_one (packagesource & pkgsource, HWND owner)
     }
   if (success)
     return 0;
-  /* FIXME: Do we want to note this? if so how? */
   return 1;
 }
 
+static std::vector <packageversion> download_failures;
+static std::string download_warn_pkgs;
+
+static INT_PTR CALLBACK
+download_error_proc (HWND h, UINT message, WPARAM wParam, LPARAM lParam)
+{
+  switch (message)
+    {
+    case WM_INITDIALOG:
+      eset (h, IDC_DOWNLOAD_EDIT, download_warn_pkgs);
+      SetFocus (GetDlgItem(h, IDRETRY));
+      return FALSE;
+
+    case WM_COMMAND:
+      switch (LOWORD (wParam))
+	{
+	case IDRETRY:
+	case IDC_BACK:
+	case IDIGNORE:
+	case IDABORT:
+	  EndDialog (h, LOWORD (wParam));
+	default:
+	  // Not reached.
+	  return 0;
+	}
+
+    default:
+      // Not handled.
+      return FALSE;
+    }
+  return TRUE;
+}
+
+static int
+query_download_errors (HINSTANCE h, HWND owner)
+{
+  download_warn_pkgs = "";
+  Log (LOG_PLAIN) << "The following package(s) had download errors:" << endLog;
+  for (std::vector <packageversion>::const_iterator i = download_failures.begin (); i != download_failures.end (); i++)
+    {
+      packageversion pv = *i;
+      std::string pvs = pv.Name () + "-" + pv.Canonical_version ();
+      Log (LOG_PLAIN) << "  " << pvs << endLog;
+      download_warn_pkgs += pvs + "\r\n";
+    }
+  return DialogBox (h, MAKEINTRESOURCE (IDD_DOWNLOAD_ERROR), owner,
+		    download_error_proc);
+}
+
 static int
 do_download_thread (HINSTANCE h, HWND owner)
 {
   int errors = 0;
   total_download_bytes = 0;
   total_download_bytes_sofar = 0;
+  download_failures.clear ();
 
   Progress.SetText1 ("Checking for packages to download...");
   Progress.SetText2 ("");
@@ -235,6 +285,8 @@ do_download_thread (HINSTANCE h, HWND owner)
 	  int e = 0;
 	  e += download_one (*version.source(), owner);
 	  errors += e;
+	  if (e)
+	    download_failures.push_back (version);
 #if 0
 	  if (e)
 	    pkg->action = ACTION_ERROR;
@@ -246,21 +298,35 @@ do_download_thread (HINSTANCE h, HWND owner)
     {
       // In unattended mode we retry the download, but not forever.
       static int retries = 5;
+      int rc;
       if (unattended_mode && --retries <= 0)
         {
 	  Log (LOG_PLAIN) << "download error in unattended_mode: out of retries" << endLog;
-	  Logger ().setExitMsg (IDS_INSTALL_INCOMPLETE);
-	  Logger ().exit (1);
+	  rc = IDABORT;
 	}
       else if (unattended_mode)
         {
 	  Log (LOG_PLAIN) << "download error in unattended_mode: " << retries
 	    << (retries > 1 ? " retries" : " retry") << " remaining." << endLog;
+	  rc = IDRETRY;
+	}
+      else
+	rc = query_download_errors (h, owner);
+      switch (rc)
+	{
+	case IDRETRY:
 	  Progress.SetActivateTask (WM_APP_START_DOWNLOAD);
 	  return IDD_INSTATUS;
+	case IDC_BACK:
+	  return IDD_CHOOSE;
+	case IDABORT:
+	  Logger ().setExitMsg (IDS_DOWNLOAD_INCOMPLETE_EXIT);
+	  Logger ().exit (1);
+	case IDIGNORE:
+	  break;
+	default:
+	  break;
 	}
-      else if (yesno (owner, IDS_DOWNLOAD_INCOMPLETE) == IDYES)
-	return IDD_SITE;
     }
 
   if (source == IDC_SOURCE_DOWNLOAD)
diff --git a/res.rc b/res.rc
index d1f0871..62fbe40 100644
--- a/res.rc
+++ b/res.rc
@@ -416,6 +416,28 @@ BEGIN
 
 END
 
+IDD_DOWNLOAD_ERROR DIALOG DISCARDABLE  0, 0, SETUP_STANDARD_DIALOG_DIMS
+STYLE DS_MODALFRAME | DS_CENTER | WS_POPUP | WS_CAPTION
+CAPTION "Download Incomplete"
+FONT 8, "MS Shell Dlg"
+BEGIN
+    ICON            IDI_WARNING,IDC_HEADICON,10,10
+    LTEXT           "The following package(s) had download errors:",
+                    IDC_STATIC,7,8,320,16
+    EDITTEXT        IDC_DOWNLOAD_EDIT,7,24,320,88,WS_VSCROLL |
+                    ES_LEFT | ES_MULTILINE | ES_READONLY |
+                    ES_AUTOVSCROLL
+    LTEXT           "Select 'Retry' to retry the download, "
+                    "'Back' to return to the package selection page, "
+                    "'Continue' to go on anyway (NOT RECOMMENDED), or "
+                    "'Cancel' to exit.",
+                    IDC_STATIC,7,120,320,24
+    DEFPUSHBUTTON   "&Retry",IDRETRY,45,150,50,15
+    PUSHBUTTON      "&Back",IDC_BACK,110,150,50,15
+    PUSHBUTTON      "&Continue",IDIGNORE,175,150,50,15
+    PUSHBUTTON      "Cancel",IDABORT,240,150,50,15
+END
+
 IDD_POSTINSTALL DIALOG DISCARDABLE  0, 0, SETUP_STANDARD_DIALOG_W, 142
 STYLE DS_MODALFRAME | DS_3DLOOK | DS_CENTER | WS_CHILD | WS_VISIBLE |
     WS_CAPTION | WS_SYSMENU
diff --git a/resource.h b/resource.h
index 98a4a0f..c04c13d 100644
--- a/resource.h
+++ b/resource.h
@@ -68,6 +68,7 @@
 #define IDD_DROPPED                       221
 #define IDD_POSTINSTALL                   222
 #define IDD_FILE_INUSE                    223
+#define IDD_DOWNLOAD_ERROR                224
 
 // Bitmaps
 
@@ -178,3 +179,4 @@
 #define IDC_FILE_INUSE_HELP               592
 #define IDC_NET_DIRECT_LEGACY             593
 #define IDC_CHOOSE_SYNC                   594
+#define IDC_DOWNLOAD_EDIT                 595
-- 
2.15.0

[PATCH setup 1/5] Just retry download after error in unattended mode

[Ken Brown]

After a download error, setup was going back to IDD_SITE.  This is
pointless in unattended mode, since no changes in the mirrors or
packages can be made.

Change misleading comment about retries in unattended mode; the Yes/No
dialog is not used in that case.
---
 download.cc | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/download.cc b/download.cc
index e561c24..f6aa6fc 100644
--- a/download.cc
+++ b/download.cc
@@ -244,11 +244,7 @@ do_download_thread (HINSTANCE h, HWND owner)
 
   if (errors)
     {
-      /* In unattended mode, all dialog boxes automatically get
-         answered with a Yes/OK/other positive response.  This
-	 means that if there's a download problem, setup will
-	 potentially retry forever if we don't take care to give
-	 up at some finite point.  */
+      // In unattended mode we retry the download, but not forever.
       static int retries = 4;
       if (unattended_mode && retries-- <= 0)
         {
@@ -260,7 +256,8 @@ do_download_thread (HINSTANCE h, HWND owner)
         {
 	  Log (LOG_PLAIN) << "download error in unattended_mode: " << retries
 	    << (retries > 1 ? " retries" : " retry") << " remaining." << endLog;
-	  return IDD_SITE;
+	  Progress.SetActivateTask (WM_APP_START_DOWNLOAD);
+	  return IDD_INSTATUS;
 	}
       else if (yesno (owner, IDS_DOWNLOAD_INCOMPLETE) == IDYES)
 	return IDD_SITE;
-- 
2.15.0

[PATCH setup 2/5] Fix off-by-one error in download retry report

[Ken Brown]

'retries' was decremented after it was tested but before it was
reported in the log, so the reported number was always 1 too low.
---
 download.cc | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/download.cc b/download.cc
index f6aa6fc..a430f7f 100644
--- a/download.cc
+++ b/download.cc
@@ -245,8 +245,8 @@ do_download_thread (HINSTANCE h, HWND owner)
   if (errors)
     {
       // In unattended mode we retry the download, but not forever.
-      static int retries = 4;
-      if (unattended_mode && retries-- <= 0)
+      static int retries = 5;
+      if (unattended_mode && --retries <= 0)
         {
 	  Log (LOG_PLAIN) << "download error in unattended_mode: out of retries" << endLog;
 	  Logger ().setExitMsg (IDS_INSTALL_INCOMPLETE);
-- 
2.15.0

Re: [PATCH setup 0/5] Improve behavior after download error, v2

[Jon Turney]

On 10/11/2017 14:43, Ken Brown wrote:
> Currently setup goes back to the mirror selection page after a
> download error if the user answers "Yes" to "Download incomplete.  Try
> again?".  The same happens in unattended mode until the retries have
> been exhausted.
> 
> And if the user answers "No", then installation continues, even though
> this can damage the user's installation.  For example, if a package is
> selected for reinstall but cannot be downloaded, it will be
> uninstalled.
> 
> This series of patches changes the behavior as follows:
> 
>   - In unattended mode, simply retry the download.
> 
>   - In interactive mode, pop up a dialog showing which packages had
>     download errors and giving the user the following options:
>      - Retry (retries download)
>      - Back (return to the package selection page)
>      - Continue, with a warning
>      - Cancel (exit)

Thanks.  This looks great.  Please apply to master.

> Ken Brown (5):
>    Just retry download after error in unattended mode
>    Fix off-by-one error in download retry report
>    Remove "Try again?" from exit message.
>    Query user after download error in interactive mode
>    Limit the number of packages shown in the IDD_DOWNLOAD_ERROR listbox

You can drop this last one.  I don't think you need to limit the list if 
we have a scrollable textbox to show it in.

I was just pointing out that the list could be large, because I was 
assuming use of a MessageBox(), but you've sidestepped that since you 
are using a custom dialog.