[PATCH setup 0/3] Setup download site list fixes

[PATCH setup 0/3] Setup download site list fixes

[Jon TURNEY]

A few patches to improve the way that the download site list works in setup.

I don't really like the heuristic approach taken in patch [3/3], which was 
discussed a bit in Nov 2010, but nothing better has emerged from my bikeshed in 
the meantime.

Jon TURNEY (3):
  Fix selecting sites added to the download site list box
  Correctly make displayed_url for non-FQDNs
  Add the last element of URL path to site chooser, if interesting.

 site.cc | 36 +++++++++++++++++++++++++++++++-----
 1 file changed, 31 insertions(+), 5 deletions(-)

-- 
1.8.5.5

[PATCH setup 1/3] Fix selecting sites added to the download site list box

[Jon TURNEY]

Fix selecting sites added to the download site list box which have the same
protocol and hostname as an offical cygwin mirror.

If I add the site http://mirrors.kernel.org/sources.redhat.com/cygwinports/ to
setup's mirror list, using the GUI or --site option, I get two indistinguishable
entries named http://mirrors.kernel.org in the mirror list box.

Because PopulateListBox() uses the displayed string in the list control to
locate the itesm to select, we end up with a random one of those two
indistinguishable entries selected (usually the previously existing one).

Even if you manually correct the selection, the same problem will re-occur when
the selected sites are saved and restored on the next setup run.

Fix this by selecting by the index in the all_site_list vector, instead of by
displayed text.

2014-04-19 Jon TURNEY <jon.turney@dronecode.org.uk>

	* site.cc (PopulateListBox): Select listbox items by finding the
	index of the item with a matching full URL, not by LB_FINDSTRING
	which does an inexact match on the displayed URL.

Signed-off-by: Jon TURNEY <jon.turney@dronecode.org.uk>
---
 site.cc | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/site.cc b/site.cc
