[PATCH setup v2 2/5] Set the initial state of IDC_ALLOW_USER_URL

[PATCH setup v2 2/5] Set the initial state of IDC_ALLOW_USER_URL

[Ken Brown]

It starts checked if all_site_list contains a site that is not a
cygwin.com mirror.  Such a site would have to come from the
"last-mirror" user setting, indicating that the user used a non-mirror
on the last run.  Introduce a new function check_for_user_urls() to
help with this.
---
 site.cc | 26 ++++++++++++++++++++++++++
 site.h  |  1 +
 2 files changed, 27 insertions(+)

diff --git a/site.cc b/site.cc
index 607c591..0222ec9 100644
--- a/site.cc
+++ b/site.cc
@@ -242,6 +242,23 @@ save_dialog (HWND h)
     }
 }
 
+// Does the_site_list contain any sites that are not mirrors (possibly stale)?
+static void
+check_for_user_urls ()
+{
+  for (SiteList::const_iterator i = all_site_list.begin ();
+       i != all_site_list.end (); i++)
+    {
+      if (!i->from_mirrors_lst
+	  && (find (cached_site_list.begin (), cached_site_list.end (), *i)
+	      == cached_site_list.end ()))
+	{
+	  allow_user_url = true;
+	  return;
+	}
+    }
+}
+
 // This is called only for lists of mirrors that came (now or in a
 // previous setup run) from mirrors.lst.
 void
@@ -366,6 +383,8 @@ get_site_list (HINSTANCE h, HWND owner)
   delete[] theMirrorString;
   delete[] theCachedString;
 
+  check_for_user_urls ();
+
   return 0;
 }
 
@@ -569,6 +588,13 @@ bool SitePage::Create ()
   return PropertyPage::Create (IDD_SITE);
 }
 
+void
+SitePage::OnInit ()
+{
+  int a = allow_user_url ? BST_CHECKED : BST_UNCHECKED;
+  CheckDlgButton (GetHWND (), IDC_ALLOW_USER_URL, a);
+}
+
 long
 SitePage::OnNext ()
 {
diff --git a/site.h b/site.h
index d16db8e..c517ac2 100644
--- a/site.h
+++ b/site.h
@@ -31,6 +31,7 @@ public:
 
   bool Create ();
 
+  virtual void OnInit ();
   virtual void OnActivate ();
   virtual long OnNext ();
   virtual long OnBack ();
-- 
2.15.1

[PATCH setup v2 3/5] Adjust site list if IDC_ALLOW_USER_URL is unchecked

[Ken Brown]

If the user unchecks the box, remove all sites except for fresh
mirrors and selected stale mirrors.  Introduce a new function
remove_user_urls() to help with this
---
 site.cc | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/site.cc b/site.cc
index 0222ec9..9b7b816 100644
--- a/site.cc
+++ b/site.cc
@@ -259,6 +259,35 @@ check_for_user_urls ()
     }
 }
 
+static void
+remove_user_urls ()
+{
+  SiteList result;
+  for (SiteList::const_iterator i = all_site_list.begin ();
+       i != all_site_list.end (); i++)
+    {
+      if (i->from_mirrors_lst)
+	// It's a fresh mirror.
+	result.push_back (*i);
+      else
+	{
+	  // Is it selected?
+	  SiteList::iterator j = find (site_list.begin (),
+				       site_list.end (), *i);
+	  if (j != site_list.end ())
+	    {
+	      if (find (cached_site_list.begin (), cached_site_list.end (), *i)
+		  != cached_site_list.end ())
+		// It's a selected stale mirror.
+		result.push_back (*i);
+	      else
+		site_list.erase (j);
+	    }
+	}
+    }
+  all_site_list = result;
+}
+
 // This is called only for lists of mirrors that came (now or in a
 // previous setup run) from mirrors.lst.
 void
@@ -757,6 +786,12 @@ bool SitePage::OnMessageCmd (int id, HWND hwndctl, UINT code)
 	if (code == BN_CLICKED)
 	  {
 	    allow_user_url = IsButtonChecked (IDC_ALLOW_USER_URL);
+	    if (!allow_user_url)
+	      {
+		// The button was just unchecked.
+		remove_user_urls ();
+		PopulateListBox ();
+	      }
 	    CheckControlsAndDisableAccordingly ();
 	  }
 	break;
-- 
2.15.1

