Closed Bug 308916 Opened 19 years ago Closed 18 years ago

Extension Manager ui string review / cleanup

Categories

(Toolkit :: Add-ons Manager, defect)

Product:
Toolkit
Component:
Add-ons Manager
Version:
1.8 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.8.1beta1

People

(Reporter: robert.strong.bugs, Assigned: robert.strong.bugs)

References

Details

(Keywords: fixed1.8.1, late-l10n, Whiteboard: [AOMTestday])

Attachments

(3 files, 1 obsolete file)

uninstall screenshot
26.94 KB, image/png
beltzner
: ui-review+
Details
Screenshot - action messages
14.99 KB, image/png
beltzner
: ui-review+
Details
updated patch
10.62 KB, patch
moco
: review+
mconnor
: approval1.8.1+
Details | Diff | Splinter Review 
There are several bugs floating around to change individual ui strings used by
the extension manager. Instead of fixing them individually this bug is for
reviewing all of them and updating them as appropriate. This will make it easier
to create consistent verbiage in the ui

See bug 303863, bug 303687, bug 302527, bug 292854, bug 290021, bug 275629, and
I'm sure others as well.
In relation to extension packaging errors I'd prefer to use a generic message
that also includes a statement to refer to the JavaScript Console for details.
My reasons off the top of my head are:
a) These errors should only be seen by extension authors.
b) when there is a regression in the EM we end up with bug reports regarding how
the detailed message is not appropriate to show to a user (see bug 302527 and
bug 292854).
c) simpler code for displaying these errors.
I'm a fan of the generic message for things like packaging errors, but not so
sure that it needs to have a specific call out to the JS console. Users need to
know that an error happened, and developers should know to look in the JS
console for more information.
Mike, that would be very newbie-developer unfriendly. It may sound weird, but
not every developer knows to look in the JavaScript console.

I think that since messages about packaging errors are seen by developers more
often than by users (ideally, users should never see it), the mention of JS
console is justified.
Well, we don't point people to the JS console when there are JS errors, nor for
CSS or HTML errors. I think the better approach is to train developers that they
can find detailed error messages in the JS console rather than repeat that fact
in error dialogs. But we can hash this out in the specific bugs :)
I don't see any harm with the message stating something along the lines of
"Please review the JavaScript Console for further information" for error
messages that only ext. developers would see with the exception of when there is
a regression that causes the message to be displayed. The fact is that many ext.
developers don't open the js console when they experience an error and this
would be a good way to train them since average users would seldom if ever see
these messages. As is we periodically get bug reports about the alert being too
generic, etc. from ext. developers when the details are already in the js console.

I believe that only one of the bugs this depends on relates to this type of
message and there are a couple of additional messages that fall into this
category... these probably haven't been reported since we haven't regressed in
the areas that would cause the message to be displayed whereas we did once in
the case of bug 302527.

Mike, do you have suggestions for a general packaging errors that only ext.
developers would see except in the case of a regression?
If you're saying that these are errors that only developers are likely to see, then something like:

"%productShortName was unable to load %extensionName. Please see the JavaScript console for details."

Might also be nice to have a checkbox option to "[ ] Open JavaScript Console" at the bottom, too.
Rob, some more EM UI string changes (these from the review requested in EM Dependencies, bug 298497)

Uninstall dialog
----------------
I like the changes to the button labels here, but the current text is a little redundant. I don't think people need to be informed that if they uninstall something, its function won't be available. Especially since we can't make any intelligent statements about what that function is, specifically. Maybe in the future we want to leave that as something that can be added by the extension itself. For now I'd suggest:

uninstallWarningMessage=This will remove the %S extension.

Similarly, the warning for installation with dependencies would become:

uninstallWarningDependMessage=%S is required by one or more extensions. If you continue, the following items will be disabled:

Missing Dependency message:
---------------------------
Any reason we're calling things "items" instead of extensions? "Operate" is also a slightly awkward term.

needsDependenciesDisabled=Disabled - missing one or more required extensions
Thanks Mike, the reason I used items is due to what we discussed regarding the use of the term extensions for things that aren't extensions as defined today. I am fine with using the term extensions for all items / addons (e.g. themes, plugins, searchplugins, locales, etc.) that are packaged for the Extension Manager.
Ah, then, go with whatever the generic term ends up being: extension, add-on, thingamajigger ...
Yo Mike, for the operation type messages I would like to go with the following which removes the redundant extension name which is displayed immediately above the message.
Will be upgraded when &brandShortName; is restarted
Will be installed when &brandShortName; is restarted
Will be uninstalled when &brandShortName; is restarted
Will be enabled when &brandShortName; is restarted
Will be disabled when &brandShortName; is restarted

