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


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


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