[PATCH setup v2 5/5] Make user URLs sort last in the displayed site list

[Ken Brown]

Also fix load_site_list() so that when a mirror replaces an existing
entry, the sort order is updated.

Change site_list_type::operator== to compare URLs instead of sort
keys, since the key of a user URL no longer depends solely on the URL.

Introduce a new helper function merge_site().
---
 site.cc | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/site.cc b/site.cc
index 9972a29..641a6bb 100644
--- a/site.cc
+++ b/site.cc
@@ -183,11 +183,13 @@ site_list_type::site_list_type (const string &_url,
       idx = 0;
   } while (idx > 0);
   key += url;
-  /* Display the full URL (without trailing slash) of a non-mirror.  */
+  /* Display the full URL (without trailing slash) of non-mirrors, and
+     make them sort last.  */
   if (!from_mirrors_lst)
     {
       displayed_url = url;
       displayed_url.erase (displayed_url.end () - 1);
+      key = "zzzz " + key;
     }
 }
 
@@ -218,7 +220,7 @@ site_list_type::operator= (site_list_type const &rhs)
 bool
 site_list_type::operator == (site_list_type const &rhs) const
 {
-  return stricmp (key.c_str(), rhs.key.c_str()) == 0; 
+  return stricmp (url.c_str(), rhs.url.c_str()) == 0;
 }
 
 bool
@@ -306,6 +308,16 @@ remove_user_urls ()
   all_site_list = result;
 }
 
+static void
+merge_site (SiteList & sites, site_list_type newsite)
+{
+  SiteList result;
+  merge (sites.begin(), sites.end(),
+	 &newsite, &newsite + 1,
+	 inserter (result, result.begin()));
+  sites = result;
+}
+
 // This is called only for lists of mirrors that came (now or in a
 // previous setup run) from mirrors.lst.
 void
@@ -361,17 +373,9 @@ load_site_list (SiteList& theSites, char *theString)
 	  site_list_type newsite (bol, semi, semi2, semi3, true);
 	  SiteList::iterator i = find (theSites.begin(),
 				       theSites.end(), newsite);
-	  if (i == theSites.end())
-	    {
-	      SiteList result;
-	      merge (theSites.begin(), theSites.end(),
-		     &newsite, &newsite + 1,
-		     inserter (result, result.begin()));
-	      theSites = result;
-	    }
-	  else
-	    //TODO: remove and remerge 
-	    *i = newsite;
+	  if (i != theSites.end())
+	    theSites.erase (i);
+	  merge_site (theSites, newsite);
 	}
         else
         {
@@ -458,11 +462,8 @@ SiteSetting::registerSavedSite (const char * site)
 	  || strnicmp (site, NOSAVE2, NOSAVE2_LEN) == 0
 	  || strnicmp (site, NOSAVE3, NOSAVE3_LEN) == 0)
 	return;
-      SiteList result;
-      merge (all_site_list.begin(), all_site_list.end(),
-	     &tempSite, &tempSite + 1,
-	     inserter (result, result.begin()));
-      all_site_list = result;
+
+      merge_site (all_site_list, tempSite);
       site_list.push_back (tempSite);
     }
   else
-- 
2.15.1

[PATCH setup v2 1/5] Add checkbox IDC_ALLOW_USER_URL to IDD_SITE dialog

[Ken Brown]

Enable the "User URL" edit box and "Add" button only if
IDC_ALLOW_USER_URL is checked.
---
 res.rc     | 15 ++++++++-------
 resource.h |  1 +
 site.cc    | 15 +++++++++++++++
 3 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/res.rc b/res.rc
index a4d7e70..b5dcb3e 100644
--- a/res.rc
+++ b/res.rc
@@ -135,21 +135,22 @@ CAPTION "Cygwin Setup - Choose Download Site(s)"
 FONT 8, "MS Shell Dlg"
 BEGIN
     ICON            IDI_CYGWIN,IDC_HEADICON,SETUP_HEADICON_X,0,21,20
-    LISTBOX         IDC_URL_LIST,66,45,185,110,LBS_NOINTEGRALHEIGHT | 
+    LISTBOX         IDC_URL_LIST,66,34,185,110,LBS_NOINTEGRALHEIGHT |
                     LBS_EXTENDEDSEL | WS_VSCROLL | WS_HSCROLL | WS_GROUP | 
                     WS_TABSTOP
