Re: User URLs in setup.exe
- Original Message - From: "Robert Collins" <[EMAIL PROTECTED]> To: "John Marshall" <[EMAIL PROTECTED]> Cc: <[EMAIL PROTECTED]> Sent: Thursday, June 27, 2002 8:25 PM > > One way to fix that would be to change #3 (as defined in my previous > > posting) so that adding an URL that happens to have the same key as an > > existing one *does* overwrite it. I think [1] the following surprisingly > > simple patch achieves this: Committed (but still untested :}). Rob
Re: User URLs in setup.exe
On Thu, Jun 27, 2002 at 08:25:29PM +1000, Robert Collins wrote: >> That change seems like something easily big enough for HEAD, and >> probably it'd be over the threshold that I'd need to be copyright- >> assigned. That's doable, but it'll take time of course. > > Copyright assignment isn't needed for setup.exe. (Searches archives... where did I get the idea that it was?) Oh, okay, oops. That's fine then. And uncivilised employment contracts aren't an issue in this case. Okay, no more excuses... it's about time I got set up to build setup.exe... > Yes, lets turn it on it's head though: how does a user that *does* want that > enter it via the GUI if we strip the final /? I was thinking of "http://foo.com/foo%2f";, but apparently URL-encoding doesn't apply to slashes, and I have to go and reread the relevant RFC. (Probably there's an "of course it doesn't apply to separators!" involved here :-)) Never mind, then. John
Re: User URLs in setup.exe
- Original Message -
From: "John Marshall" <[EMAIL PROTECTED]>
To: "Robert Collins" <[EMAIL PROTECTED]>
> It sounds very good to me! So add syntax (or just more parsing because
> it's already there?) to mirrors.lst so setup.exe could display stuff
> like
> http://foonet.no (Norway)
>
> Cool :-).
>
> I'm not sure how the friendly extra information would get into
> last-mirror though. If a last-mirror is from mirrors.lst then you've
> got the information in mirrors.lst anyway, and otherwise it's a User URL
> and you'd have to get the user to type it in, which seems over-the-top.
Thats why there is the clause to only show friendly info if it exists. Thus
user URL's show unfriendly details :}.
> That change seems like something easily big enough for HEAD, and
> probably it'd be over the threshold that I'd need to be copyright-
> assigned. That's doable, but it'll take time of course.
Copyright assignment isn't needed for setup.exe. The only requirement is
that you have the right to licence the code you've created under the GPL.
(i.e. you may not have that right if you have an IP assignment clause in
your employment contract).
> In the meantime, we have what I think is a serious UI bug in the 2002-06
> branch: people can't correct typos in User URLs without manipulating
> last-mirror in a way that needs some expertise.
>
> One way to fix that would be to change #3 (as defined in my previous
> posting) so that adding an URL that happens to have the same key as an
> existing one *does* overwrite it. I think [1] the following surprisingly
> simple patch achieves this:
>
> --- site.cc.orig Thu Jun 27 02:09:10 2002
> +++ site.cc Thu Jun 27 02:24:41 2002
> @@ -409,6 +409,7 @@ bool SitePage::OnMessageCmd (int id, HWN
> if (&listobj != newsite)
> {
> // That site was already registered
> + listobj = *newsite;
> delete
> newsite;
> }
Should do. I'll play with this a little later. And yes I agree that this is
a reasonable interim step.
> > I think this is the wrong thing to do. Us users shouldn't need to
> > understand url encoding, which is why trailing '/'s and spaces are
> > currently preserved. (because http://foo.com/cygwin//setup.ini is a
> > valid URL different from http://foo.com/cygwin/setup.ini).
>
> Sure it's valid, and I agree with you, but to be really convincing
> you'd have to demonstrate a situation in which someone actually *wanted*
> that :-). It's a UI question: what proportion of the times that a user
> types a trailing space or slash (so ends up with "[...]//setup.ini") do
> they actually *intend* to do that?
Yes, lets turn it on it's head though: how does a user that *does* want that
enter it via the GUI if we strip the final /?
> However, it occurs to me that the real solution to that is not to muck
> with setup.exe, but for me to play more games with HTTP redirection on
> my server. So that's fine.
Yep :}.
Rob
Re: User URLs in setup.exe
On Thu, Jun 27, 2002 at 06:27:36AM +1000, Robert Collins wrote:
> I always like patches. As for the right thing, how does this sound:
> For all urls (i.e. no special cases);
> Show one of:
> * a friendly path provided in the mirrors.lst or last-mirror files.
> * the entire path
It sounds very good to me! So add syntax (or just more parsing because
it's already there?) to mirrors.lst so setup.exe could display stuff
like
http://foonet.no (Norway)
Cool :-).
I'm not sure how the friendly extra information would get into
last-mirror though. If a last-mirror is from mirrors.lst then you've
got the information in mirrors.lst anyway, and otherwise it's a User URL
and you'd have to get the user to type it in, which seems over-the-top.
That change seems like something easily big enough for HEAD, and
probably it'd be over the threshold that I'd need to be copyright-
assigned. That's doable, but it'll take time of course.
In the meantime, we have what I think is a serious UI bug in the 2002-06
branch: people can't correct typos in User URLs without manipulating
last-mirror in a way that needs some expertise.
One way to fix that would be to change #3 (as defined in my previous
posting) so that adding an URL that happens to have the same key as an
existing one *does* overwrite it. I think [1] the following surprisingly
simple patch achieves this:
--- site.cc.origThu Jun 27 02:09:10 2002
+++ site.cc Thu Jun 27 02:24:41 2002
@@ -409,6 +409,7 @@ bool SitePage::OnMessageCmd (int id, HWN
if (&listobj != newsite)
{
// That site was already registered
+ listobj = *newsite;
delete
newsite;
}
>> There is also the question of whether setup User URLs should be
>> susceptible to trailing slashes and spaces. Here's a (ugly and
>> untested) patch that strips trailing spaces:
>
> I think this is the wrong thing to do. Us users shouldn't need to
> understand url encoding, which is why trailing '/'s and spaces are
> currently preserved. (because http://foo.com/cygwin//setup.ini is a
> valid URL different from http://foo.com/cygwin/setup.ini).
Sure it's valid, and I agree with you, but to be really convincing
you'd have to demonstrate a situation in which someone actually *wanted*
that :-). It's a UI question: what proportion of the times that a user
types a trailing space or slash (so ends up with "[...]//setup.ini") do
they actually *intend* to do that?
For example, at the moment I'm getting real live bug reports because
morons insist on typing in "http://prc-tools.sourceforge.net/install/";
and "[...]/install//setup.ini" doesn't exist.
However, it occurs to me that the real solution to that is not to muck
with setup.exe, but for me to play more games with HTTP redirection on
my server. So that's fine.
John
[1] This is just a gedankenexperiment; I still haven't got around to
building setup.exe myself :-(
RE: User URLs in setup.exe
> -Original Message- > From: [EMAIL PROTECTED] > [mailto:[EMAIL PROTECTED]] On Behalf Of John Marshall > Sent: Thursday, 27 June 2002 4:22 AM > I'm not sure what the right fix is. I think probably #2 > above is right > (because if http://example.com appears twice in the list but the full > URLs behind them are different... that's confusing), but #1 and #3 are > wrong. I suspect displayed_url for an URL that you type in yourself > should be the full thing you typed, so that you can see it. (And then > #3 would no longer be an issue.) > > If others agree (and want me to back my words with actions :-)), I'll > send a patch to do this. I always like patches. As for the right thing, how does this sound: For all urls (i.e. no special cases); Show one of: * a friendly path provided in the mirrors.lst or last-mirror files. * the entire path And: * Use a pop-up box to show the entire path for -every- mirror. * And generate the unique key based on the entire path not the display path. > There is also the question of whether setup User URLs should be > susceptible to trailing slashes and spaces. Here's a (ugly and > untested) patch that strips trailing spaces: I think this is the wrong thing to do. Us users shouldn't need to understand url encoding, which is why trailing '/'s and spaces are currently preserved. (because http://foo.com/cygwin//setup.ini is a valid URL different from http://foo.com/cygwin/setup.ini). > If that turned out not to be a good idea, it might be worth making the > spaces visible in the error message, e.g. like this: Thanks, I'm checking these two changes in. Rob
User URLs in setup.exe
A few things are currently (2.249.2.4) interacting to make setup's
user interface for adding "User URLs" extremely susceptible to typos.
Suppose you have a bunch of packages living outside the usual set of
Cygwin mirrors, so you tell your users to add (random example :-))
http://prc-tools.sourceforge.net/install
to their list of mirror sites (i.e. on setup's site.cc page). Then
1. It gets listed as just "http://prc-tools.sourceforge.net"; so they
can't tell if they actually typed it all in correctly. While I think
hiding the details of the URL is good for the ones in mirrors.lst,
I'm not sure it's right for ones the users type in themselves.
2. The URL's unique key in all_site_list is (derived from) the
abbreviated one that's displayed, not the full URL.
3. If you type in another URL with the same key as one that's already in
the list (say you're trying to fix a suspected (and invisible! :-))
typo), your new URL will get discarded (!) instead of overwriting the
old one or appearing beside it (SitePage::OnMessageCmd, site.cc:418).
Example: Add "http://example.com/foo"; as a User URL. Now try to add
"http://example.com/bar";. To get the change to stick you have to either
manually delete /etc/setup/last-mirror and rerun setup, or ensure that
"http://example.com"; is not selected and quit setup (thus getting it out
of last-mirror) and rerun setup. Both of these are difficult to explain
to clueless newbies, especially ones who don't believe that they could
possibly have made a typo.
Dumb user #1 example: user types in
"http://prc-tools.sourceforge.net/install/";, which sadly doesn't work.
They get this error message:
Unable to retrieve setup.ini from http://prc-tools.sourceforge.net/install/
so with luck they can see that they need to retype it. So they try
again with "http://prc-tools.sourceforge.net/install"; and check it
*really carefully* before they press Add. Unfortunately they can check
it as much as they like; setup will ignore the change and they'll still
get the same error.
Dumb user #2 example: it turns out that instead of typing in the URL
above, many of the users cut&paste it from the instructions Web page,
which until today would often give them a trailing space. This would
lead to an error message that looks like this:
Unable to retrieve setup.ini from http://prc-tools.sourceforge.net/install
and they *really* couldn't tell what they'd done wrong! And similarly
trying to fix the invisible typo wasn't actually doing anything.
I'm not sure what the right fix is. I think probably #2 above is right
(because if http://example.com appears twice in the list but the full
URLs behind them are different... that's confusing), but #1 and #3 are
wrong. I suspect displayed_url for an URL that you type in yourself
should be the full thing you typed, so that you can see it. (And then
#3 would no longer be an issue.)
If others agree (and want me to back my words with actions :-)), I'll
send a patch to do this.
There is also the question of whether setup User URLs should be
susceptible to trailing slashes and spaces. Here's a (ugly and
untested) patch that strips trailing spaces:
Index: site.cc
===
RCS file: /cvs/cygwin-apps/setup/site.cc,v
retrieving revision 2.18
diff -u -p -r2.18 site.cc
--- site.cc 6 May 2002 14:40:41 - 2.18
+++ site.cc 26 Jun 2002 13:54:51 -
@@ -25,6 +25,7 @@ static const char *cvsid =
#include
#include
#include
+#include
#include
#include "dialog.h"
@@ -50,7 +51,13 @@ list < site_list_type, String, String::c
void
site_list_type::init (String const &newurl)
{
- url = newurl;
+ char *spaces = newurl.cstr();
+ char *space = spaces + strlen (spaces);
+ while (space > spaces && isspace (space[-1]))
+--space;
+ *space = '\0';
+ url = String (spaces);
+ delete[] spaces;
char *dots = newurl.cstr();
char *dot = strchr (dots, '.');
It might also be worth stripping trailing '/' characters; if some mirror
really needed to end in a space, you could presumably use %20 notation.
Comments? Me, I'm not particularly comfortable about changing what the
user typed in, but...
If that turned out not to be a good idea, it might be worth making the
spaces visible in the error message, e.g. like this:
Index: res.rc
===
RCS file: /cvs/cygwin-apps/setup/res.rc,v
retrieving revision 2.40
diff -u -p -r2.40 res.rc
--- res.rc 12 May 2002 11:28:22 - 2.40
+++ res.rc 26 Jun 2002 13:54:51 -
@@ -439,7 +439,7 @@ BEGIN
IDS_CYGWIN_FUNC_MISSING "Error: unable to find function `%s' in %s"
IDS_DOWNLOAD_SHORT "Download error: %s too short (%d, wanted %d)"
IDS_ERR_OPEN_WRITE "Can't open %s for writing: %s"
-IDS_SETUPINI_MISSING"Unable to get setup.ini from %s"
+IDS_SETUPINI_MISSING"Unable to get setup.ini from <%s>"
