Closed Bug 562890 Opened 14 years ago Closed 14 years ago

Preferences button for the add-on should be placed in the entry of the digest view

Categories

(Toolkit :: Add-ons Manager, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla2.0b1
Tracking Status
blocking2.0 --- final+

People

(Reporter: alice0775, Assigned: Unfocused)

References

()

Details

(Whiteboard: [AddonsRewriteTestday][rewrite])

Attachments

(2 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a5pre) Gecko/20100425 Minefield/3.7a5pre ID:20100425040736
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a5pre) Gecko/20100425 Minefield/3.7a5pre ID:20100425040736

Preferences of the extension is at the deep hierarchy.it  should be in a list view.
I think Author/Rating/Date are not necessary in list view if space problem.
Alice, are you talking about the details view? Mockups are up but probably need a revisit. Some items have been changed meanwhile.
Keywords: uiwanted
OS: Windows 7 → All
Hardware: x86 → All
Whiteboard: [AddonsRewriteTestday][rewrite]
Preferences button of the extension  should be placed in Digest View
Summary: Preferences of the extension is at the deep hierarchy.There should be it in a list view → Preferences button for the add-on should be placed in the entry of the digest view
I Agree it would help a lot for the newbies a preference button at the left side of each add-on in the add-ons manager window.
Priority: -- → P2
Yes, this is fine for now.  I'll attach a mockup in a few
Will this preference button be aligned to the Remove and Disable buttons or at a fixed position? Aligning to the other buttons wouldn't be a good idea because the Remove button could be hidden (for globally installed addons) and causes the Preference button to move right.

