216399 - RFE: Allow custom button labels in xpinstall dialogs

Status: RESOLVED FIXED

(Core Graveyard :: Installer: XPInstall Engine, enhancement)

Description

[Len Weisberg]

User-Agent:       Mozilla/5.0 (Windows; U; Win 9x 4.90; en-US; rv:1.4b) Gecko/20030516 Mozilla Firebird/0.6
Build Identifier: 

This RFE is motivated by the choice being offered to the
user of where to install a Browser extension.  Currently
this dialog has only the JavaScript builtin function
confirm() for this, with its limitation of "OK" and "Cancel"
as the only button labels available.

Even the best of several versions of this dialog in use is quite 
awkward and confusing:
    "Press OK to install to the current profile directory.
     Press Cancel to install to the global Firebird directory"
       [OK]   [Cancel]

Besides requiring very careful reading, this dialog misleads
the user into worrying that the operation may be cancelled.

With the enhancement requested it could be something much
clearer, such as:

  "Do you want to install this extension
   to the current Profile directory, 
   or to the Global application directory?"
       [Profile]    [Global]

The confirmEx() function in the nsIPromptService component
provides exactly the functionality required.

So perhaps this RFE should be titled something like one of:
 - make the confirmEx() function available to the install object
 - make a Components object available to the install process

Feel free to rename this bug to whatever is most
appropriate.  (Also feel free to change the component of
this bug, as I couldn't see anything for XPCOM nor XPInstall)

(There is more discussion of this topic in
http://forums.mozillazine.org/viewtopic.php?t=20148&start=15 )


Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Simon Paquet [:sipaq]]

Over to right component

Comment 2

[Steffen Wilberg]

Reassigning bug to owner and QA contact of that component.

Comment 3

[Jens Bannmann]

This was filed as a firefox bug, but Firefox no longer requires install scripts
and automatically manages install location. Does someone want this for the suite?

Comment 4

[Karsten Düsterloh]

> Does someone want this for the suite?

Yes. It's still a valid RFE there.

Comment 5

[Jens Bannmann]

Taking. I'll try to come up with a patch within the next week(s).

Comment 6

[Jens Bannmann]

Okay, I only recently started working. I'm confident to have a patch within the
next two weeks (yes, holidays are great)...

Comment 7

[Jens Bannmann]

Status update: the custom button labels part works, but the optional checkbox
provided by confirmEx does not yet. Although the latter is not really needed
according to the original bug description, I'd like to have the full confirmEx
functionality in XPInstall.

Comment 8

[Jens Bannmann]

Attachment: makes the confirmEx() function available to install scripts

Comment 9

[Jens Bannmann]

Attachment: example install.js snippet using confirmEx()

Attaching an example for install.js that demonstrates how to use custom buttons
with or without a checkbox.

Comment 10

[Jens Bannmann]

Attachment: patch v2

Updated patch to adress dveditz's remarks:
- overloading confirm() instead of introducing confirmEx
- improving alert() and confirm() to show the installation's display name (set
in initInstall call), if available, as the dialog title

Comment 11

[Jens Bannmann]

Attachment: updated example install.js

Attaching an updated example. Note how the BUTTON_POS_0 constant is used to
detect the extended confirm support. Also demonstrates the new title logic for
alert/confirm.

Comment 12

[Asa Dotzler [:asa]]

not a blocker but we'd consider taking a reviewed patch.

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 170183 [details] [diff] [review]
patch v2

Thanks for using more than the default 3 lines of context. In the future please
also use the "-p" cvs diff option.

> nsInstall::Alert(nsString& string)
>-    return ui->Alert( GetTranslatedString(NS_LITERAL_STRING("Alert").get()),
>-                      string.get());
>+    const PRUnichar *title;
>+    if (!mUIName.IsEmpty())
>+    {
>+      title = mUIName.get();
>+    }
>+    else
>+    {
>+      title = GetTranslatedString(NS_LITERAL_STRING("Alert").get());
>+    }
>+    return ui->Alert( title, string.get());

Now I'm a little worried install spoofers will use a blank pretty name, or try
to spoof some other dialog with a creative name. On the other hand, if an
installer is already running to be able to call these functions you're already
toast if it's a malicious installer. Ok, it's good as is, and I filed bug
277041 to think about ways to improve the text.