r u down wit dat?
Assignee: nobody → robert.bugzilla
Target Milestone: --- → Firefox 2 beta1
I suspect that beltzner will end up doing the majority of the work on this one and it will be around 1 day of my time
Whiteboard: 1d
Flags: blocking-firefox2+
Depends on: 260861
Bug 275629 - has the following strings and the bug is about displaying these strings from a local install.
itemWarningIntroMultiple=A web site is requesting permission to install the following %S items:
itemWarningIntroSingle=A web site is requesting permission to install the following item:

Not sure what we want for this one... I don't like the idea of having four strings to handle this. Perhaps making the strings more generic (e.g. remove "web site" from the strings).

Bug 302527 - this is an error that should only be seen by ext. authors or during nightly builds when there is a regression.
malformedRegistrationMessage=%S could not install this item because of a failure in Chrome Registration. Please contact the author about this problem.

Bug 292854 - is also an error that should only be seen by ext. authors. I'd like to remove the filename from the message since it is a temp file which has caused confusion.
%S could not install this item because "%S" (provided by the item) is not well-formed or does not exist. Please contact the author about this problem.

For Bug 302527 and Bug 292854 I'd like something along the lines of:
%S was unable install this item. Please contact the author about this problem (additional details are available in the JavaScript Console).
Added comments to dependencies: bug 275629, bug 292854, and bug 302527. Going against what I said in comment 6, I think we should still keep error messages short and sweet, and expect that developers will know/learn about the error console. I think that's quite likely the case, and want to avoid a needlessly tecchie experience on the off-chance an end user hits these warnings.

For this bug:

uninstallWarningMessage=This will remove the %S extension.

uninstallWarningDependMessage=%S is required by one or more extensions. If you
continue, the following items will be disabled:

needsDependenciesDisabled=Disabled - missing one or more required extensions.

> Yo Mike, for the operation type messages I would like to go with the following
> which removes the redundant extension name which is displayed immediately 
> above the message.
> Will be upgraded when &brandShortName; is restarted
> Will be installed when &brandShortName; is restarted
> Will be uninstalled when &brandShortName; is restarted
> Will be enabled when &brandShortName; is restarted
> Will be disabled when &brandShortName; is restarted
> 
> r u down wit dat?

Almost. Add a "This" and make it "will" and tack a "." on the end and we're golden. :)