Also if we go on with the idea on bug 562776 to not show buttons for list entries which are not selected, the Preference button should also only be visible for the currently selected entry.
(In reply to comment #6)
> Will this preference button be aligned to the Remove and Disable buttons or at
> a fixed position? Aligning to the other buttons wouldn't be a good idea because
> the Remove button could be hidden (for globally installed addons) and causes
> the Preference button to move right.

For add-ons that cannot be removed, the remove button should be disabled and greyed out rather than gone.  So, the spacing of the buttons should not change.

> Also if we go on with the idea on bug 562776 to not show buttons for list
> entries which are not selected, the Preference button should also only be
> visible for the currently selected entry.

This is unlikely to happen - I think we're safe to go ahead and implement Preferences to the left of disable and remove
Keywords: uiwanted
What are the chances of you swapping the position of the "remove" and "disable" buttons?  "Remove" is an action of last resort.  A person is more likely to disable, so I believe "remove" should be furthest from other buttons -- not in the middle of them.  Thanks.
(In reply to comment #8)
> What are the chances of you swapping the position of the "remove" and "disable"
> buttons?  "Remove" is an action of last resort.  A person is more likely to
> disable, so I believe "remove" should be furthest from other buttons -- not in
> the middle of them.  Thanks.

What about one row of buttons at the bottom of the entry, maybe something like

[More info][Preferences]|...elastic space...|[Enable/Disable][Uninstall]

(either the selected entry as before the rewrite, or every entry)? This would make the buttons look less "all cramped in a small space at right". This proposal would also put "Uninstall" at the end of the row, i.e., farthest from "the middle of them". (Whether to call it "Remove" or "Uninstall", or the one in some themes and the other in others, is a "novelty vs. tradition" question, IMHO of only cosmetic importance. On SeaMonkey which comes with two themes packed-in, I imagine that the "default" theme, which looks more like Firefox, could call it "Remove", while the "Modern" theme, which IMHO looks more like NS/MozSuite, and thus is actually of more "traditional" look-and-feel, could call it "Uninstall". But that's only my imagination running wild.)
blocking2.0: --- → final+
Attached patch Patch v1 (obsolete) — Splinter Review
Note that I'm passing in the addon object to doCommand(), since consumers might call that without the addon item being selected. Should have allowed doing that in the first place.
Assignee: nobody → bmcbride
Status: NEW → ASSIGNED
Attachment #448677 - Flags: review?(dtownsend)
Comment on attachment 448677 [details] [diff] [review]
Patch v1

>diff --git a/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd b/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd

> <!ENTITY detail.updateAutomatically.label     "Update automatically">
> <!ENTITY detail.updateAutomatically.accesskey "U">
> <!ENTITY detail.updateAutomatically.tooltip   "Update this add-on automatically whenever there is an update available">
> <!ENTITY detail.checkForUpdates.label         "Check for updates">
> <!ENTITY detail.checkForUpdates.accesskey     "F">
> <!ENTITY detail.checkForUpdates.tooltip       "Check for updates for this add-on">
>-<!ENTITY detail.showPreferences.label         "Preferences">
>-<!ENTITY detail.showPreferences.accesskey     "P">
>-<!ENTITY detail.showPreferences.tooltip       "Change this add-on's preferences">

Double check this with Pike. We may have to keep both strings around as we are showing the button in two different contexts.

>diff --git a/toolkit/mozapps/extensions/content/extensions.xml b/toolkit/mozapps/extensions/content/extensions.xml

>       <method name="_updateState">
>         <body><![CDATA[
>           this.setAttribute("incompatible", !this.mAddon.isCompatible);
> 
>+          this._preferencesBtn.hidden = !(this.mAddon.isActive &&
>+                                          this.mAddon.optionsURL);

The API should be returning null for optionsURL on disabled add-ons so I think rely on that, on the off chance we add an add-on type that supports settings while disabled.
Attachment #448677 - Flags: review?(dtownsend) → review+
Attached patch Patch v1.1 (obsolete) — Splinter Review
Finally got around to fixing this up and adding a test.
Attachment #448677 - Attachment is obsolete: true
Attachment #452624 - Flags: review?(dtownsend)
Comment on attachment 452624 [details] [diff] [review]
Patch v1.1

I think you need to add the access key to the XUL.
Attachment #452624 - Flags: review?(dtownsend) → review+
After discussion on IRC: the suggestion in comment 13 would cause bug 562776 (lets stop using "digest view" - its confusing). However, there was an unneeded accesskey entity in extensions.dtd - this updated patch removes that. Not landing yet, as the tree looks scary right now.
Attachment #452624 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/96ad49c97c4f
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a6
Verified fixed with trunk builds on all platforms like Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a6pre) Gecko/20100625 Minefield/3.7a6pre

Looks like a good automated test which doesn't need any Mozmill or manual test.
Status: RESOLVED → VERIFIED
Flags: in-litmus-
Running mochitest-browser-chrome on Ubuntu 10.04, on latest mozilla-central, debug build, I get a failure for the test for this bug,

 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug562890.js | Timed out
 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug562890.js | Found an unexpected tab at the end of test run: about:addons

However it appears to be running ok on tinderbox.
(In reply to comment #17)
> Running mochitest-browser-chrome on Ubuntu 10.04, on latest mozilla-central,
> debug build, I get a failure for the test for this bug,

Hmm, its constantly failing? The first failure would cause the second failure. Any idea where its timing out?
It constantly fails, yes.

Is there a straightforward way to see where it times out? Here is the full test output, maybe that can help:

TEST-START | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug562890.js
WARNING: Positioned frame that does not handle positioned kids; looking further up the parent chain: file /scratchbox/users/alon/home/alon/mozilla-central/layout/base/nsCSSFrameConstructor.cpp, line 5584
pldhash: for the table at address 0x4524c6b8, the given entrySize of 48 probably favors chaining over double hashing.
++DOCSHELL 0x4524c650 == 8
++DOMWINDOW == 14 (0x46e10d38) [serial = 14] [outer = (nil)]
++DOMWINDOW == 15 (0x46e10eb8) [serial = 15] [outer = 0x46e10d00]
WARNING: Positioned frame that does not handle positioned kids; looking further up the parent chain: file /scratchbox/users/alon/home/alon/mozilla-central/layout/base/nsCSSFrameConstructor.cpp, line 5584
WARNING: Positioned frame that does not handle positioned kids; looking further up the parent chain: file /scratchbox/users/alon/home/alon/mozilla-central/layout/base/nsCSSFrameConstructor.cpp, line 5584
WARNING: Positioned frame that does not handle positioned kids; looking further up the parent chain: file /scratchbox/users/alon/home/alon/mozilla-central/layout/base/nsCSSFrameConstructor.cpp, line 5584
WARNING: Positioned frame that does not handle positioned kids; looking further up the parent chain: file /scratchbox/users/alon/home/alon/mozilla-central/layout/base/nsCSSFrameConstructor.cpp, line 5584
++DOMWINDOW == 16 (0x482834b8) [serial = 16] [outer = 0x46e10d00]
pldhash: for the table at address 0x4524cc88, the given entrySize of 48 probably favors chaining over double hashing.
++DOCSHELL 0x4524cc20 == 9
++DOMWINDOW == 17 (0x48283ab8) [serial = 17] [outer = (nil)]
WARNING: NS_ENSURE_TRUE(chromeWindow) failed: file /scratchbox/users/alon/home/alon/mozilla-central/content/base/src/nsFrameLoader.cpp, line 1905
WARNING: NS_ENSURE_TRUE(mutableURL) failed: file /scratchbox/users/alon/home/alon/mozilla-central/content/xul/templates/src/nsXULContentUtils.cpp, line 338
++DOMWINDOW == 18 (0x482849b8) [serial = 18] [outer = 0x48283a80]
TEST-PASS | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug562890.js | Should have an add-ons manager window
TEST-PASS | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug562890.js | Should be displaying the correct UI
LoadPlugin() /scratchbox/users/alon/home/alon/m-c-ff/modules/plugin/test/testplugin/libnptest.so returned 48ee2ae0
GetMIMEDescription() returned "application/x-test:tst:Test mimetype"
LoadPlugin() /usr/lib/mozilla/plugins/librhythmbox-itms-detection-plugin.so returned 493112c0
GetMIMEDescription() returned "application/itunes-plugin::;"
LoadPlugin() /usr/lib/flashplugin-installer/libflashplayer.so returned 493113a0
GetMIMEDescription() returned "application/x-shockwave-flash:swf:Shockwave Flash;application/futuresplash:spl:FutureSplash Player"
TEST-PASS | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug562890.js | Prefs button should be hidden for addon with no optionsURL set
TEST-PASS | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug562890.js | Prefs button should be shown for addon with a optionsURL set
*** loading ISO8601DateUtils
--DOMWINDOW == 17 (0x46e0e7b8) [serial = 10] [outer = 0x46e0d100] [url = about:blank]
--DOMWINDOW == 16 (0x46e10eb8) [serial = 15] [outer = 0x46e10d00] [url = about:blank]
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug562890.js | Timed out
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug562890.js | Found a tab after previous test timed out: about:addons
WARNING: NS_ENSURE_TRUE(bCollapsed) failed: file /scratchbox/users/alon/home/alon/mozilla-central/editor/libeditor/text/nsTextEditRules.cpp, line 1009
TEST-INFO | checking window state
TEST-PASS | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug562890.js | before wait for focus -- loaded: complete active window: ([object ChromeWindow @ 0x45c45300 (native @ 0x43a36eb8)]) chrome://browser/content/browser.xul focused window: ([object XPCNativeWrapper [object Window @ 0x47c9eec0 (native @ 0x46e0d138)]]) about:blank desired window: ([object ChromeWindow @ 0x45c45300 (native @ 0x43a36eb8)]) chrome://browser/content/browser.xul child window: ([object XPCNativeWrapper [object Window @ 0x47c9eec0 (native @ 0x46e0d138)]]) about:blank docshell visible: true
TEST-PASS | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug562890.js | already focused
TEST-PASS | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug562890.js | maybe run tests <load:true, focus:true> -- loaded: complete active window: ([object ChromeWindow @ 0x45c45300 (native @ 0x43a36eb8)]) chrome://browser/content/browser.xul focused window: ([object XPCNativeWrapper [object Window @ 0x47c9eec0 (native @ 0x46e0d138)]]) about:blank desired window: ([object ChromeWindow @ 0x45c45300 (native @ 0x43a36eb8)]) chrome://browser/content/browser.xul child window: ([object XPCNativeWrapper [object Window @ 0x47c9eec0 (native @ 0x46e0d138)]]) about:blank docshell visible: true
WARNING: Positioned frame that does not handle positioned kids; looking further up the parent chain: file /scratchbox/users/alon/home/alon/mozilla-central/layout/base/nsCSSFrameConstructor.cpp, line 5584
WARNING: Positioned frame that does not handle positioned kids; looking further up the parent chain: file /scratchbox/users/alon/home/alon/mozilla-central/layout/base/nsCSSFrameConstructor.cpp, line 5584
Depends on: 575524
Depends on: 604136
You need to log in before you can comment on or make changes to this bug.