-    LTEXT           "Available Download Sites:",IDC_STATIC,66,34,183,8,NOT 
-                    WS_GROUP
     CONTROL         "",IDC_HEADSEPARATOR,"Static",SS_BLACKFRAME | SS_SUNKEN,0,28,
                     SETUP_STANDARD_DIALOG_W,1
     LTEXT           "Choose a site from this list, or add your own sites to the list",
                     IDC_STATIC,21,9,239,16,NOT WS_GROUP
     LTEXT           "Choose A Download Site",IDC_STATIC_HEADER_TITLE,7,0,258,
                     8,NOT WS_GROUP
-    EDITTEXT        IDC_EDIT_USER_URL,65,160,185,14,ES_AUTOHSCROLL | 
-                    WS_GROUP
-    LTEXT           "User URL:",IDC_SITE_USERURL,15,162,45,8,NOT WS_GROUP
-    PUSHBUTTON      "Add",IDC_BUTTON_ADD_URL,255,160,50,14
+    EDITTEXT        IDC_EDIT_USER_URL,65,165,185,14,ES_AUTOHSCROLL |
+                    WS_GROUP | WS_DISABLED
+    LTEXT           "User URL:",IDC_SITE_USERURL,15,167,45,8,NOT WS_GROUP |
+                    WS_DISABLED
+    PUSHBUTTON      "Add",IDC_BUTTON_ADD_URL,255,165,50,14,WS_DISABLED
+    CONTROL         "Allow sites that are not cygwin.com mirrors",
+                    IDC_ALLOW_USER_URL,"Button",BS_AUTOCHECKBOX,15,150,200,8
 END
 
 IDD_NET DIALOG DISCARDABLE  0, 0, SETUP_STANDARD_DIALOG_DIMS
diff --git a/resource.h b/resource.h
index a2e867f..d111853 100644
--- a/resource.h
+++ b/resource.h
@@ -177,3 +177,4 @@
 #define IDC_FILE_INUSE_HELP               592
 #define IDC_NET_DIRECT_LEGACY             593
 #define IDC_DOWNLOAD_EDIT                 594
+#define IDC_ALLOW_USER_URL                595
diff --git a/site.cc b/site.cc
index 230d82c..607c591 100644
--- a/site.cc
+++ b/site.cc
@@ -64,6 +64,7 @@ static ControlAdjuster::ControlInfo SiteControlsInfo[] = {
   {IDC_EDIT_USER_URL,		CP_STRETCH, CP_BOTTOM},
   {IDC_BUTTON_ADD_URL,		CP_RIGHT,   CP_BOTTOM},
   {IDC_SITE_USERURL,            CP_LEFT,    CP_BOTTOM},
+  {IDC_ALLOW_USER_URL,          CP_LEFT,    CP_BOTTOM},
   {0, CP_LEFT, CP_TOP}
 };
 
@@ -78,6 +79,7 @@ SitePage::SitePage ()
 
 using namespace std;
 
+bool allow_user_url;
 bool cache_is_usable;
 bool cache_needs_writing;
 string cache_warn_urls;
@@ -637,6 +639,10 @@ SitePage::CheckControlsAndDisableAccordingly () const
       ButtonFlags |= PSWIZB_NEXT;
     }
   GetOwner ()->SetButtons (ButtonFlags);
+
+  EnableWindow (GetDlgItem (IDC_EDIT_USER_URL), allow_user_url);
+  EnableWindow (GetDlgItem (IDC_SITE_USERURL), allow_user_url);
+  EnableWindow (GetDlgItem (IDC_BUTTON_ADD_URL), allow_user_url);
 }
 
 void
@@ -720,6 +726,15 @@ bool SitePage::OnMessageCmd (int id, HWND hwndctl, UINT code)
 	  }
 	break;
       }
+    case IDC_ALLOW_USER_URL:
+      {
+	if (code == BN_CLICKED)
+	  {
+	    allow_user_url = IsButtonChecked (IDC_ALLOW_USER_URL);
+	    CheckControlsAndDisableAccordingly ();
+	  }
+	break;
+      }
     default:
       // Wasn't recognized or handled.
       return false;
-- 
2.15.1

[PATCH setup v2 0/5] Distinguish between user URLs and cygwin mirrors in UI

[Ken Brown]

