Closed
Bug 216399
Opened 21 years ago
Closed 20 years ago
RFE: Allow custom button labels in xpinstall dialogs
Categories
(Core Graveyard :: Installer: XPInstall Engine, enhancement)
Core Graveyard
Installer: XPInstall Engine
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.8beta1
People
(Reporter: Len, Assigned: jens.b)
Details
Attachments
(2 files, 7 obsolete files)
805 bytes,
text/plain
|
Details | |
19.62 KB,
patch
|
dveditz
:
review+
dveditz
:
superreview+
|
Details | Diff | Splinter Review |
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•21 years ago
|
||
Over to right component
Component: Downloading → Installer: XPInstall Engine
Product: Firebird → Browser
Version: unspecified → Trunk
Comment 2•21 years ago
|
||
Reassigning bug to owner and QA contact of that component.
Assignee: blake → ssu
QA Contact: asa → gbush
Assignee | ||
Comment 3•20 years ago
|
||
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•20 years ago
|
||
> Does someone want this for the suite?
Yes. It's still a valid RFE there.
Assignee | ||
Comment 5•20 years ago
|
||
Taking. I'll try to come up with a patch within the next week(s).
Assignee: ssu0262 → jens.b
Assignee | ||
Comment 6•20 years ago
|
||
Okay, I only recently started working. I'm confident to have a patch within the next two weeks (yes, holidays are great)...
Status: NEW → ASSIGNED
Assignee | ||
Comment 7•20 years ago
|
||
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.
Assignee | ||
Comment 8•20 years ago
|
||
Attachment #170175 -
Flags: review?(dveditz)
Assignee | ||
Comment 9•20 years ago
|
||
Attaching an example for install.js that demonstrates how to use custom buttons with or without a checkbox.
Assignee | ||
Comment 10•20 years ago
|
||
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
Attachment #170183 -
Flags: review?(dveditz)
Assignee | ||
Updated•20 years ago
|
Attachment #170175 -
Attachment is obsolete: true
Attachment #170175 -
Flags: review?(dveditz)
Assignee | ||
Comment 11•20 years ago
|
||
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.
Attachment #170177 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Flags: blocking1.8a6?
Comment 12•20 years ago
|
||
not a blocker but we'd consider taking a reviewed patch.
Flags: blocking1.8a6? → blocking1.8a6-
Comment 13•20 years ago
|
||
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•20 years ago
|
||
Comment on attachment 170183 [details] [diff] [review] patch v2 r-, I want a new patch with descriptive variable names at least.
Attachment #170183 -
Flags: review?(dveditz) → review-
Assignee | ||
Comment 15•20 years ago
|
||
(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...
Assignee | ||
Comment 16•20 years ago
|
||
Descriptive variable names
Attachment #170183 -
Attachment is obsolete: true
Attachment #170281 -
Flags: review?(dveditz)
Comment 17•20 years ago
|
||
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.
Attachment #170281 -
Flags: review?(dveditz) → review-
Assignee | ||
Comment 18•20 years ago
|
||
- 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.
Assignee | ||
Updated•20 years ago
|
Attachment #170281 -
Attachment is obsolete: true
Attachment #170337 -
Flags: review?(dveditz)
Assignee | ||
Comment 19•20 years ago
|
||
(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•20 years ago
|
||
Comment on attachment 170337 [details] [diff] [review] patch v4 r/sr=dveditz
Attachment #170337 -
Flags: superreview+
Attachment #170337 -
Flags: review?(dveditz)
Attachment #170337 -
Flags: review+
Comment 21•20 years ago
|
||
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
Assignee | ||
Comment 22•20 years ago
|
||
I'll address biesi's comments in a new patch.
Target Milestone: --- → mozilla1.8beta
Assignee | ||
Comment 23•20 years ago
|
||
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?
Attachment #170337 -
Attachment is obsolete: true
Attachment #171198 -
Flags: review?(cbiesinger)
Comment 24•20 years ago
|
||
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.
Attachment #171198 -
Flags: review?(cbiesinger) → review-
Assignee | ||
Comment 25•20 years ago
|
||
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).
Assignee | ||
Updated•20 years ago
|
Attachment #171198 -
Attachment is obsolete: true
Attachment #171199 -
Flags: review?(cbiesinger)
Assignee | ||
Updated•20 years ago
|
Attachment #171199 -
Flags: superreview?(dveditz)
Updated•20 years ago
|
Attachment #171199 -
Flags: review?(cbiesinger) → review+
Assignee | ||
Updated•20 years ago
|
Attachment #171199 -
Attachment description: patch v6 → v6: fixes memory leaks, adds fallback in case GetTranslatedString() fails
Comment 26•20 years ago
|
||
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.
Attachment #171199 -
Flags: superreview?(dveditz) → superreview+
Assignee | ||
Comment 27•20 years ago
|
||
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.
Assignee | ||
Updated•20 years ago
|
Attachment #171199 -
Attachment is obsolete: true
Attachment #172287 -
Flags: superreview?(dveditz)
Attachment #172287 -
Flags: review?(dveditz)
Comment 28•20 years ago
|
||
Comment on attachment 172287 [details] [diff] [review] v7: minor tweaks r/sr=dveditz on the differences between this and the previously reviewed patch.
Attachment #172287 -
Flags: superreview?(dveditz)
Attachment #172287 -
Flags: superreview+
Attachment #172287 -
Flags: review?(dveditz)
Attachment #172287 -
Flags: review+
Comment 29•20 years ago
|
||
Patch checked in.
Assignee | ||
Updated•20 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Comment 30•20 years ago
|
||
Are you planning on documenting this anywhere?
Assignee | ||
Comment 31•20 years ago
|
||
(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.
Assignee | ||
Comment 32•19 years ago
|
||
(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.
Updated•9 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•