index 2ed5d7b..e7d4113 100644
--- a/site.cc
+++ b/site.cc
@@ -637,10 +637,12 @@ SitePage::PopulateListBox ()
   for (SiteList::const_iterator n = site_list.begin ();
        n != site_list.end (); ++n)
     {
-      int index = SendMessage (listbox, LB_FINDSTRING, (WPARAM) - 1,
-			       (LPARAM) n->displayed_url.c_str());
-      if (index != LB_ERR)
-	{
+      SiteList::iterator i = find (all_site_list.begin(),
+                                   all_site_list.end(), *n);
+      if (i != all_site_list.end())
+        {
+          int index = i - all_site_list.begin();
+
 	  // Highlight the selected item
 	  SendMessage (listbox, LB_SELITEMRANGE, TRUE, (index << 16) | index);
 	  // Make sure it's fully visible
-- 
1.8.5.5

[PATCH setup 2/3] Correctly make displayed_url for non-FQDNs

[Jon TURNEY]

Fix the logic for identifying protocol and site name part of the URL to find the
first '/' after a '//', rather than the first '/' after a '.', to handle
sitenames which aren't FQDNs correctly

This makes non-FQDNs added to the mirror list display correctly.

2010-11-26  Jon TURNEY  <jon.turney@dronecode.org.uk>

     * site.cc (init): Handle sitenames which aren't FQDNs correctly.

Signed-off-by: Jon TURNEY <jon.turney@dronecode.org.uk>
---
 site.cc | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/site.cc b/site.cc
index e7d4113..48ec0aa 100644
--- a/site.cc
+++ b/site.cc
@@ -150,8 +150,11 @@ site_list_type::init (const string &_url, const string &_servername,
     url.append("/");
 
   /* displayed_url is protocol and site name part of url */
-  displayed_url = url.substr (0, url.find ("/", url.find (".")));
+  string::size_type path_offset = url.find ("/", url.find ("//") + 2);
+  displayed_url = url.substr(0, path_offset);
 
+  /* the sorting key is hostname components in reverse order (to sort by country code)
+     plus the url (to ensure uniqueness) */
   key = string();
   string::size_type last_idx = displayed_url.length () - 1;
   string::size_type idx = url.find_last_of("./", last_idx);
-- 
1.8.5.5

[PATCH setup 3/3] Add the last element of URL path to site chooser, if interesting.

[Jon TURNEY]

If I add the site http://mirrors.kernel.org/sources.redhat.com/cygwinports/ to
setup's mirror list, using the GUI or --site option, I get two indistinguishable
entries named http://mirrors.kernel.org in the mirror list box.

So, to make the site chooser list entries more distinguishable, add the last
element of the URL path to the site chooser, if it exists and isn't 'cygwin' (or
some other alternatives used by current mirrors)

As Corinna pointed out in Nov 2010, there is still a corner case of URLs which
share protocol, hostname and the last element of the URL path being
indistinguishable.  Additionally, it will need updating for any new mirrors which
don't use one of the expected strings for the last path element in the URL.

2014-04-19  Jon TURNEY  <jon.turney@dronecode.org.uk>

	* site.cc (init): If interesting, show the last element
	of URL, as well as the protocol and sitename in the site chooser.

Signed-off-by: Jon TURNEY <jon.turney@dronecode.org.uk>
---
 site.cc | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/site.cc b/site.cc
index 48ec0aa..70f6303 100644
--- a/site.cc
+++ b/site.cc
@@ -173,6 +173,27 @@ site_list_type::init (const string &_url, const string &_servername,
       idx = 0;
   } while (idx > 0);
   key += url;
+
+  /* add last element of url if it exists, and isn't "cygwin" to displayed_url */
+  if (path_offset+1 < url.length())
+    {
+      string url_path = url.substr(path_offset+1);
+
+      /* trim any trailing / */
+      if (url_path.at(url_path.length()-1) == '/')
+        url_path.erase(url_path.length()-1);
+
+      /* add the last path element, if it exists, and isn't "cygwin"
+         (or some aliases used by existing sites) */
+      string::size_type pos = url_path.rfind('/');
+      string lpe = url_path.substr(pos+1);
+      if ((pos != string::npos) && (lpe.compare("cygwin") != 0) &&
+          (lpe.compare("cygwin.com") != 0) && (lpe.compare("cygwin32") != 0) &&
+          (lpe.compare("gnu-win32") != 0))
+        {
+          displayed_url.append(" (" + lpe +  ")");
+        }
+    }
 }
 
 site_list_type::site_list_type (const string &_url,
-- 
1.8.5.5

Re: [PATCH setup 3/3] Add the last element of URL path to site chooser, if interesting.

[Christopher Faylor]

On Mon, Apr 21, 2014 at 12:16:27PM +0100, Jon TURNEY wrote:
>If I add the site http://mirrors.kernel.org/sources.redhat.com/cygwinports/ to
>setup's mirror list, using the GUI or --site option, I get two indistinguishable
>entries named http://mirrors.kernel.org in the mirror list box.
>
>So, to make the site chooser list entries more distinguishable, add the last
>element of the URL path to the site chooser, if it exists and isn't 'cygwin' (or
>some other alternatives used by current mirrors)
>
>As Corinna pointed out in Nov 2010, there is still a corner case of URLs which
>share protocol, hostname and the last element of the URL path being
>indistinguishable.  Additionally, it will need updating for any new mirrors which
>don't use one of the expected strings for the last path element in the URL.
>
>2014-04-19  Jon TURNEY  <jon.turney@dronecode.org.uk>
>
>	* site.cc (init): If interesting, show the last element
>	of URL, as well as the protocol and sitename in the site chooser.
>
>Signed-off-by: Jon TURNEY <jon.turney@dronecode.org.uk>
>---
> site.cc | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
>
>diff --git a/site.cc b/site.cc
>index 48ec0aa..70f6303 100644
>--- a/site.cc
>+++ b/site.cc
>@@ -173,6 +173,27 @@ site_list_type::init (const string &_url, const string &_servername,
>       idx = 0;
>   } while (idx > 0);
>   key += url;
>+
>+  /* add last element of url if it exists, and isn't "cygwin" to displayed_url */
>+  if (path_offset+1 < url.length())
>+    {
>+      string url_path = url.substr(path_offset+1);
>+
>+      /* trim any trailing / */
>+      if (url_path.at(url_path.length()-1) == '/')
>+        url_path.erase(url_path.length()-1);
>+
>+      /* add the last path element, if it exists, and isn't "cygwin"
>+         (or some aliases used by existing sites) */
>+      string::size_type pos = url_path.rfind('/');
>+      string lpe = url_path.substr(pos+1);
>+      if ((pos != string::npos) && (lpe.compare("cygwin") != 0) &&
>+          (lpe.compare("cygwin.com") != 0) && (lpe.compare("cygwin32") != 0) &&
>+          (lpe.compare("gnu-win32") != 0))
>+        {
>+          displayed_url.append(" (" + lpe +  ")");
>+        }
>+    }
> }

I'd actually be ok with just displaying the whole URL even if it ends with Cygwin.
Is there a reason not to do that?

cgf

Re: [PATCH setup 3/3] Add the last element of URL path to site chooser, if interesting.

[Jon TURNEY]

On 21/04/2014 15:10, Christopher Faylor wrote:
> On Mon, Apr 21, 2014 at 12:16:27PM +0100, Jon TURNEY wrote:
>> If I add the site http://mirrors.kernel.org/sources.redhat.com/cygwinports/ to
>> setup's mirror list, using the GUI or --site option, I get two indistinguishable
>> entries named http://mirrors.kernel.org in the mirror list box.
>>
>> So, to make the site chooser list entries more distinguishable, add the last
>> element of the URL path to the site chooser, if it exists and isn't 'cygwin' (or
>> some other alternatives used by current mirrors)
>>
>> As Corinna pointed out in Nov 2010, there is still a corner case of URLs which
>> share protocol, hostname and the last element of the URL path being
>> indistinguishable.  Additionally, it will need updating for any new mirrors which
>> don't use one of the expected strings for the last path element in the URL.
> 
> I'd actually be ok with just displaying the whole URL even if it ends with Cygwin.
> Is there a reason not to do that?