This is the same as the previous patch series with one cosmetic
improvement: I've now made user URLs sort last, so that the
distinction between user URLs and cygwin.com mirrors is visually more
clear.

Ken Brown (5):
  Add checkbox IDC_ALLOW_USER_URL to IDD_SITE dialog
  Set the initial state of IDC_ALLOW_USER_URL
  Adjust site list if IDC_ALLOW_USER_URL is unchecked
  Display full URL of non-mirrors in the site list
  Make user URLs sort last in the displayed site list

 res.rc     |  15 +++----
 resource.h |   1 +
 site.cc    | 129 +++++++++++++++++++++++++++++++++++++++++++++++++++++--------
 site.h     |   1 +
 4 files changed, 122 insertions(+), 24 deletions(-)

-- 
2.15.1

[PATCH setup v2 4/5] Display full URL of non-mirrors in the site list

[Ken Brown]

Set displayed_url to full URL in the site_list_type constructor if
from_mirrors_lst is false.  Modify check_for_user_urls() to change
displayed_url back to the short form if a site in all_site_list turns
out to be a stale mirror.
---
 site.cc | 32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/site.cc b/site.cc
index 9b7b816..9972a29 100644
--- a/site.cc
+++ b/site.cc
@@ -183,6 +183,12 @@ site_list_type::site_list_type (const string &_url,
       idx = 0;
   } while (idx > 0);
   key += url;
+  /* Display the full URL (without trailing slash) of a non-mirror.  */
+  if (!from_mirrors_lst)
+    {
+      displayed_url = url;
+      displayed_url.erase (displayed_url.end () - 1);
+    }
 }
 
 site_list_type::site_list_type (site_list_type const &rhs)
@@ -242,19 +248,31 @@ save_dialog (HWND h)
     }
 }
 
-// Does the_site_list contain any sites that are not mirrors (possibly stale)?
+// Set allow_user_url = true if all_site_list contains any sites that
+// are not mirrors (possibly stale).  At the same time, fixup the
+// stored information about stale mirrors.
 static void
 check_for_user_urls ()
 {
-  for (SiteList::const_iterator i = all_site_list.begin ();
+  for (SiteList::iterator i = all_site_list.begin ();
        i != all_site_list.end (); i++)
     {
-      if (!i->from_mirrors_lst
-	  && (find (cached_site_list.begin (), cached_site_list.end (), *i)
-	      == cached_site_list.end ()))
+      if (!i->from_mirrors_lst)
 	{
-	  allow_user_url = true;
-	  return;
+	  // Is it a stale mirror?
+	  SiteList::iterator j = find (cached_site_list.begin (),
+				       cached_site_list.end (), *i);
+	  if (j == cached_site_list.end ())
+	    allow_user_url = true;
+	  else
+	    {
+	      i->servername = j->servername;
+	      i->area = j->area;
+	      i->location = j->location;
+	      // i->from_mirrors_lst stays false.
+	      i->displayed_url = j->displayed_url;
+	      i->key = j->key;
+	    }
 	}
     }
 }
-- 
2.15.1

Re: [PATCH setup v2 0/5] Distinguish between user URLs and cygwin mirrors in UI

[Ken Brown]

On 12/4/2017 10:18 AM, Ken Brown wrote:
> This is the same as the previous patch series with one cosmetic
> improvement: I've now made user URLs sort last, so that the
> distinction between user URLs and cygwin.com mirrors is visually more
> clear.

Sorry for repeatedly responding to myself, but I think I see now how to 
build on what I've done and show user URLs on a separate page as Jon 
originally suggested.

If you want to hold off on reviewing this series of patches, I'll give 
that a try.

Ken

Re: [PATCH setup v2 0/5] Distinguish between user URLs and cygwin mirrors in UI

[Jon Turney]

On 04/12/2017 21:48, Ken Brown wrote:
> On 12/4/2017 10:18 AM, Ken Brown wrote:
>> This is the same as the previous patch series with one cosmetic
>> improvement: I've now made user URLs sort last, so that the
>> distinction between user URLs and cygwin.com mirrors is visually more
>> clear.
> 
> Sorry for repeatedly responding to myself, but I think I see now how to 
> build on what I've done and show user URLs on a separate page as Jon 
> originally suggested.

No problem, thanks for looking into this.

My suggestions are just that, I'm sure there are other and better ways 
of approaching this.