> PR_STATIC_CALLBACK(JSBool)
> InstallConfirm(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
> {
>   nsInstall *nativeThis = (nsInstall*)JS_GetPrivate(cx, obj);
>   nsAutoString b0;
>+  PRUint32 flags = 0;
>+  nsAutoString b1;
>+  nsAutoString b3;
>+  nsAutoString b4;
>+  nsAutoString b5;
>+  nsAutoString b6;
>+  JSObject* b7;

These need better names. As we've mucked in these files over time we've changed
the original auto-generated b0 etc here and there. Definitely don't want to
perpetuate this if you're adding so many variables.

>+  else if(argc == 8)
>+  {
>+    //  public int InstallConfirm (String aDialogTitle,
>+    //                             String aText,
>+    //                             Number aButtonFlags,
>+    //                             String aButton0Title,
>+    //                             String aButton1Title,
>+    //                             String aButton2Title,
>+    //                             String aCheckMsg,
>+    //                             Object aCheckState);

I'm OK with this for the freeze, but if all those extra parameters are
unnecessary (at times) why require placeholders? That is, the flags might say
what button text to use, and there may not be a check message or a check state
return, and if the script author doesn't need those things why not make it easy
on them and supply defaults?

Then you don't need to call both confirm() and confirmEx() on the backend, you
just call confirmEx() with the default arguments if they aren't specified.

waffling on this one...

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 170183 [details] [diff] [review]
patch v2

r-, I want a new patch with descriptive variable names at least.

Comment 15

[Jens Bannmann]

(In reply to comment #13)
> >+  else if(argc == 8)
> >+  {
> >+    //  public int InstallConfirm (String aDialogTitle,
> >+    //                             String aText,
> >+    //                             Number aButtonFlags,
> >+    //                             String aButton0Title,
> >+    //                             String aButton1Title,
> >+    //                             String aButton2Title,
> >+    //                             String aCheckMsg,
> >+    //                             Object aCheckState);
> 
> I'm OK with this for the freeze, but if all those extra parameters are
> unnecessary (at times) why require placeholders? That is, the flags might say
> what button text to use, and there may not be a check message or a check state
> return, and if the script author doesn't need those things why not make it easy
> on them and supply defaults?

What do you mean with "supply defaults" - do you suggest having new variants
with e.g. only 6 parameters (without checkbox), and/or having one without button
flags (auto-detect how many buttons are needed)?

> Then you don't need to call both confirm() and confirmEx() on the backend, you
> just call confirmEx() with the default arguments if they aren't specified.

But that wouldn't change anything for the author, right? I mean with the one-arg
call, he'd get the defaults, and with the 8-arg-call, he couldn't help but
overwrite the defaults...

Comment 16

[Jens Bannmann]

Attachment: patch v3

Descriptive variable names

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 170281 [details] [diff] [review]
patch v3

>   nsInstall *nativeThis = (nsInstall*)JS_GetPrivate(cx, obj);
>-  nsAutoString b0;
>+  nsAutoString aDialogTitle;
>+  nsAutoString aText;
>+  PRUint32 aButtonFlags = 0;
>+  nsAutoString aButton0Title;
>+  nsAutoString aButton1Title;
>+  nsAutoString aButton2Title;
>+  nsAutoString aCheckMsg;
>+  JSObject* aCheckState;

Thanks. However, we use the 'a' prefix to indicate arguments passed in and
these are locals. "dialogTitle" (or just "title") is better than
"aDialogTitle", etc.

This also raised my awareness that now we have confirm(text) and confirm(title,
text, ...). It works, but it's going to confuse people. Better to treat it as
if it were a single confirm(text, [title, flags, etc]) function with optional
args rather than two separate functions with completely different args.

I don't care about the order of the C++ version, you can leave that one
matching nsIPrompt if you like.

> What do you mean with "supply defaults" - do you suggest having new variants
> with e.g. only 6 parameters (without checkbox), and/or having one without button
> flags (auto-detect how many buttons are needed)?

Yes, pretty much. Everything after the flags could default to blank in a lot of
useful cases.

...
+  PRUint32 aButtonFlags = nsIPromptService::STD_OK_CANCEL_BUTTONS;
...
   if (argc >= 3)  // instead of 8, and we won't object to 9 or more
		   // in case someone adds more in the future
   {
... go ahead and check flags for numeric, bail if required
      JSObject* checkObj = 0;  // initialize this! (was aCheckState)
      ConvertJSValToStr(text, cx argv[0]);
      ConvertJSValToStr(title, cx, argv[1]);
      buttonFlags = JSVAL_TO_INT(argv[2]);

      if (argc > 3)
	 ConvertJSValToStr(button0, cx, argv[3]);
      if (argc > 4)
	 ConvertJSValToStr(button1, cx, argv[4]);
     ... etc...
     if (argc > 7 && JSVAL_IS_OBJECT(argv[7])) // why care about null, just
ignore others
	checkObj = JSVAL_TO_OBJECT(argv[7]);
	...

>+    // Only save back checkState when an object was passed as last parameter
>+    if (jsCheckState)

This would be better as "if (checkObj)", which is now initialized to null.
Checking the jsval works (if you initialize it), but it's not appropriate to
use a jsval as a boolean.

still r-, but almost there. btw if it's getting too late I can check in for you
if the next patch is OK.

Comment 18

[Jens Bannmann]

Attachment: patch v4

- Makes all parameters beside the confirm-text optional, allows a two-arg
confirm(text, title).
- Stays backward-compatible with the old confirm() call: returns 0=OK/first
button, 1=Cancel/second button/window closed, 2=Third button. Note that this is
different behaviour than nsPromptService's confirmEx.
- Polish: removes internal confirm() calls that are no longer used.

Re-applying this on the current, unmodified xpinstall/ files builds and works
correctly; the previous patches did not include neccessary xpinstall/public/
changes.

Comment 19

[Jens Bannmann]

(In reply to comment #18)
> - Stays backward-compatible with the old confirm() call: returns 0=OK/first
> button, 1=Cancel/second button/window closed, 2=Third button.

I mixed it up in this comment, actually it's:
 0 (false)     = Cancel/second button/window closed
 1 (true)      = OK/first button
 2 (also true) = Third button

So we really only add a third possible return value here.

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 170337 [details] [diff] [review]
patch v4

r/sr=dveditz

Comment 21

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 170337 [details] [diff] [review]
patch v4

+      title = GetTranslatedString(NS_LITERAL_STRING("Alert").get());
+    }
+    return ui->Alert( title, string.get());

this is a memory leak...

public/nsPIXPIProxy.idl

this needs a new IID

Comment 22

[Jens Bannmann]

I'll address biesi's comments in a new patch.

Comment 23

[Jens Bannmann]

Attachment: patch v5

Fixed the memory leaks (they were in the old code, too). However, I left the
old IID as bumping it resulted in my build not showing any alerts/confirms at
all.

Requesting r from biesi as he spotted the leaks. Is the sr carried over from
the last patch, or should I request it again?

Comment 24

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 171198 [details] [diff] [review]
patch v5

+ title = aDialogTitle.get();

I'd rather you didn't do that... this forces a string copy. instead, do:
  title = aDialogTitle;
which can often share the buffer.

Comment 25

[Jens Bannmann]

Attachment: v6: fixes memory leaks, adds fallback in case GetTranslatedString() fails

Fixed that.

Oh, and I forgot to mention: patches v5/6 add another thing (sponsored by
timeless): A hard-coded fallback for the "Confirm" or "Alert" titles in case
they're not translated . Although that's unlikely, the fallback doesn't hurt
either (and is always better than an empty title).

Comment 26

[Daniel Veditz [:dveditz]]

Comment on attachment 171199 [details] [diff] [review]
v6: fixes memory leaks, adds fallback in case GetTranslatedString() fails

sr=dveditz
I'm OK not revving the IID for nsPIXPIProxy, it's private.

Comment 27

[Jens Bannmann]

Attachment: v7: minor tweaks

Sorry to obsolete a reviewed patch once again, but timeless convinced me to
make some minor changes. This one includes a real change, though - a clearer IF
block for the 'checkState' parameter check (search for 'JS_GetProperty' to find
it) - so I need new reviews. The rest fixes incorrect indentation, improves
comments and fixes javadoc. I recommend diffing it to the previous one to see
the changes.

Comment 28

[Daniel Veditz [:dveditz]]

Comment on attachment 172287 [details] [diff] [review]
v7: minor tweaks

r/sr=dveditz on the differences between this and the previously reviewed patch.

Comment 29

[Simon Montagu :smontagu]

Patch checked in.

Comment 30

[[e-mail]]

Are you planning on documenting this anywhere?

Comment 31

[Jens Bannmann]

(In reply to comment #30)
> Are you planning on documenting this anywhere?

Sure, I want to get this up on developer.mozilla.org soon - in a way that will
fit into the XPInstall API once the Netscape DevEdge content is transferred.

Comment 32

[Jens Bannmann]

(In reply to comment #30)
> Are you planning on documenting this anywhere?

Draft documentation is up at DevMo, right inside the XPInstall API Reference.

Current location (during DevMo alpha phase):
http://developer-test.mozilla.org/docs/XPInstall_API_Reference:Install_Object:Methods:confirm
Feel free to suggest (or just make) changes over there.