[PATCH setup v3 0/6] Distinguish between user URLs and cygwin mirrors in UI

[PATCH setup v3 0/6] Distinguish between user URLs and cygwin mirrors in UI

[Ken Brown]

This is a followup to

  https://cygwin.com/ml/cygwin-apps/2011-04/msg00014.html,

in which Jon suggested splitting site selection into two pages, one
for cygwin.com mirrors and one for additional user URLs.  The latter
would be visible only if the user had previously checked a suitable
checkbox.  This patch series implements that suggestion.

The page for mirrors shows the area and location of each mirror, as
suggested by Brian Inglis, but it still uses truncated URLs as before.

[Brian, see the last patch, which is in your name.  Feel free to
improve it.]

The page for user URLs displays the full URL of each site to guarantee
that there is no ambiguity.

Brian Inglis (1):
  Display area and location of mirrors, and add these to the sort key

Ken Brown (5):
  Use the IDD_SITE dialog for cygwin.com mirrors only
  Use SitePage for cygwin.com mirrors only
  Create new page UserSitePage for user URLs
  Display full URLs in the user site list
  Keep the mirror list sorted properly

 Makefile.am |   2 +
 main.cc     |   4 +
 res.rc      |  31 +++++++-
 resource.h  |   3 +
 site.cc     | 243 +++++++++++++++++++++++++++++++++++-------------------------
 site.h      |   4 +-
 usersite.cc | 242 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 usersite.h  |  43 +++++++++++
 8 files changed, 465 insertions(+), 107 deletions(-)
 create mode 100644 usersite.cc
 create mode 100644 usersite.h

-- 
2.15.1

[PATCH setup v3 1/6] Use the IDD_SITE dialog for cygwin.com mirrors only

[Ken Brown]

Remove controls related to adding a user URL.  Add a checkbox
IDC_ALLOW_USER_URL; checking this sets a new boolean variable
allow_user_url.
---
 res.rc     | 14 ++++++--------
 resource.h |  1 +
 site.cc    | 39 ++++-----------------------------------
 3 files changed, 11 insertions(+), 43 deletions(-)

diff --git a/res.rc b/res.rc
index a4d7e70..ffc4722 100644
--- a/res.rc
+++ b/res.rc
@@ -131,25 +131,23 @@ IDD_SITE DIALOG DISCARDABLE  0, 0, SETUP_STANDARD_DIALOG_DIMS
 STYLE DS_MODALFRAME | DS_3DLOOK | DS_CENTER | WS_CHILD | WS_VISIBLE | 
     WS_CAPTION | WS_SYSMENU
 EXSTYLE WS_EX_CONTROLPARENT
-CAPTION "Cygwin Setup - Choose Download Site(s)"
+CAPTION "Cygwin Setup - Choose Mirror(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 | 
                     LBS_EXTENDEDSEL | WS_VSCROLL | WS_HSCROLL | WS_GROUP | 
                     WS_TABSTOP
-    LTEXT           "Available Download Sites:",IDC_STATIC,66,34,183,8,NOT 
+    LTEXT           "Available Mirrors:",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",
+    LTEXT           "Choose a cygwin.com mirror from this list",
                     IDC_STATIC,21,9,239,16,NOT WS_GROUP