(and ta-daah, I'm no longer blocking Rob!)
(In reply to comment #13)
> against what I said in comment 6, I think we should still keep error messages
> short and sweet, and expect that developers will know/learn about the error
> console. I think that's quite likely the case, and want to avoid a needlessly
> tecchie experience on the off-chance an end user hits these warnings.

I should mention, though, that if it's possible to add a "Open Error Console" button which closes the error message and opens the Error Console, that'd be acceptable. So:
  
  .----------------------------------------------------------------.
  | Oh noes! Something went wrong. It must be Rob Strong's Fault!  |
  |                                                                |
  |                                ( Open Error Console ) ( OK )   |
  '----------------------------------------------------------------'

 - or -

  |  [ ] Open Error Console for more details                       |
  |                                                       ( OK )   |
  '----------------------------------------------------------------'

I suppose this could be done as a checkbox, but that just looks heavier and uglier to me.
(In reply to comment #14)
>  .----------------------------------------------------------------.
>  | Oh noes! Something went wrong. It must be Rob Strong's Fault!  |
>  |                                                                |
>  |                                ( Open Error Console ) ( OK )   |
>  '----------------------------------------------------------------'
If I can change that to "Robert Strong's" then I'm fine with that. :P

Benjamin, do all toolkit apps provide the error console?


(In reply to comment #13)
> uninstallWarningMessage=This will remove the %S extension.
This message is for extensions, themes, locales, and EM managed plug-ins. It also has a second line that states "Do you want to uninstall %s?" and having the "This will remove the %S extension." prior to that seems a bit redundant. How about just having the "Do you want to uninstall %s?"

> uninstallWarningDependMessage=%S is required by one or more extensions. If you
> continue, the following items will be disabled:
s/extensions/add-ons/
should items be changed to add-ons too? I could go either way.
 
> needsDependenciesDisabled=Disabled - missing one or more required extensions.
This is no longer used after the ui rewrite since now it can have multiple messages.

> > Yo Mike, for the operation type messages I would like to go with the following
> > which removes the redundant extension name which is displayed immediately 
> > above the message.
> > Will be upgraded when &brandShortName; is restarted
> > Will be installed when &brandShortName; is restarted
> > Will be uninstalled when &brandShortName; is restarted
> > Will be enabled when &brandShortName; is restarted
> > Will be disabled when &brandShortName; is restarted
> > 
> > r u down wit dat?
> 
> Almost. Add a "This" and make it "will" and tack a "." on the end and we're
> golden. :)
I'm going to hold off on this on for a day to try to come up with something a little less awkward... any help would be appreciated.
beltzner, I am leaning towards "This add-on will be... etc." instead of "This will be... etc." since the latter seems a bit too Yoda-like.
Attached image uninstall screenshot — 
beltzner, screenshots of the proposed changes to uninstall without the "This will remove the %S extension." text.
Attachment #225810 - Flags: ui-review?(beltzner)
beltzner, screenshots of the action messages so you can get a feel for it. This will be just seems too awkward. :)
Attachment #225814 - Flags: ui-review?(beltzner)
Attached patch patc (obsolete) — — Splinter Review 
mconnor, this implements what is seen in the two attached images.
Attachment #225840 - Flags: review?(mconnor)
Comment on attachment 225810 [details]
uninstall screenshot

Should there be a minimum width on the uninstall dialog? In the "Normal" case it looks a little unbalanced, like it could use some extra pixels on the right.
Attachment #225810 - Flags: ui-review?(beltzner) → ui-review+
Comment on attachment 225814 [details]
Screenshot - action messages

Yes, you're right, the alternative text is better. Please use that!
Attachment #225814 - Flags: ui-review?(beltzner) → ui-review+
(In reply to comment #15)
> (In reply to comment #14)
> >  .----------------------------------------------------------------.
> >  | Oh noes! Something went wrong. It must be Rob Strong's Fault!  |
> >  |                                                                |
> >  |                                ( Open Error Console ) ( OK )   |
> >  '----------------------------------------------------------------'
> If I can change that to "Robert Strong's" then I'm fine with that. :P
> 
> Benjamin, do all toolkit apps provide the error console?

The other way to do this -- assuming that you're sent the full error message text and assuming that you can extend the error dialog to support it -- would be as shaver suggested:

  .----------------------------------------------------------------.
  | Oh noes! Something went wrong. It must be Rob Strong's Fault!  |
  |                                                                |
  |                                      ( Show Details ) ( OK )   |
  '----------------------------------------------------------------'

 - clicking details reveals .. -

  |  .---------------------------------------------------------.   |
  |  |                                                         |   |
  |  |  <error text that can be copied and pasted>             |   |
  |  |                                                         |   |
  |  |                                                         |   |
  |  |                                                         |   |
  |  '---------------------------------------------------------'   |
  |                                                                |
  |                                      ( Hide Details ) ( OK )   |
  '----------------------------------------------------------------'

(In reply to comment #20)
> (From update of attachment 225810 [details] [edit])
> Should there be a minimum width on the uninstall dialog? In the "Normal" case
> it looks a little unbalanced, like it could use some extra pixels on the right.
I was thinking that there should be and will add it. Thanks.
Version: Trunk → 2.0 Branch
(In reply to comment #22)
To be honest I'm not that interested in adding this additional complexity to the EM for the small percentage of people that would find value in it. I think features like this belong in an extension developers kit which might be comprised of a set of extensions, possibly be a XULRunner app, or something else.
Whiteboard: 1d → [needs review]
Comment on attachment 225840 [details] [diff] [review]
patc

Benjamin, mconnor is a bit swamped with review requests... could you review this? Thanks
Attachment #225840 - Flags: review?(mconnor) → review?(benjamin)
Attached patch updated patch — — Splinter Review 
Seth, Benjamin is off today and this has late l10n changes... could you review this?
Attachment #226861 - Flags: review?(sspitzer)
Comment on attachment 226861 [details] [diff] [review]
updated patch

r=sspitzer

do you need me to review that other patch to?
Attachment #226861 - Flags: review?(sspitzer) → review+
Attachment #225840 - Attachment is obsolete: true
Attachment #225840 - Flags: review?(benjamin)
nope and thanks for the review!
Whiteboard: [needs review]
Checked in to trunk
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Comment on attachment 226861 [details] [diff] [review]
updated patch

More late-l10n strings. :(
Attachment #226861 - Flags: approval1.8.1?
Attachment #226861 - Flags: approval1.8.1? → approval1.8.1+
Checked in to MOZILLA_1_8_BRANCH for Firefox 2.0
Keywords: fixed1.8.1
Product: Firefox → Toolkit
This bug seems to have been for 1.8.1, no need to keep it open any longer
agreed
No longer depends on: 292854
Status: RESOLVED → VERIFIED
Whiteboard: [AOMTestday]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: