Re: [Patch] Setup: Display mirrors sorted by location

[Igor Pechtchanski <pechtcha at cs dot nyu dot edu>]

On Thu, 10 Nov 2005, Rajesh Balakrishnan wrote:

> The following patch will display the location info beside the download
> site.  e.g.
>
> North America, Virginia  ftp://mirrors.rcn.net
> North America, Virginia  http://mirrors.rcn.net
> North America, Wisconsin  ftp://sourceware.mirrors.tds.net
>
> Please let me know your comments.

Ok, here goes.  First, thanks for doing this.  However, if this is to be
done at all, let's do it right.

> Notes on the changes
> * The earlier setup sorts sites based on url.
>   This patch sorts on area, location too.

That's good, but the sort order should be user-selectable, not
unconditional as you have it.  In fact, why are you still using the
ListBox widget?  Why not a ListView (with sortable columns)?  MSDN has
some sample code for this...

> * The non-official mirrors will not show the location information.
>   They will be shown together at the bottom of the list.
>   This could act as a cue, to the user, that it is a non-official mirror.

This is good, but the fact that they'll always end up at the bottom isn't.

> * area, location are new members of the class.
> * The 'key' for site sorting is area + location + url

Again, this is a bad default.  We should keep the current sort order, but
allow sorting by other fields.  What I especially don't like is the
"Sorting order is" comment in operator<() -- if it belongs anywhere at
all, it should precede the assignment to "key" in site_list_type::init().

Oh, and the location info should *follow* the URL, not precede it.

> * Two sites are same if the URLs are same (area doesn't matter).
>   last-mirror doesn't store the area info.

Huh?  Will we ever have this situation?

> * The existing code tries to handle .com/.edu/.org (sort together)
>   but I don't think that it is actually effective.

In fact, that part of the code was buggy.  There was at least one fix
posted, but it didn't seem to have made it into CVS.  Your code is
cleaner.

> * I've added String.find(char, pos) method in String++.
>   string.find(c, pos) handles negative pos better.
>   substr() is better too.

Why not also add "String.split(char)" that returns an array or a vector of
Strings?  That would make your parseSite() much simpler.

>   I'm confused about string vs String.  Can't we just use std::string?
>   Maybe I should lookup the archives for the history of String++.
> * I haven't integrated Bas van Gompel's changes to site.cc

And two more comments:

1) The above isn't a proper ChangeLog.

2) Next time, please attach the patch, rather than including it inline.

	Igor
-- 
				http://cs.nyu.edu/~pechtcha/
      |\      _,,,---,,_		pechtcha@cs.nyu.edu
ZZZzz /,`.-'`'    -.  ;-;;,_		igor@watson.ibm.com
     |,4-  ) )-,_. ,\ (  `'-'		Igor Pechtchanski, Ph.D.
    '---''(_/--'  `-'\_) fL	a.k.a JaguaR-R-R-r-r-r-.-.-.  Meow!

If there's any real truth it's that the entire multidimensional infinity
of the Universe is almost certainly being run by a bunch of maniacs. /DA