-    LTEXT           "Choose A Download Site",IDC_STATIC_HEADER_TITLE,7,0,258,
+    LTEXT           "Choose A Mirror",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
+    CONTROL         "Allow sites that are not cygwin.com mirrors (to be chosen on the next page)",
+                    IDC_ALLOW_USER_URL,"Button",BS_AUTOCHECKBOX,15,162,350,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 a945d28..1b04343 100644
--- a/site.cc
+++ b/site.cc
@@ -61,9 +61,7 @@ enum
  */
 static ControlAdjuster::ControlInfo SiteControlsInfo[] = {
   {IDC_URL_LIST, 		CP_STRETCH, CP_STRETCH},
-  {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 +76,7 @@ SitePage::SitePage ()
 
 using namespace std;
 
+bool allow_user_url;
 bool cache_is_usable;
 bool cache_needs_writing;
 string cache_warn_urls;
@@ -677,13 +676,6 @@ bool SitePage::OnMessageCmd (int id, HWND hwndctl, UINT code)
 {
   switch (id)
     {
-    case IDC_EDIT_USER_URL:
-      {
-	// Set the default pushbutton to ADD if the user is entering text.
-	if (code == EN_CHANGE)
-	  SendMessage (GetHWND (), DM_SETDEFID, (WPARAM) IDC_BUTTON_ADD_URL, 0);
-	break;
-      }
     case IDC_URL_LIST:
       {
 	if (code == LBN_SELCHANGE)
@@ -693,33 +685,10 @@ bool SitePage::OnMessageCmd (int id, HWND hwndctl, UINT code)
 	  }
 	break;
       }
-    case IDC_BUTTON_ADD_URL:
+    case IDC_ALLOW_USER_URL:
       {
 	if (code == BN_CLICKED)
-	  {
-	    // User pushed the Add button.
-	    std::string other_url = egetString (GetHWND (), IDC_EDIT_USER_URL);
-	    if (other_url.size())
-	    {
-	    site_list_type newsite (other_url, "", "", "", false);
-	    SiteList::iterator i = find (all_site_list.begin(),
-					 all_site_list.end(), newsite);
-	    if (i == all_site_list.end())
-	      {
-		all_site_list.push_back (newsite);
-		Log (LOG_BABBLE) << "Adding site: " << other_url << endLog;
-		site_list.push_back (newsite);
-	      }
-	    else
-	      site_list.push_back (*i);
-
-	    // Update the list box.
-	    PopulateListBox ();
-	    // And allow the user to continue
-	    CheckControlsAndDisableAccordingly ();
-	    eset (GetHWND (), IDC_EDIT_USER_URL, "");
-	    }
-	  }
+	  allow_user_url = IsButtonChecked (IDC_ALLOW_USER_URL);
 	break;
       }
     default:
-- 
2.15.1

[PATCH setup v3 4/6] Display full URLs in the user site list

[Ken Brown]

---
 site.cc | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/site.cc b/site.cc
index accc264..98dd756 100644
--- a/site.cc
+++ b/site.cc
@@ -187,6 +187,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)
-- 
2.15.1

[PATCH setup v3 3/6] Create new page UserSitePage for user URLs

[Ken Brown]

This is done in two new files, usersite.h and usersite.cc, based on
site.h and site.cc.  The new page is activated after the mirror
selection page if IDC_ALLOW_USER_URL is checked.
---
 Makefile.am |   2 +
 main.cc     |   4 +
 res.rc      |  25 +++++++
 resource.h  |   2 +
 site.cc     |  24 +++---
 usersite.cc | 242 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 usersite.h  |  43 +++++++++++
 7 files changed, 329 insertions(+), 13 deletions(-)
 create mode 100644 usersite.cc
 create mode 100644 usersite.h

diff --git a/Makefile.am b/Makefile.am
index a238d88..a0b0450 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -258,6 +258,8 @@ inilint_SOURCES = \
 	threebar.h \
 	UserSettings.cc \
 	UserSettings.h \
+	usersite.cc \
+	usersite.h \
 	win32.cc \
 	win32.h \
 	window.cc \
diff --git a/main.cc b/main.cc
index b44f9b6..a01fb43 100644
--- a/main.cc
+++ b/main.cc
@@ -53,6 +53,7 @@
 #include "localdir.h"
 #include "net.h"
 #include "site.h"
+#include "usersite.h"
 #include "choose.h"
 #include "prereq.h"
 #include "threebar.h"
@@ -133,6 +134,7 @@ main_display ()
   LocalDirPage LocalDir;
   NetPage Net;
   SitePage Site;
+  UserSitePage UserSite;
   ChooserPage Chooser;
   PrereqPage Prereq;
   DesktopSetupPage Desktop;
@@ -173,6 +175,7 @@ main_display ()
   LocalDir.Create ();
   Net.Create ();
   Site.Create ();
+  UserSite.Create ();
   Chooser.Create ();
   Prereq.Create ();
   Progress.Create ();
@@ -187,6 +190,7 @@ main_display ()
   MainWindow.AddPage (&LocalDir);
   MainWindow.AddPage (&Net);
   MainWindow.AddPage (&Site);
+  MainWindow.AddPage (&UserSite);
   MainWindow.AddPage (&Chooser);
   MainWindow.AddPage (&Prereq);
   MainWindow.AddPage (&Progress);
diff --git a/res.rc b/res.rc
index ffc4722..d982e9d 100644
--- a/res.rc
+++ b/res.rc
@@ -150,6 +150,31 @@ BEGIN
                     IDC_ALLOW_USER_URL,"Button",BS_AUTOCHECKBOX,15,162,350,8
 END
 
+IDD_USERSITE DIALOG DISCARDABLE  0, 0, SETUP_STANDARD_DIALOG_DIMS
+STYLE DS_MODALFRAME | DS_3DLOOK | DS_CENTER | WS_CHILD | WS_VISIBLE |
+    WS_CAPTION | WS_SYSMENU
+EXSTYLE WS_EX_CONTROLPARENT
+CAPTION "Cygwin Setup - Choose Additional Download Site(s)"
+FONT 8, "MS Shell Dlg"
+BEGIN
+    ICON            IDI_CYGWIN,IDC_HEADICON,SETUP_HEADICON_X,0,21,20
+    LISTBOX         IDC_USER_URL_LIST,66,45,185,110,LBS_NOINTEGRALHEIGHT |
+                    LBS_EXTENDEDSEL | WS_VSCROLL | WS_HSCROLL | WS_GROUP |
+                    WS_TABSTOP
+    LTEXT           "Known 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 additional 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
+END
+
 IDD_NET DIALOG DISCARDABLE  0, 0, SETUP_STANDARD_DIALOG_DIMS
 STYLE DS_MODALFRAME | DS_CENTER | WS_CHILD | WS_CAPTION | WS_SYSMENU
 CAPTION "Cygwin Setup - Select Connection Type"
diff --git a/resource.h b/resource.h
index d111853..27449f9 100644
--- a/resource.h
+++ b/resource.h
@@ -67,6 +67,7 @@
 #define IDD_POSTINSTALL                   222
 #define IDD_FILE_INUSE                    223
 #define IDD_DOWNLOAD_ERROR                224
+#define IDD_USERSITE                      225
 
 // Bitmaps
 
@@ -178,3 +179,4 @@
 #define IDC_NET_DIRECT_LEGACY             593
 #define IDC_DOWNLOAD_EDIT                 594
 #define IDC_ALLOW_USER_URL                595
+#define IDC_USER_URL_LIST                 596
diff --git a/site.cc b/site.cc
index 85ae70c..accc264 100644
--- a/site.cc
+++ b/site.cc
@@ -642,10 +642,10 @@ SitePage::OnNext ()
        n != selected_mirror_list.end (); ++n)
     Log (LOG_PLAIN) << "site: " << n->url << endLog;
 
+  if (allow_user_url)
+    return IDD_USERSITE;
   Progress.SetActivateTask (WM_APP_START_SETUP_INI_DOWNLOAD);
   return IDD_INSTATUS;
-
-  return 0;
 }
 
 long
@@ -662,12 +662,9 @@ SitePage::OnBack ()
 void
 SitePage::OnActivate ()
 {
-  // Fill the list box with all known sites.
+  // Fill the list box with all known mirrors.
   PopulateListBox ();
 
-  // Load the user URL box with nothing - it is in the list already.
-  eset (GetHWND (), IDC_EDIT_USER_URL, "");
-
   // Get the enabled/disabled states of the controls set accordingly.
   CheckControlsAndDisableAccordingly ();
 }
@@ -675,7 +672,8 @@ SitePage::OnActivate ()
 long
 SitePage::OnUnattended ()
 {
-  if (SendMessage (GetDlgItem (IDC_URL_LIST), LB_GETSELCOUNT, 0, 0) > 0)
+  if (allow_user_url
+      || SendMessage (GetDlgItem (IDC_URL_LIST), LB_GETSELCOUNT, 0, 0) > 0)
     return OnNext ();
   else
     return -2;
@@ -686,12 +684,11 @@ SitePage::CheckControlsAndDisableAccordingly () const
 {
   DWORD ButtonFlags = PSWIZB_BACK;
 
-  // Check that at least one download site is selected.
-  if (SendMessage (GetDlgItem (IDC_URL_LIST), LB_GETSELCOUNT, 0, 0) > 0)
-    {
-      // At least one site selected, enable "Next".
-      ButtonFlags |= PSWIZB_NEXT;
-    }
+  // Enable Next if at least one mirror is selected or if we'll be
+  // going to the user URL page.
+  if (allow_user_url
+      || SendMessage (GetDlgItem (IDC_URL_LIST), LB_GETSELCOUNT, 0, 0) > 0)
+    ButtonFlags |= PSWIZB_NEXT;
   GetOwner ()->SetButtons (ButtonFlags);
 }
 
@@ -752,6 +749,7 @@ bool SitePage::OnMessageCmd (int id, HWND hwndctl, UINT code)
 		selected_usersite_list.clear ();
 		site_list = selected_mirror_list;
 	      }
+	    CheckControlsAndDisableAccordingly ();
 	  }
 	break;
       }
diff --git a/usersite.cc b/usersite.cc
new file mode 100644
index 0000000..34fc097
--- /dev/null
+++ b/usersite.cc
@@ -0,0 +1,242 @@
+/*
+ * Copyright (c) 2017, Ken Brown
+ *
+ *     This program is free software; you can redistribute it and/or modify
+ *     it under the terms of the GNU General Public License as published by
+ *     the Free Software Foundation; either version 2 of the License, or
+ *     (at your option) any later version.
+ *
+ *     A copy of the GNU General Public License can be found at
+ *     http://www.gnu.org/
+ *
+ * Based on site.cc, written by DJ Delorie <dj@cygnus.com>
+ *
+ */
+
+/* The purpose of this file is to let the user choose download sites
+   that are not cygwin.com mirrors.  */
+
+#include <string>
+#include <algorithm>
+
+#include "usersite.h"
+#include "site.h"
+#include "win32.h"
+#include "dialog.h"
+#include "resource.h"
+#include "state.h"
+#include "LogSingleton.h"
+#include "io_stream.h"
+#include "propsheet.h"
+#include "threebar.h"
+#include "ControlAdjuster.h"
+
+using namespace std;
+
+extern ThreeBarProgressPage Progress;
+
+/*
+  Sizing information.
+ */
+static ControlAdjuster::ControlInfo UserSiteControlsInfo[] = {
+  {IDC_USER_URL_LIST, 		CP_STRETCH, CP_STRETCH},
+  {IDC_EDIT_USER_URL,		CP_STRETCH, CP_BOTTOM},
+  {IDC_BUTTON_ADD_URL,		CP_RIGHT,   CP_BOTTOM},
+  {IDC_SITE_USERURL,            CP_LEFT,    CP_BOTTOM},
+  {0, CP_LEFT, CP_TOP}
+};
+
+UserSitePage::UserSitePage ()
+{
+  sizeProcessor.AddControlInfo (UserSiteControlsInfo);
+}
+
+using namespace std;
+
+/* Defined in site.cc */
+extern SiteList all_usersite_list;
+extern SiteList site_list;
+extern SiteList selected_mirror_list;
+extern SiteList selected_usersite_list;
+
+static void
+save_dialog (HWND h)
+{
+  // Remove anything that was previously in the selected usersite list.
+  selected_usersite_list.clear ();
+
+  HWND listbox = GetDlgItem (h, IDC_USER_URL_LIST);
+  int sel_count = SendMessage (listbox, LB_GETSELCOUNT, 0, 0);
+  if (sel_count > 0)
+    {
+      int sel_buffer[sel_count];
+      SendMessage (listbox, LB_GETSELITEMS, sel_count, (LPARAM) sel_buffer);
+      for (int n = 0; n < sel_count; n++)
+	{
+	  int mirror =
+	    SendMessage (listbox, LB_GETITEMDATA, sel_buffer[n], 0);
+	  selected_usersite_list.push_back (all_usersite_list[mirror]);
+	}
+    }
+  site_list = selected_mirror_list;
+  site_list.insert (site_list.end (), selected_usersite_list.begin (),
+		    selected_usersite_list.end ());
+}
+
+bool UserSitePage::Create ()
+{
+  return PropertyPage::Create (IDD_USERSITE);
+}
+
+long
+UserSitePage::OnNext ()
+{
+  HWND h = GetHWND ();
+
+  save_dialog (h);
+
+  // Log all the selected URLs from the list.
+  for (SiteList::const_iterator n = selected_usersite_list.begin ();
+       n != selected_usersite_list.end (); ++n)
+    Log (LOG_PLAIN) << "site: " << n->url << endLog;
+
+  Progress.SetActivateTask (WM_APP_START_SETUP_INI_DOWNLOAD);
+  return IDD_INSTATUS;
+}
+
+long
+UserSitePage::OnBack ()
+{
+  HWND h = GetHWND ();
+
+  save_dialog (h);
+
+  // Go back to the mirror selection page
+  return 0;
+}
+
+void
+UserSitePage::OnActivate ()
+{
+  // Fill the list box with all known sites.
+  PopulateListBox ();
+
+  // Load the user URL box with nothing - it is in the list already.
+  eset (GetHWND (), IDC_EDIT_USER_URL, "");
+
+  // Get the enabled/disabled states of the controls set accordingly.
+  CheckControlsAndDisableAccordingly ();
+}
+
+long
+UserSitePage::OnUnattended ()
+{
+  if (site_list.size ())
+    return OnNext ();
+  else
+    return -2;
+}
+
+void
+UserSitePage::CheckControlsAndDisableAccordingly () const
+{
+  DWORD ButtonFlags = PSWIZB_BACK;
+
+  // Enable Next if at least one download site is selected.
+  if (selected_mirror_list.size ()
+      || SendMessage (GetDlgItem (IDC_USER_URL_LIST), LB_GETSELCOUNT, 0, 0) > 0)
+    ButtonFlags |= PSWIZB_NEXT;
+  GetOwner ()->SetButtons (ButtonFlags);
+}
+
+void
+UserSitePage::PopulateListBox ()
+{
+  int j;
+  HWND listbox = GetDlgItem (IDC_USER_URL_LIST);
+
+  // Populate the list box with the URLs.
+  SendMessage (listbox, LB_RESETCONTENT, 0, 0);
+  for (SiteList::const_iterator i = all_usersite_list.begin ();
+       i != all_usersite_list.end (); ++i)
+    {
+      j = SendMessage (listbox, LB_ADDSTRING, 0,
+		       (LPARAM) i->displayed_url.c_str());
+      SendMessage (listbox, LB_SETITEMDATA, j, j);
+    }
+
+  // Select the selected ones.
+  for (SiteList::const_iterator n = selected_usersite_list.begin ();
+       n != selected_usersite_list.end (); ++n)
+    {
+      SiteList::iterator i = find (all_usersite_list.begin(),
+                                   all_usersite_list.end(), *n);
+      if (i != all_usersite_list.end())
+        {
+          int index = i - all_usersite_list.begin();
+
+	  // Highlight the selected item
+	  SendMessage (listbox, LB_SELITEMRANGE, TRUE, (index << 16) | index);
+	  // Make sure it's fully visible
+	  SendMessage (listbox, LB_SETCARETINDEX, index, FALSE);
+	}
+    }
+}
+
+bool UserSitePage::OnMessageCmd (int id, HWND hwndctl, UINT code)
+{
+  switch (id)
+    {
+    case IDC_EDIT_USER_URL:
+      {
+	// Set the default pushbutton to ADD if the user is entering text.
+	if (code == EN_CHANGE)
+	  SendMessage (GetHWND (), DM_SETDEFID, (WPARAM) IDC_BUTTON_ADD_URL, 0);
+	break;
+      }
+    case IDC_USER_URL_LIST:
+      {
+	if (code == LBN_SELCHANGE)
+	  {
+	    CheckControlsAndDisableAccordingly ();
+	    save_dialog (GetHWND ());
+	  }
+	break;
+      }
+    case IDC_BUTTON_ADD_URL:
+      {
+	if (code == BN_CLICKED)
+	  {
+	    // User pushed the Add button.
+	    std::string other_url = egetString (GetHWND (), IDC_EDIT_USER_URL);
+	    if (other_url.size())
+	    {
+	    site_list_type newsite (other_url, "", "", "", false);
+	    SiteList::iterator i = find (all_usersite_list.begin(),
+					 all_usersite_list.end(), newsite);
+	    if (i == all_usersite_list.end())
+	      {
+		all_usersite_list.push_back (newsite);
+		Log (LOG_BABBLE) << "Adding site: " << other_url << endLog;
+		selected_usersite_list.push_back (newsite);
+	      }
+	    else
+	      selected_usersite_list.push_back (*i);
+
+	    // Update the list box.
+	    PopulateListBox ();
+	    // And allow the user to continue
+	    CheckControlsAndDisableAccordingly ();
+	    eset (GetHWND (), IDC_EDIT_USER_URL, "");
+	    }
+	  }
+	break;
+      }
+    default:
+      // Wasn't recognized or handled.
+      return false;
+    }
+
+  // Was handled since we never got to default above.
+  return true;
+}
diff --git a/usersite.h b/usersite.h
new file mode 100644
index 0000000..842a0b0
--- /dev/null
+++ b/usersite.h
@@ -0,0 +1,43 @@
+/*
+ * Copyright (c) 2017, Ken Brown
+ *
+ *     This program is free software; you can redistribute it and/or modify
+ *     it under the terms of the GNU General Public License as published by
+ *     the Free Software Foundation; either version 2 of the License, or
+ *     (at your option) any later version.
+ *
+ *     A copy of the GNU General Public License can be found at
+ *     http://www.gnu.org/
+ *
+ * Based on site.h, written by Robert Collins <rbtcollins@hotmail.com>
+ *
+ */
+
+#ifndef SETUP_USERSITE_H
+#define SETUP_USERSITE_H
+
+#include "proppage.h"
+
+// For user URLs that are not mirrors of cygwin.com.
+class UserSitePage : public PropertyPage
+{
+public:
+  UserSitePage ();
+  virtual ~ UserSitePage ()
+  {
+  };
+
+  bool Create ();
+
+  virtual void OnActivate ();
+  virtual long OnNext ();
+  virtual long OnBack ();
+  virtual long OnUnattended ();
+
+  virtual bool OnMessageCmd (int id, HWND hwndctl, UINT code);
+
+  void PopulateListBox();
+  void CheckControlsAndDisableAccordingly () const;
+};
+
+#endif /* SETUP_USERSITE_H */
-- 
2.15.1

[PATCH setup v3 2/6] Use SitePage for cygwin.com mirrors only

[Ken Brown]

Keep separate lists of mirrors and user URLs.  Rename several SiteList
variables to indicate that they now contain only mirrors.

Introduce a helper function remove_user_urls() to move non-mirrors
from the initial list of sites to the list of user URLs.

Set the initial state of allow_user_url depending on whether there are
any saved sites that are not mirrors.

Implement the appropriate action if the user unchecks
IDC_ALLOW_USER_URL.

As an unrelated bit of cleanup, define a helper function merge_site()
to avoid repeating the same code several times.
---
 site.cc | 172 ++++++++++++++++++++++++++++++++++++++++++++--------------------
 site.h  |   4 +-
 2 files changed, 120 insertions(+), 56 deletions(-)

diff --git a/site.cc b/site.cc
index 1b04343..85ae70c 100644
--- a/site.cc
+++ b/site.cc
@@ -84,14 +84,21 @@ string cache_warn_urls;
 /* Selected sites */
 SiteList site_list;
 
-/* Fresh mirrors + selected sites */
-SiteList all_site_list;
+/* Fresh mirrors + selected stale mirrors */
+SiteList all_mirror_list;
+
+/* Selected mirrors */
+SiteList selected_mirror_list;
 
 /* Previously fresh + cached before */
-SiteList cached_site_list;
+SiteList cached_mirror_list;
+
+/* Stale selected mirrors to warn about and add to cache */
+SiteList dropped_mirror_list;
 
-/* Stale selected sites to warn about and add to cache */
-SiteList dropped_site_list;
+/* User URLs */
+SiteList all_usersite_list;
+SiteList selected_usersite_list;
 
 StringArrayOption SiteOption('s', "site", "Download site");
 
@@ -221,8 +228,8 @@ site_list_type::operator < (site_list_type const &rhs) const
 static void
 save_dialog (HWND h)
 {
-  // Remove anything that was previously in the selected site list.
-  site_list.clear ();
+  // Remove anything that was previously in the selected mirror list.
+  selected_mirror_list.clear ();
 
   HWND listbox = GetDlgItem (h, IDC_URL_LIST);
   int sel_count = SendMessage (listbox, LB_GETSELCOUNT, 0, 0);
@@ -234,9 +241,23 @@ save_dialog (HWND h)
 	{
 	  int mirror =
 	    SendMessage (listbox, LB_GETITEMDATA, sel_buffer[n], 0);
-	  site_list.push_back (all_site_list[mirror]);
+	  selected_mirror_list.push_back (all_mirror_list[mirror]);
 	}
     }
+  site_list = selected_mirror_list;
+  if (allow_user_url)
+    site_list.insert (site_list.end (), selected_usersite_list.begin (),
+		      selected_usersite_list.end ());
+}
+
+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
@@ -295,13 +316,7 @@ load_site_list (SiteList& theSites, char *theString)
 	  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;
-	    }
+	    merge_site (theSites, newsite);
 	  else
 	    //TODO: remove and remerge 
 	    *i = newsite;
@@ -313,6 +328,49 @@ load_site_list (SiteList& theSites, char *theString)
     }
 }
 
+static void
+remove_user_urls ()
+{
+  selected_mirror_list. clear ();
+  selected_usersite_list.clear ();
+  SiteList result;
+  for (SiteList::iterator i = all_mirror_list.begin ();
+       i != all_mirror_list.end (); i++)
+    {
+      bool mirror = false;
+      SiteList::iterator j;
+      if (i->from_mirrors_lst)
+	mirror = true;
+      else if ((j = find (cached_mirror_list.begin (), cached_mirror_list.end (), *i))
+	       != cached_mirror_list.end ())
+	{
+	  // It's a stale mirror.
+	  mirror = true;
+	  // Copy mirror info from cached version.
+	  i->servername = j->servername;
+	  i->area = j->area;
+	  i->location = j->location;
+	  // i->from_mirrors_lst stays false so we still know that *i is stale.
+	  i->displayed_url = j->displayed_url;
+	  i->key = j->key;
+	}
+      if (mirror)
+	{
+	  result.push_back (*i);
+	  if (find (site_list.begin (), site_list.end (), *i) != site_list.end ())
+	    selected_mirror_list.push_back (*i);
+	}
+      else
+	{
+	  merge_site (all_usersite_list, *i);
+	  if (find (site_list.begin (), site_list.end (), *i) != site_list.end ())
+	    selected_usersite_list.push_back (*i);
+	}
+    }
+  all_mirror_list = result;
+  allow_user_url = selected_usersite_list.size ();
+}
+
 static int
 get_site_list (HINSTANCE h, HWND owner)
 {
@@ -357,12 +415,14 @@ get_site_list (HINSTANCE h, HWND owner)
   theMirrorString = new_cstr_char_array (mirrors);
   theCachedString = new_cstr_char_array (cached_mirrors);
 
-  load_site_list (all_site_list, theMirrorString);
-  load_site_list (cached_site_list, theCachedString);
+  load_site_list (all_mirror_list, theMirrorString);
+  load_site_list (cached_mirror_list, theCachedString);
   
   delete[] theMirrorString;
   delete[] theCachedString;
 
+  remove_user_urls ();
+
   return 0;
 }
 
@@ -379,9 +439,9 @@ void
 SiteSetting::registerSavedSite (const char * site)
 {
   site_list_type tempSite(site, "", "", "", false);
-  SiteList::iterator i = find (all_site_list.begin(),
-			       all_site_list.end(), tempSite);
-  if (i == all_site_list.end())
+  SiteList::iterator i = find (all_mirror_list.begin(),
+			       all_mirror_list.end(), tempSite);
+  if (i == all_mirror_list.end())
     {
       /* Don't default to certain machines if they suffer
 	 from bandwidth limitations. */
@@ -389,11 +449,7 @@ 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_mirror_list, tempSite);
       site_list.push_back (tempSite);
     }
   else
@@ -501,26 +557,20 @@ drop_proc (HWND h, UINT message, WPARAM wParam, LPARAM lParam)
 int check_dropped_mirrors (HWND h)
 {
   cache_warn_urls = "";
-  dropped_site_list.clear ();
+  dropped_mirror_list.clear ();
 
-  for (SiteList::const_iterator n = site_list.begin ();
-       n != site_list.end (); ++n)
+  for (SiteList::const_iterator n = selected_mirror_list.begin ();
+       n != selected_mirror_list.end (); ++n)
     {
-      SiteList::iterator i = find (all_site_list.begin(), all_site_list.end(),
+      SiteList::iterator i = find (all_mirror_list.begin(), all_mirror_list.end(),
 				   *n);
-      if (i == all_site_list.end() || !i->from_mirrors_lst)
+      if (i == all_mirror_list.end () || !i->from_mirrors_lst)
 	{
-	  SiteList::iterator j = find (cached_site_list.begin(),
-				       cached_site_list.end(), *n);
-	  if (j != cached_site_list.end())
-	    {
-	      Log (LOG_PLAIN) << "Dropped selected mirror: " << n->url
-		  << endLog;
-	      dropped_site_list.push_back (*j);
-	      if (cache_warn_urls.size())
-		cache_warn_urls += "\r\n";
-	      cache_warn_urls += i->url;
-	    }
+	  Log (LOG_PLAIN) << "Dropped selected mirror: " << n->url << endLog;
+	  dropped_mirror_list.push_back (*i);
+	  if (cache_warn_urls.size())
+	    cache_warn_urls += "\r\n";
+	  cache_warn_urls += i->url;
 	}
     }
   if (cache_warn_urls.size())
@@ -549,13 +599,13 @@ void save_cache_file (int cache_action)
   io_stream *f = UserSettings::instance().open ("mirrors-lst");
   if (f)
     {
-      write_cache_list (f, all_site_list);
+      write_cache_list (f, all_mirror_list);
       if (cache_action == CACHE_ACCEPT_WARN)
 	{
 	  Log (LOG_PLAIN) << "Adding dropped mirrors to cache to warn again."
 	      << endLog;
 	  *f << "# Following mirrors re-added by setup.exe to warn again about dropped urls.";
-	  write_cache_list (f, dropped_site_list);
+	  write_cache_list (f, dropped_mirror_list);
 	}
       delete f;
     }
@@ -566,6 +616,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 ()
 {
@@ -581,8 +638,8 @@ SitePage::OnNext ()
     save_cache_file (cache_action);
 
   // Log all the selected URLs from the list.
-  for (SiteList::const_iterator n = site_list.begin ();
-       n != site_list.end (); ++n)
+  for (SiteList::const_iterator n = selected_mirror_list.begin ();
+       n != selected_mirror_list.end (); ++n)
     Log (LOG_PLAIN) << "site: " << n->url << endLog;
 
   Progress.SetActivateTask (WM_APP_START_SETUP_INI_DOWNLOAD);
@@ -646,8 +703,8 @@ SitePage::PopulateListBox ()
 
   // Populate the list box with the URLs.
   SendMessage (listbox, LB_RESETCONTENT, 0, 0);
-  for (SiteList::const_iterator i = all_site_list.begin ();
-       i != all_site_list.end (); ++i)
+  for (SiteList::const_iterator i = all_mirror_list.begin ();
+       i != all_mirror_list.end (); ++i)
     {
       j = SendMessage (listbox, LB_ADDSTRING, 0,
 		       (LPARAM) i->displayed_url.c_str());
@@ -655,14 +712,14 @@ SitePage::PopulateListBox ()
     }
 
   // Select the selected ones.
-  for (SiteList::const_iterator n = site_list.begin ();
-       n != site_list.end (); ++n)
+  for (SiteList::const_iterator n = selected_mirror_list.begin ();
+       n != selected_mirror_list.end (); ++n)
     {
-      SiteList::iterator i = find (all_site_list.begin(),
-                                   all_site_list.end(), *n);
-      if (i != all_site_list.end())
+      SiteList::iterator i = find (all_mirror_list.begin(),
+                                   all_mirror_list.end(), *n);
+      if (i != all_mirror_list.end())
         {
-          int index = i - all_site_list.begin();
+          int index = i - all_mirror_list.begin();
 
 	  // Highlight the selected item
 	  SendMessage (listbox, LB_SELITEMRANGE, TRUE, (index << 16) | index);
@@ -688,7 +745,14 @@ bool SitePage::OnMessageCmd (int id, HWND hwndctl, UINT code)
     case IDC_ALLOW_USER_URL:
       {
 	if (code == BN_CLICKED)
-	  allow_user_url = IsButtonChecked (IDC_ALLOW_USER_URL);
+	  {
+	    allow_user_url = IsButtonChecked (IDC_ALLOW_USER_URL);
+	    if (!allow_user_url)
+	      {
+		selected_usersite_list.clear ();
+		site_list = selected_mirror_list;
+	      }
+	  }
 	break;
       }
     default:
diff --git a/site.h b/site.h
index d16db8e..0b85caf 100644
--- a/site.h
+++ b/site.h
@@ -21,6 +21,7 @@
 
 #include "proppage.h"
 
+// For mirrors of cygwin.com.
 class SitePage : public PropertyPage
 {
 public:
@@ -31,6 +32,7 @@ public:
 
   bool Create ();
 
+  virtual void OnInit ();
   virtual void OnActivate ();
   virtual long OnNext ();
   virtual long OnBack ();
@@ -75,8 +77,6 @@ typedef std::vector <site_list_type> SiteList;
 
 /* user chosen sites */
 extern SiteList site_list;
-/* potential sites */
-extern SiteList all_site_list;
 
 class SiteSetting
 {
-- 
2.15.1

[PATCH setup v3 5/6] Keep the mirror list sorted properly

[Ken Brown]

When site.cc:load_site_list() added a mirror whose URL was already in
the list, it put the new entry in the same position as the old.
Remove the old entry and merge the new one instead.
---
 site.cc | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/site.cc b/site.cc
index 98dd756..1dd42d8 100644
--- a/site.cc
+++ b/site.cc
@@ -321,11 +321,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())
-	    merge_site (theSites, newsite);
-	  else
-	    //TODO: remove and remerge 
-	    *i = newsite;
+	  if (i != theSites.end ())
+	    theSites.erase (i);
+	  merge_site (theSites, newsite);
 	}
         else
         {
-- 
2.15.1

[PATCH setup v3 6/6] Display area and location of mirrors, and add these to the sort key

[Ken Brown]

From: Brian Inglis <Brian.Inglis@SystematicSw.ab.ca>

Also change site_list_type::operator== to compare URLs rather than
keys, since the key no longer depends only on the URL.
---
 site.cc | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/site.cc b/site.cc
index 1dd42d8..72e8e80 100644
--- a/site.cc
+++ b/site.cc
@@ -187,8 +187,14 @@ 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)
+  /* Display the area and location of a mirror; display the full URL
+     (without the trailing slash) of a non-mirror.  */
+  if (from_mirrors_lst)
+    {
+      displayed_url = area + " - " + location + " - " + displayed_url;
+      key = area + " " + location + " " + displayed_url;
+    }
+  else
     {
       displayed_url = url;
       displayed_url.erase (displayed_url.end () - 1);
@@ -222,7 +228,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
-- 
2.15.1

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

[Brian Inglis]

On 2017-12-06 13:45, Ken Brown wrote:
> This is a followup to
>   https://cygwin.com/ml/cygwin-apps/2011-04/msg00014.html,
> in which Jon suggested splitting site selection into two pages, one
> for cygwin.com mirrors and one for additional user URLs.  The latter
> would be visible only if the user had previously checked a suitable
> checkbox.  This patch series implements that suggestion.
> The page for mirrors shows the area and location of each mirror, as
> suggested by Brian Inglis, but it still uses truncated URLs as before.
> [Brian, see the last patch, which is in your name.  Feel free to
> improve it.]
> Brian Inglis (1):
>   Display area and location of mirrors, and add these to the sort key

I had added Last mirror and User added descriptive prefixes to the displayed
URLs, and uniqueness checks, as you suggested and we discussed, but then saw
your patches, and have been holding off until those land.

Should I still look at adding those prefixes once I can pull the updated sources?

-- 
Take care. Thanks, Brian Inglis, Calgary, Alberta, Canada

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

[Ken Brown]

On 12/6/2017 4:07 PM, Brian Inglis wrote:
> On 2017-12-06 13:45, Ken Brown wrote:
>> This is a followup to
>>    https://cygwin.com/ml/cygwin-apps/2011-04/msg00014.html,
>> in which Jon suggested splitting site selection into two pages, one
>> for cygwin.com mirrors and one for additional user URLs.  The latter
>> would be visible only if the user had previously checked a suitable
>> checkbox.  This patch series implements that suggestion.
>> The page for mirrors shows the area and location of each mirror, as
>> suggested by Brian Inglis, but it still uses truncated URLs as before.
>> [Brian, see the last patch, which is in your name.  Feel free to
>> improve it.]
>> Brian Inglis (1):
>>    Display area and location of mirrors, and add these to the sort key
> 
> I had added Last mirror and User added descriptive prefixes to the displayed
> URLs, and uniqueness checks, as you suggested and we discussed, but then saw
> your patches, and have been holding off until those land.
> 
> Should I still look at adding those prefixes once I can pull the updated sources?

I suggest that we wait and see what happens with my patches.  Then we 
can discuss what further improvements can be made.

Ken

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

[Brian Inglis]

On 2017-12-06 14:55, Ken Brown wrote:
> On 12/6/2017 4:07 PM, Brian Inglis wrote:
>> On 2017-12-06 13:45, Ken Brown wrote:
>>> This is a followup to
>>>    https://cygwin.com/ml/cygwin-apps/2011-04/msg00014.html,
>>> in which Jon suggested splitting site selection into two pages, one
>>> for cygwin.com mirrors and one for additional user URLs.  The latter
>>> would be visible only if the user had previously checked a suitable
>>> checkbox.  This patch series implements that suggestion.
>>> The page for mirrors shows the area and location of each mirror, as 
>>> suggested by Brian Inglis, but it still uses truncated URLs as before. 
>>> [Brian, see the last patch, which is in your name.  Feel free to improve
>>> it.]
>>>    Display area and location of mirrors, and add these to the sort key
>> I had added Last mirror and User added descriptive prefixes to the
>> displayed URLs, and uniqueness checks, as you suggested and we discussed,
>> but then saw your patches, and have been holding off until those land.
>> Should I still look at adding those prefixes once I can pull the updated sources?
> I suggest that we wait and see what happens with my patches. Then we can 
> discuss what further improvements can be made.
OK by me - I prefer not dealing with code conflicts.

-- 
Take care. Thanks, Brian Inglis, Calgary, Alberta, Canada

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

[Jon Turney]

On 06/12/2017 22:15, Brian Inglis wrote:
> On 2017-12-06 14:55, Ken Brown wrote:
>> On 12/6/2017 4:07 PM, Brian Inglis wrote:
>>> On 2017-12-06 13:45, Ken Brown wrote:
>>>> This is a followup to
>>>>     https://cygwin.com/ml/cygwin-apps/2011-04/msg00014.html,
>>>> in which Jon suggested splitting site selection into two pages, one
>>>> for cygwin.com mirrors and one for additional user URLs.  The latter
>>>> would be visible only if the user had previously checked a suitable
>>>> checkbox.  This patch series implements that suggestion.

Thanks very much for looking at this.

This looks pretty good.

My main concern is how this deals with private cygwin mirrors.  I guess 
you need to not select anything at the "choose a mirror page", then tick 
"allow sites that are not cygwin.com mirrors" to get to the page that 
lets you enter the URL, which is somewhat confusing.

I'm going to guess that private cygwin mirrors are used more than repos 
of additional packages.

(Another facet to consider: At the moment, I think we do something like 
trying all the keys we know to validate the signature of every setup.ini 
we download. It would be nice to maintain an idea of which URLs are 
supposed to be validated using the cygwin signing key.)

I guess we could achieve this by keeping the 'Add URL' control on the 
mirrors page, but then the migration becomes a big problem (as we don't 
know if a URL is a mirror or not until we read it)

(I'm not sure migrating anything not in mirrors.lst to the usersite list 
as you currently do is the best idea, as it will migrate the currently 
selected mirror if it happens to be stale when we are run?)

>>>> The page for mirrors shows the area and location of each mirror, as
>>>> suggested by Brian Inglis, but it still uses truncated URLs as before.
>>>> [Brian, see the last patch, which is in your name.  Feel free to improve
>>>> it.]
>>>>     Display area and location of mirrors, and add these to the sort key
>>> I had added Last mirror and User added descriptive prefixes to the
>>> displayed URLs, and uniqueness checks, as you suggested and we discussed,
>>> but then saw your patches, and have been holding off until those land.
>>> Should I still look at adding those prefixes once I can pull the updated sources?
>> I suggest that we wait and see what happens with my patches. Then we can
>> discuss what further improvements can be made.
> OK by me - I prefer not dealing with code conflicts.

A question about scheduling:  I think I'd like to make a new setup 
release in the next week or so, to pick up changes accumulated so far, 
and perhaps also cherry-picking some of the non-libsolv related changes 
which have been accumulating in the topic/libsolv branch.

Too soon? Should I wait for this change to be ready? Any other changes 
you'd like to get in?

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

[Ken Brown]

On 12/7/2017 8:55 AM, Jon Turney wrote:
> On 06/12/2017 22:15, Brian Inglis wrote:
>> On 2017-12-06 14:55, Ken Brown wrote:
>>> On 12/6/2017 4:07 PM, Brian Inglis wrote:
>>>> On 2017-12-06 13:45, Ken Brown wrote:
>>>>> This is a followup to
>>>>>     https://cygwin.com/ml/cygwin-apps/2011-04/msg00014.html,
>>>>> in which Jon suggested splitting site selection into two pages, one
>>>>> for cygwin.com mirrors and one for additional user URLs.  The latter
>>>>> would be visible only if the user had previously checked a suitable
>>>>> checkbox.  This patch series implements that suggestion.
> 
> Thanks very much for looking at this.
> 
> This looks pretty good.
> 
> My main concern is how this deals with private cygwin mirrors.  I guess 
> you need to not select anything at the "choose a mirror page", then tick 
> "allow sites that are not cygwin.com mirrors" to get to the page that 
> lets you enter the URL, which is somewhat confusing.
> I'm going to guess that private cygwin mirrors are used more than repos 
> of additional packages.

I hadn't thought about that.  I'll try to think of different terminology 
to make it clear that the first page is for mirrors listed on 
https://cygwin.com/mirrors.html, and the second page is for private 
mirrors and other repos of packages.  I'll try to send v4 with these 
changes later today and you can see if it looks better.

> (Another facet to consider: At the moment, I think we do something like 
> trying all the keys we know to validate the signature of every setup.ini 
> we download. It would be nice to maintain an idea of which URLs are 
> supposed to be validated using the cygwin signing key.)

After my patches, I think 'all_mirror_list' is precisely the list of 
such URLs.  Maybe I should rename it to 'official_mirror_list' or 
something similar?

> I guess we could achieve this by keeping the 'Add URL' control on the 
> mirrors page, but then the migration becomes a big problem (as we don't 
> know if a URL is a mirror or not until we read it)
> 
> (I'm not sure migrating anything not in mirrors.lst to the usersite list 
> as you currently do is the best idea, as it will migrate the currently 
> selected mirror if it happens to be stale when we are run?)

I tried to handle stale mirrors correctly in this snippet from patch 2:

+      if (i->from_mirrors_lst)
+	mirror = true;
+      else if ((j = find (cached_mirror_list.begin (), 
cached_mirror_list.end (), *i))
+	       != cached_mirror_list.end ())
+	{
+	  // It's a stale mirror.
+	  mirror = true;
[...]
+      if (mirror)
+	{
+	  result.push_back (*i);
[...]
+      else
+	{
+	  merge_site (all_usersite_list, *i);

> A question about scheduling:  I think I'd like to make a new setup 
> release in the next week or so, to pick up changes accumulated so far, 
> and perhaps also cherry-picking some of the non-libsolv related changes 
> which have been accumulating in the topic/libsolv branch.
> 
> Too soon? Should I wait for this change to be ready? Any other changes 
> you'd like to get in?

This change is pretty invasive and needs more testing and more feedback 
from others.  I think you should go ahead with the release and save this 
change for the next release.

The only other thing I can think of that's still pending is the attempt 
to make Enter do the right thing in the package chooser textbox:

   https://sourceware.org/ml/cygwin-apps/2017-12/msg00024.html

Ken

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

[Jon Turney]

On 07/12/2017 15:07, Ken Brown wrote:
> On 12/7/2017 8:55 AM, Jon Turney wrote:
>> On 06/12/2017 22:15, Brian Inglis wrote:
>>> On 2017-12-06 14:55, Ken Brown wrote:
>>>> On 12/6/2017 4:07 PM, Brian Inglis wrote:
>>>>> On 2017-12-06 13:45, Ken Brown wrote:
>>>>>> This is a followup to
>>>>>>     https://cygwin.com/ml/cygwin-apps/2011-04/msg00014.html,
>>>>>> in which Jon suggested splitting site selection into two pages, one
>>>>>> for cygwin.com mirrors and one for additional user URLs.  The latter
>>>>>> would be visible only if the user had previously checked a suitable
>>>>>> checkbox.  This patch series implements that suggestion.
>>
>> Thanks very much for looking at this.
>>
>> This looks pretty good.
>>
>> My main concern is how this deals with private cygwin mirrors.  I 
>> guess you need to not select anything at the "choose a mirror page", 
>> then tick "allow sites that are not cygwin.com mirrors" to get to the 
>> page that lets you enter the URL, which is somewhat confusing.
>> I'm going to guess that private cygwin mirrors are used more than 
>> repos of additional packages.
> 
> I hadn't thought about that.  I'll try to think of different terminology 
> to make it clear that the first page is for mirrors listed on 
> https://cygwin.com/mirrors.html, and the second page is for private 
> mirrors and other repos of packages.  I'll try to send v4 with these 

Hmm... I don't think that's perhaps the best direction to be heading in.

> changes later today and you can see if it looks better.
> 
>> (Another facet to consider: At the moment, I think we do something 
>> like trying all the keys we know to validate the signature of every 
>> setup.ini we download. It would be nice to maintain an idea of which 
>> URLs are supposed to be validated using the cygwin signing key.)
> 
> After my patches, I think 'all_mirror_list' is precisely the list of 
> such URLs.  Maybe I should rename it to 'official_mirror_list' or 
> something similar?

No, because private cygwin mirrors (by which I just mean a mirror whose 
URL isn't published in mirrors.lst) can't be in that list.

>> I guess we could achieve this by keeping the 'Add URL' control on the 
>> mirrors page, but then the migration becomes a big problem (as we 
>> don't know if a URL is a mirror or not until we read it)
>>
>> (I'm not sure migrating anything not in mirrors.lst to the usersite 
>> list as you currently do is the best idea, as it will migrate the 
>> currently selected mirror if it happens to be stale when we are run?)
> 
> I tried to handle stale mirrors correctly in this snippet from patch 2:
> 
> +      if (i->from_mirrors_lst)
> +    mirror = true;
> +      else if ((j = find (cached_mirror_list.begin (), 
> cached_mirror_list.end (), *i))
> +           != cached_mirror_list.end ())
> +    {
> +      // It's a stale mirror.
> +      mirror = true;
> [...]
> +      if (mirror)
> +    {
> +      result.push_back (*i);
> [...]
> +      else
> +    {
> +      merge_site (all_usersite_list, *i);

Yes, you are right.

This whole thing of "remember the mirror list from last time so we can 
see if this site came from it", rather than explicitly remembering that 
for the site is a bit odd, though.

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

[Achim Gratz]

Jon Turney writes:
>> After my patches, I think 'all_mirror_list' is precisely the list of
>> such URLs.  Maybe I should rename it to 'official_mirror_list' or
>> something similar?
>
> No, because private cygwin mirrors (by which I just mean a mirror
> whose URL isn't published in mirrors.lst) can't be in that list.

It might make sense to check everything that claims to "release: cygwin"
with the cygwin key and everything else with the extra keys provided.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Wavetables for the Terratec KOMPLEXER:
http://Synth.Stromeko.net/Downloads.html#KomplexerWaves

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

[Ken Brown]

On 12/7/2017 2:08 PM, Jon Turney wrote:
> On 07/12/2017 15:07, Ken Brown wrote:
>> On 12/7/2017 8:55 AM, Jon Turney wrote:
>>> On 06/12/2017 22:15, Brian Inglis wrote:
>>>> On 2017-12-06 14:55, Ken Brown wrote:
>>>>> On 12/6/2017 4:07 PM, Brian Inglis wrote:
>>>>>> On 2017-12-06 13:45, Ken Brown wrote:
>>>>>>> This is a followup to
>>>>>>>     https://cygwin.com/ml/cygwin-apps/2011-04/msg00014.html,
>>>>>>> in which Jon suggested splitting site selection into two pages, one
>>>>>>> for cygwin.com mirrors and one for additional user URLs.  The latter
>>>>>>> would be visible only if the user had previously checked a suitable
>>>>>>> checkbox.  This patch series implements that suggestion.
>>>
>>> Thanks very much for looking at this.
>>>
>>> This looks pretty good.
>>>
>>> My main concern is how this deals with private cygwin mirrors.  I 
>>> guess you need to not select anything at the "choose a mirror page", 
>>> then tick "allow sites that are not cygwin.com mirrors" to get to the 
>>> page that lets you enter the URL, which is somewhat confusing.
>>> I'm going to guess that private cygwin mirrors are used more than 
>>> repos of additional packages.
>>
>> I hadn't thought about that.  I'll try to think of different 
>> terminology to make it clear that the first page is for mirrors listed 
>> on https://cygwin.com/mirrors.html, and the second page is for private 
>> mirrors and other repos of packages.  I'll try to send v4 with these 
> 
> Hmm... I don't think that's perhaps the best direction to be heading in.
> 
>> changes later today and you can see if it looks better.
>>
>>> (Another facet to consider: At the moment, I think we do something 
>>> like trying all the keys we know to validate the signature of every 
>>> setup.ini we download. It would be nice to maintain an idea of which 
>>> URLs are supposed to be validated using the cygwin signing key.)
>>
>> After my patches, I think 'all_mirror_list' is precisely the list of 
>> such URLs.  Maybe I should rename it to 'official_mirror_list' or 
>> something similar?
> 
> No, because private cygwin mirrors (by which I just mean a mirror whose 
> URL isn't published in mirrors.lst) can't be in that list.

OK, I was missing the point.  You want cygwin mirrors, whether official 
or not, to be treated on the mirror selection page.  The second page 
should be for package repos that don't claim to be mirrors.  Is that 
right?  It makes a lot more sense than what I was trying to do.

>>> I guess we could achieve this by keeping the 'Add URL' control on the 
>>> mirrors page, but then the migration becomes a big problem (as we 
>>> don't know if a URL is a mirror or not until we read it)

How about this: We put the 'Add URL' control back on the mirrors page, 
but we make it clear that it's intended for cygwin mirrors.  When we 
later check signatures, we check such mirrors only with the cygwin 
signing key.  In case the user has turned off signature checking, we 
could follow Achim's suggestion of checking for "release: cygwin".

We also stop saving non-mirrors under "last-mirror" in setup.rc.  Maybe 
we need a new "last-user-site" setting for these.

> This whole thing of "remember the mirror list from last time so we can 
> see if this site came from it", rather than explicitly remembering that 
> for the site is a bit odd, though.

Agreed.

Ken

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

[Achim Gratz]

Ken Brown writes:
> How about this: We put the 'Add URL' control back on the mirrors page,
> but we make it clear that it's intended for cygwin mirrors.  When we
> later check signatures, we check such mirrors only with the cygwin
> signing key.  In case the user has turned off signature checking, we
> could follow Achim's suggestion of checking for "release: cygwin".

I think we'd need to change the machinations of parsing to pre-parse the
start of the file before checking the signature validity, though.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

SD adaptation for Waldorf microQ V2.22R2:
http://Synth.Stromeko.net/Downloads.html#WaldorfSDada