This is the mail archive of the cygwin-apps mailing list for the Cygwin project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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

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
> North America, Virginia
> North America, Wisconsin
> 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

> * 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

And two more comments:

1) The above isn't a proper ChangeLog.

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

      |\      _,,,---,,_
ZZZzz /,`.-'`'    -.  ;-;;,_
     |,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

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]