This is the mail archive of the
cygwin-apps
mailing list for the Cygwin project.
Re: [Patch] Setup: Display mirrors sorted by location
- From: Rajesh Balakrishnan <rajesh_balakrishnan at yahoo dot com>
- To: cygwin-apps at cygwin dot com
- Date: Fri, 11 Nov 2005 19:10:36 -0800 (PST)
- Subject: Re: [Patch] Setup: Display mirrors sorted by location
Igor Pechtchanski <pechtcha@cs.nyu.edu> wrote:
> On Thu, 10 Nov 2005, Rajesh Balakrishnan wrote:
> Ok, here goes. First, thanks for doing this. However, if this is to be
> done at all, let's do it right.
Thanks Igor for your comments and I agree with it.
I was just trying to display the location info,
without expecting the user to sort it.
> > 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...
OK, shall look at ListView, am a novice at UI :-)
> > * 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
Sorting by URL isn't a good default. User-selectable sort order
is of course better.
> 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?
A couple of reasons for this:
* "last-mirror" only has the URL and no location.
So when the find() is done on the all_site_list (mirrors.lst),
they should be treated as the same site, despite lacking location info.
A similar situation would be when the user adds an existing site.
* I think that mirrors.lst actually has a duplicate entry (for Australia?)
> > * 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.
Yes, that will be better.
> And two more comments:
> 1) The above isn't a proper ChangeLog.
> 2) Next time, please attach the patch, rather than including it inline.
Thanks Igor, shall follow that for future patches.
Regards,
rb
__________________________________
Yahoo! FareChase: Search multiple travel sites in one click.
http://farechase.yahoo.com