Last time this was discussed [1], I think you felt that the current site list
and dialog box is too small to display the full URL usefully.

[1] http://cygwin.com/ml/cygwin-apps/2010-11/msg00134.html

Re: [PATCH setup 3/3] Add the last element of URL path to site chooser, if interesting.

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 1752 bytes --]

On Apr 21 19:12, Jon TURNEY wrote:
> On 21/04/2014 15:10, Christopher Faylor wrote:
> > On Mon, Apr 21, 2014 at 12:16:27PM +0100, Jon TURNEY wrote:
> >> If I add the site http://mirrors.kernel.org/sources.redhat.com/cygwinports/ to
> >> setup's mirror list, using the GUI or --site option, I get two indistinguishable
> >> entries named http://mirrors.kernel.org in the mirror list box.
> >>
> >> So, to make the site chooser list entries more distinguishable, add the last
> >> element of the URL path to the site chooser, if it exists and isn't 'cygwin' (or
> >> some other alternatives used by current mirrors)
> >>
> >> As Corinna pointed out in Nov 2010, there is still a corner case of URLs which
> >> share protocol, hostname and the last element of the URL path being
> >> indistinguishable.  Additionally, it will need updating for any new mirrors which
> >> don't use one of the expected strings for the last path element in the URL.
> > 
> > I'd actually be ok with just displaying the whole URL even if it ends with Cygwin.
> > Is there a reason not to do that?
> 
> Last time this was discussed [1], I think you felt that the current site list
> and dialog box is too small to display the full URL usefully.
> 
> [1] http://cygwin.com/ml/cygwin-apps/2010-11/msg00134.html

The dialog has still a lot of space to the left and right of the URL
list window.  We could resize it to take the full available space even
in the smallest dialog size.  I think that would be sufficient most
of the time.  And the dialog can be resized if it doesn't suffice.


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH setup 3/3] Add the last element of URL path to site chooser, if interesting.

[Christopher Faylor]

