Re: [PATCH] make setup mirror list more like web page not just urls

[Ken Brown <kbrown@cornell.edu>]

On 11/20/2017 4:30 PM, Brian Inglis wrote:
> On 2017-11-17 06:53, Jon Turney wrote:
> On 11/17/2017 8:48 AM, Ken Brown wrote:
>> On 15/11/2017 21:35, Brian Inglis wrote:
> 
> Thanks for your comments, I understand all your points, and agree with them.
> I merged the posts and responses as they addressed the same issues.
> I also applied Ken's setup patch and rejigged mine where required.
> 
> KB> Ken Brown
> JT> Jon Turney
> 
> KB> I think this is a good idea in principle, but it still needs some work.
> 
> JT> use a grid control to present this information.
> 
> Don't think I want to learn that much about Windows controls!
> The width of the longest fields in each column would need to adjust the grid
> and dialogue box widths, and Add button position, dynamically, which I did
> manually, and the internal blank space may be a bit harder to scan for users
> who are interested only in their geographic region, country, or territory
> - these are pretty well aligned as strings.
> 
> KB> Adding the protocol to the key instead of the full url isn't enough to
> KB> guarantee uniqueness of the key.
> KB> Users can add their own sites for which that fails.
> KB> Also, you shouldn't be relying on the servername field being filled in.
> KB> For example, the function SiteSetting::registerSavedSite() explicitly
> KB> constructs a site 'tempSite' that has an empty servername.
> 
> JT> This code needs to handle manually added URLs with non-FQDNs.
> JT> - - url...'s going to need fixing.
> 
> If host name provided, use as servername, or default to localhost.
> 
> If at least two domain components:
> if CC TLD, use as area in uppercase, or
> if gTLD, use as area with init cap;
> if 2-3 char TLD and 2-3 char 2nd level domain, may be subTLD, so if 3rd level
> domain exists and does not start with (or contain?) "cygwin", "ftp", "mirror",
> "www", add space plus 2-3 char 2nd level domain with init cap to area.
> Use next level domain as location with init cap and uppercase after any "-".
> Otherwise default area to Local, and use servername as location with init cap.
> This adhoc approach seems to work well enough and avoids having to drag in and
> process the rules in the Public Suffix List (libpsl5 and
> publicsuffix-list-dafsa).
> 
> Examples:
> 
> Input	->
>      List
>      Displayed
>      Key
> 
> http://mirror.cpsc.ucalgary.ca/mirror/cygwin.com		->
> http://mirror.cpsc.ucalgary.ca/mirror/cygwin.com/;mirror.cpsc.ucalgary.ca;CA;Ucalgary
> CA - Ucalgary - http://mirror.cpsc.ucalgary.ca/mirror/cygwin.com
> CA Ucalgary ca ucalgary cpsc mirror
> http://mirror.cpsc.ucalgary.ca/mirror/cygwin.com/
> 
> [cyg]file://server/c$/usr/local/cygwin/var/cache/setup/packages	->
> [cyg]file://server/c$/usr/local/cygwin/var/cache/setup/packages/;server;Local;Server
> Local - Server - file://server/c$/usr/local/cygwin/var/cache/setup/packages
> Local Server server file://server/c$/usr/local/cygwin/var/cache/setup/packages/
> 
> cygfile:///var/cache/setup/packages		->
> cygfile:///var/cache/setup/packages/;localhost;Local;Localhost
> Local - Localhost - cygfile://var/cache/setup/packages
> Local Localhost localhost cygfile://var/cache/setup/packages/
> 
> file:///c|/usr/local/cygwin/var/cache/setup/packages		->
> file:///c|/usr/local/cygwin/var/cache/setup/packages/;localhost;Local;Localhost
> Local - Localhost - file:///c|/usr/local/cygwin/var/cache/setup/packages
> Local Localhost localhost file:///c|/usr/local/cygwin/var/cache/setup/packages/
> 
> Cygwin file:// does not appear to use /// or support anything but local files,
> so should always be treated as localhost; or does it support servers, but they
> can be treated as if directories under Windows, so we need to do a server or
> directory check, or that check is done at a lower level?
> 
> JT> you can also just add a path.
> 
> If UNC path with server, use as above, but include share name in display,
> otherwise default as above.
> 
> Examples:
> 
> \\server\c$\usr\local\cygwin\var\cache\setup\packages		->
> \\server\c$\usr\local\cygwin\var\cache\setup\packages/;server;Local;Server
> Local - Server - \\server\c$
> Local Server server \\server\c$\usr\local\cygwin\var\cache\setup\packages/
> 
> c:\usr\local\cygwin\var\cache\setup\packages			->
> c:\usr\local\cygwin\var\cache\setup\packages/;localhost;Local;Localhost
> Local - Localhost - c:\usr\local\cygwin\var\cache\setup\packages
> Local Localhost localhost c:\usr\local\cygwin\var\cache\setup\packages/
> 
> Where are the cached sites stashed - setup keeps showing my old test entries?
> 
> JT> if you add two URLs with the same protocol/host, they aren't distinguishable
> 
> Check for and eliminate duplicates after sort if same site_list fields.
> Handled by KB patch.
> Note on flag added by that patch - would is_current, is_good, or is_known be a
> better reflection of what that flag means?
> Should we add ==(string) to compare only the url member?
> Should we add a constructor(string,is_official) to refactor string to site
> conversions, and the url additions?
> 
> KB> If the saved url corresponds to a site in the list, I think setup should
> KB> recognize this rather than listing the same site again as " - - url".
> 
> JT> If the URL matches one in the list, that item should be selected.
> 
> Check for URL match with site list and use match as site:
> added for tempSite in RegisterSavedSites and newsite on Add;
> required as key now includes area and location, not just URL.
> Could we add a check to a string contructor to return only a reference to an
> existing entry, if the url matches, to refactor the above changes?
> 
> JT> Store area and location so we can populate those fields if it doesn't.
> 
> See above.
> 
> JT> Please, please use git-format-patch in future.
> 
> I can't tell from looking, does it differ from diff output, and is there a
> reason to use git-format-patch for review?
> I use diffs for feedback until it DTRT.
> I can manage one commit and one format-patch on a good day.
> I should just stick to using what Tortoise Git supports.
> My record with git includes a lot of rm -rf and re-clone, as the files, trees,
> and indices get out of whack when I forget some incantation and try to fix it!
> Very brittle and unfriendly - if it's going to bork itself, just don't do that!
> I never had a single problem with CVS or Hg in many years ;^>
> 
> JT> The patch also has some spurious changes to the mode of the manifest files.
> 
> Realized I shouldn't tidy up unnecessary x modes under git control - habit
> - gets me into trouble and I learn new things - checked out masters.

Hi Brian,

The issue of recognizing a URL that's already in the list is not fixed. 
Here's what I tried:

I ran setup with the last mirror (as stored in /etc/setup/setup.rc) 
being http://mirrors.kernel.org/sourceware/cygwin/.  The resulting 
mirror list display contained the following, highlighted:

   Org - Kernel - http://mirrors.kernel.org

So setup didn't recognize that 
http://mirrors.kernel.org/sourceware/cygwin/ was already in the list, 
displayed as

   United States - California - http://mirrors.kernel.org

Also, I don't like the fake 'area' and 'location' fields for user-added 
URLs.  For example, "Org" and "Kernel" are startling in the display 
above and add no information to the plain URL.  Another example is my 
own personal repository, http://sanibeltranquility.com/cygwin, which 
displays as

   Com - Sanibeltranquility - http://sanibeltranquility.com

I would rather see the area and location left blank, or perhaps replaced 
by "Unknown".  A third alternative would be to replace "Area - Location" 
by "User-added" or something similar.

Ken