On Tue, Apr 22, 2014 at 09:53:15AM +0200, Corinna Vinschen wrote:
>On Apr 21 19:12, Jon TURNEY wrote:
>> On 21/04/2014 15:10, Christopher Faylor wrote:
>> > On Mon, Apr 21, 2014 at 12:16:27PM +0100, Jon TURNEY wrote:
>> >> If I add the site http://mirrors.kernel.org/sources.redhat.com/cygwinports/ to
>> >> setup's mirror list, using the GUI or --site option, I get two indistinguishable
>> >> entries named http://mirrors.kernel.org in the mirror list box.
>> >>
>> >> So, to make the site chooser list entries more distinguishable, add the last
>> >> element of the URL path to the site chooser, if it exists and isn't 'cygwin' (or
>> >> some other alternatives used by current mirrors)
>> >>
>> >> As Corinna pointed out in Nov 2010, there is still a corner case of URLs which
>> >> share protocol, hostname and the last element of the URL path being
>> >> indistinguishable.  Additionally, it will need updating for any new mirrors which
>> >> don't use one of the expected strings for the last path element in the URL.
>> > 
>> > I'd actually be ok with just displaying the whole URL even if it ends with Cygwin.
>> > Is there a reason not to do that?
>> 
>> Last time this was discussed [1], I think you felt that the current site list
>> and dialog box is too small to display the full URL usefully.
>> 
>> [1] http://cygwin.com/ml/cygwin-apps/2010-11/msg00134.html
>
>The dialog has still a lot of space to the left and right of the URL
>list window.  We could resize it to take the full available space even
>in the smallest dialog size.  I think that would be sufficient most
>of the time.  And the dialog can be resized if it doesn't suffice.

I agree.  I was going to suggest resizing (maybe I did before too).
We'd like to get a new version of setup.exe released soon.  I think
these changes would be nice to have in it.  Any chance you could do
that Jon?

cgf

Re: [PATCH setup 3/3] Add the last element of URL path to site chooser, if interesting.

[Jon TURNEY]

On 22/04/2014 15:47, Christopher Faylor wrote:
> On Tue, Apr 22, 2014 at 09:53:15AM +0200, Corinna Vinschen wrote:
>> On Apr 21 19:12, Jon TURNEY wrote:
>>> On 21/04/2014 15:10, Christopher Faylor wrote:
>>>> On Mon, Apr 21, 2014 at 12:16:27PM +0100, Jon TURNEY wrote:
>>>>> If I add the site http://mirrors.kernel.org/sources.redhat.com/cygwinports/ to
>>>>> setup's mirror list, using the GUI or --site option, I get two indistinguishable
>>>>> entries named http://mirrors.kernel.org in the mirror list box.
>>>>>
>>>>> So, to make the site chooser list entries more distinguishable, add the last
>>>>> element of the URL path to the site chooser, if it exists and isn't 'cygwin' (or
>>>>> some other alternatives used by current mirrors)
>>>>>
>>>>> As Corinna pointed out in Nov 2010, there is still a corner case of URLs which
>>>>> share protocol, hostname and the last element of the URL path being
>>>>> indistinguishable.  Additionally, it will need updating for any new mirrors which
>>>>> don't use one of the expected strings for the last path element in the URL.
>>>>
>>>> I'd actually be ok with just displaying the whole URL even if it ends with Cygwin.
>>>> Is there a reason not to do that?
>>>
>>> Last time this was discussed [1], I think you felt that the current site list
>>> and dialog box is too small to display the full URL usefully.
>>>
>>> [1] http://cygwin.com/ml/cygwin-apps/2010-11/msg00134.html
>>
>> The dialog has still a lot of space to the left and right of the URL
>> list window.  We could resize it to take the full available space even
>> in the smallest dialog size.  I think that would be sufficient most
>> of the time.  And the dialog can be resized if it doesn't suffice.
> 
> I agree.  I was going to suggest resizing (maybe I did before too).
> We'd like to get a new version of setup.exe released soon.  I think
> these changes would be nice to have in it.  Any chance you could do
> that Jon?

Sure, I'll take another look when I can.

per IRC [1/3] and [2/3] applied, [3/3] withdrawn.