561600 - (SMAddonMgr) [Tracker] Implement the New Addons Manager UI for SeaMonkey (about:addons)

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect, P2)

Description

[Justin Wood (:Callek)]

Attachment: WIP 1

Comment 2

[Robert Kaiser]

Comment on attachment 441328 [details] [diff] [review]
WIP 1

> ; JavaScript components
>+@BINPATH@/components/amManager.js
>+@BINPATH@/components/amContentHandler.js
>+@BINPATH@/components/amWebInstallListener.js

Nit: Please order those alphabetically.

Comment 3

[Serge Gautherie (:sgautherie)]

Ftr, a bunch of bug 553169 patches just landed,
and they seem to have more than the current work-in-progress patch, like:
{
-@BINPATH@/components/nsExtensionManager.js
+@BINPATH@/components/addonManager.js
}

Comment 4

[Justin Wood (:Callek)]

(In reply to comment #3)
> Ftr, a bunch of bug 553169 patches just landed,
> and they seem to have more than the current work-in-progress patch, like:
> {
> -@BINPATH@/components/nsExtensionManager.js
> +@BINPATH@/components/addonManager.js
> }

Thanks but I was tracking those other changes too. They landed on the branch in the past 48 hours.

Comment 5

[Jens Hatlak (:InvisibleSmiley)]

Adjusting summary so that I can find this again.

Callek already noted in his patch that the Add-on Manager should open in a tab. One thing to note is that we have the menu item in non-browser windows as well so we should probably focus the browser window and new tab in any case (whether we're coming from e.g. MailNews or a browser window).

Secondly, do we already have a bug for the required Modern changes?

BTW: Bug 465090 is about adding a shortcut for the Add-on Manager to Firefox. I think we should keep an eye on that, too, especially to see whether we can use the same shortcut (in another bug of course).

Comment 6

[Robert Kaiser]

Well, the first thing is to fix things up so they how somewhat at all, which probably needs packaging and some other ground-laying changes that are in the patch attached here. Even the "open in a tab instead of a window" thing could be yet another bug/patch, IMHO.
Esp. in the light of an alpha freeze upcoming I'd like to see thing sliced a bit, so the meat (or actual cake?) can go in before the icing.

Comment 7

[[e-mail]]

Attachment: Apply Theme menu [Checked in]

Note that this requires a fix for at least 563256, and preferably 563241 too.

Comment 8

[Robert Kaiser]

The new EM has been backed out, so not blocking a1 on it (unless it relands before we can cut the relbranch)

Comment 9

[Dave Townsend [:mossop]]

Comment on attachment 441328 [details] [diff] [review]
WIP 1

Not a full review of course but this looks good to me. Do you guys have a removed-files.in? You'll probably want to update that too.

>diff --git a/suite/browser/browser-prefs.js b/suite/browser/browser-prefs.js
> pref("xpinstall.whitelist.add.103", "addons.mozilla.org");
> 
> pref("xpinstall.whitelist.add", "update.mozilla.org");

Note that you probably just want to set xpinstall.whitelist.add to addons.mozilla.org. update.mozilla.org doesn't need to be whitelisted anymore and these settings only affect new profiles.

>diff --git a/suite/installer/package-manifest.in b/suite/installer/package-manifest.in

> ; JavaScript components
>+@BINPATH@/components/amManager.js
>+@BINPATH@/components/amContentHandler.js
>+@BINPATH@/components/amWebInstallListener.js
> @BINPATH@/components/contentAreaDropListener.js
> @BINPATH@/components/contentSecurityPolicy.js
> @BINPATH@/components/crypto-SDR.js

Note that we've renamed the amManager.js to addonManager.js.

Comment 10

[Dave Townsend [:mossop]]

Comment on attachment 443057 [details] [diff] [review]
Apply Theme menu [Checked in]

This looks good to me

Comment 11

[[e-mail]]

Comment on attachment 443057 [details] [diff] [review]
Apply Theme menu [Checked in]

>+    popup.removeChild(aPopup.lastChild);
Note to self: popup.lastChild

Comment 13

[Justin Wood (:Callek)]

(In reply to comment #12)
> *** Bug 565090 has been marked as a duplicate of this bug. ***

Fwiw I should have _that_ bug fixed before this one is fully fixed, but it was the correct dupe anyway.

Comment 14

[Justin Wood (:Callek)]

Attachment: pref and packaging [checked in]

Robert, I'm not really sure if I need sr here because of pref changes.

This patch should make (most) things not fail in our nightlies, there is still more work to do here though.

Just requesting feedback from Mossop for sanity, should not block this.

Comment 15

[Justin Wood (:Callek)]

(In reply to comment #9)
> (From update of attachment 441328 [details] [diff] [review])
> >diff --git a/suite/browser/browser-prefs.js b/suite/browser/browser-prefs.js
> > pref("xpinstall.whitelist.add.103", "addons.mozilla.org");
> > 
> > pref("xpinstall.whitelist.add", "update.mozilla.org");
> 
> Note that you probably just want to set xpinstall.whitelist.add to
> addons.mozilla.org. update.mozilla.org doesn't need to be whitelisted anymore
> and these settings only affect new profiles.

Just to be clear, I should remove "xpinstall.whitelist.add.103" and change the "update.mozilla.org" to the addons site instead?

Comment 16

[Justin Wood (:Callek)]

Attachment: Build Config, and Notification bar [Checked in]

Comment 17

[Justin Wood (:Callek)]

Comment on attachment 443057 [details] [diff] [review]
Apply Theme menu [Checked in]

Without yet having tested it, I'm slightly skeptical about the enable code actually enabling the addon. [could be a misunderstanding of the API here too though]  But even if that part is broken, this is much much better than present.

Comment 18

[[e-mail]]

Comment on attachment 444830 [details] [diff] [review]
pref and packaging [checked in]

>+pref("extensions.webservice.discoverURL", "https://services.addons.mozilla.org/%LOCALE%/%APP%/discovery");
Unfortunately since changed in changeset 08696f911422. sr=me with that fixed.

Comment 19

[[e-mail]]

Comment on attachment 444831 [details] [diff] [review]
Build Config, and Notification bar [Checked in]

>-    if (gPrefs.getBoolPref("xpinstall.whitelist.required"))
>-      return BLOCK;
>-    return ALLOW;
>+    try {
>+      if (!gPrefs.getBoolPref("xpinstall.whitelist.required"))
>+        return ALLOW;
>+    }
>+    catch (e) {
>+    }
>+    return BLOCK;
I don't think we need this change; Mossop moved the pref from xpinstall.js (which no longer exists) to all.js so that it still exists.

>-                if (!this._prefs.getBoolPref("xpinstall.enabled")) {
>+                var enabled = true;
>+                try {
>+                  enabled = gPrefService.getBoolPref("xpinstall.enabled");
>+                }
>+                catch (e) {
>+                }
>+                if (!enabled) {
Same goes for this change.

>-
>+            
Adds whitespace.

Comment 20

[[e-mail]]

Comment on attachment 444831 [details] [diff] [review]
Build Config, and Notification bar [Checked in]

Well, the notification works, and failed installs work, but I couldn't find an add-on compatible with 2.1a2pre ;-)

Comment 21

[Dave Townsend [:mossop]]

(In reply to comment #15)
> (In reply to comment #9)
> > (From update of attachment 441328 [details] [diff] [review] [details])
> > >diff --git a/suite/browser/browser-prefs.js b/suite/browser/browser-prefs.js
> > > pref("xpinstall.whitelist.add.103", "addons.mozilla.org");
> > > 
> > > pref("xpinstall.whitelist.add", "update.mozilla.org");
> > 
> > Note that you probably just want to set xpinstall.whitelist.add to
> > addons.mozilla.org. update.mozilla.org doesn't need to be whitelisted anymore
> > and these settings only affect new profiles.
> 
> Just to be clear, I should remove "xpinstall.whitelist.add.103" and change the
> "update.mozilla.org" to the addons site instead?

Correct, you may also wish to add the personas site to the whitelist but that is of course your decision, see how we have done here: http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js#158

The actual names of the prefs doesn't matter with the new add-ons manager too. Any xpinstall.whitelist.* pref will be read.

Comment 22

[Dave Townsend [:mossop]]

Comment on attachment 444830 [details] [diff] [review]
pref and packaging [checked in]

Looks good to me, though as Neil mentioned the discovery URL ended up getting changed again.

Comment 23

[Justin Wood (:Callek)]

Comment on attachment 444830 [details] [diff] [review]
pref and packaging [checked in]

Pushed with nits: http://hg.mozilla.org/comm-central/rev/52cc56729f2e

Comment 24

[Justin Wood (:Callek)]

Comment on attachment 444831 [details] [diff] [review]
Build Config, and Notification bar [Checked in]

Pushed with nit: http://hg.mozilla.org/comm-central/rev/052429ec3c44

Comment 25

[Justin Wood (:Callek)]

Comment on attachment 443057 [details] [diff] [review]
Apply Theme menu [Checked in]

Pushed with nits: http://hg.mozilla.org/comm-central/rev/3c6a14ad087b

Comment 27

[Tony Mechelynck [:tonymec]]

Attachment: screenshot (interim progress report)

Interim progress report for:
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a5pre) Gecko/20100513 Lightning/1.0b2pre SeaMonkey/2.1a2pre
Build ID:   20100513012344

Good points:
- A lot of progress in three days :-) <thumbsup/>
- Both the new URL about:addons and the old one chrome://mozapps/content/extensions/extensions.xul produce the same display. (This is good for people like me, who had "jumped the gun" and were displaying the older version of the addons manager in a tab.)

Not so good but still dogfoodable:
- The layout leaves much to be desired (or is it my favourite theme's fault? Kairo's EarlyBlue). In particular the left side includes too much empty space and the right side is cramped. (see screenshot)
- The new addons manager is slow (several seconds between displaying the chrome and filling the right pane)

Bad points (for the next patches I suppose :-) )
- All extensions wake up enabled (the NS1:userDisabled="true" attributes which may be present in extensions.rdf are not taken into account AFAICT)
- The counts on the tabs in the new addons manager's sidebar are all zero (see left siode of the screenshot).

Overall note:
Full of promises, but not ready for prime time yet.

Personal opinion:
- Too Firefoxy-flashy for my taste, I prefer the former layout with the tabs on top, and the buttons only on the current row.
- Mel will have some work making MR-Tech Toolkit work again with all its addons-manager enhancements (or "hacks", as some will call them).

Comment 28

[Ruediger Lahl]

(In reply to comment #27)

> - Both the new URL about:addons and the old one
> chrome://mozapps/content/extensions/extensions.xul produce the same display.

But if you open the manager with 'Tools->AddOn-Manager' in a SeaMonkey-Trunk-Build, its only open a very small window.

> Bad points (for the next patches I suppose :-) )

Its not possible to install xpi's from your local drive.

Comment 29

[Robert Kaiser]

Hmm, seems I can't install an addon from the web right now as the notification bar doesn't come up. This is what error console says:

Fehler: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: chrome://communicator/content/bindings/notification.xml :: observe :: line 174"  data: no]
Quelldatei: chrome://communicator/content/bindings/notification.xml
Zeile: 174

Try yourself by trying to install one of the 2.1 Alpha 1 themes from http://www.kairo.at/download/mozskins

Comment 30

[[e-mail]]

(In reply to comment #19)
>(From update of attachment 444831 [details] [diff] [review])
>>+    try {
>>+      if (!gPrefs.getBoolPref("xpinstall.whitelist.required"))
...
>>+                try {
>>+                  enabled = gPrefService.getBoolPref("xpinstall.enabled");
Apparently Mossop doesn't believe in default preferences; he only bothered adding xpinstall.whitelist.required to all.js because Firefox's UI needed it. rs=me to add xpinstall.enabled to browser-prefs.js so that our UI works again.

Comment 31

[Justin Wood (:Callek)]

(In reply to comment #27)
> Not so good but still dogfoodable:
> - The layout leaves much to be desired (or is it my favourite theme's fault?
> Kairo's EarlyBlue).

Part theme, part your personal preferences ;-)

> - The new addons manager is slow (several seconds between displaying the chrome
> and filling the right pane)

I don't experience this, could be a factor of how many extensions you have.

> Bad points (for the next patches I suppose :-) )
> - The counts on the tabs in the new addons manager's sidebar are all zero (see
> left siode of the screenshot).

The "count" is not really that; and the feature that the '0' is attached to is not yet implemented. Its more of a "Get Attention" kind of count, or "These addons were updated" etc. type of thing.  It will be hidden in non-default themes [any that don't take advantage of it].

(In reply to comment #28)
> But if you open the manager with 'Tools->AddOn-Manager' in a
> SeaMonkey-Trunk-Build, its only open a very small window.

Known issue at the moment, I'll be fixing that before a2 is out.

> > Bad points (for the next patches I suppose :-) )
> 
> Its not possible to install xpi's from your local drive.

It is, though there is no "Install" button. You can File->Open the .xpi itself if you desire.

(In reply to comment #30)
> rs=me to add xpinstall.enabled to browser-prefs.js so that our UI works again.

Pushed as: http://hg.mozilla.org/comm-central/rev/225cad32a651

Comment 32

[Tony Mechelynck [:tonymec]]

Attachment: screenshot with SeaMonkey-Modern theme

In reply to comment #31

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a5pre) Gecko/20100516 Lightning/1.0b2pre SeaMonkey/2.1a2pre - Build ID: 20100516004718

This is the first (and, so far, only) Linux tinderbox-build since your push in comment #31 (I checked the Mercurial changeset in the tinderbox waterfall; but why isn't the build time reflected in the build ID?). I tested it on a copy of my "live" profile, the same one I used in bug 565090; the technical details of its extensions appear in human-readable HTML form in attachment 444727 [details].

In about:config, xpinstall.enabled is still not listed. Not sure if it ought to be.

I tried to install the NoScript extension (one of those I hadn't yet, whose AMO page didn't say it was incompatible with Sm 2.1a2pre). The install-delay popup came up, there was no addon-manager popup, my addon-manager tab has a new entry, "NoScript: Ready to install". All seems well so far in this respect.

All my extensions are again enabled, disregarding manual disables done before migrating.

The "cramped right-side" layout is also visible in the Modern theme distributed with SeaMonkey, see screenshot. However there is a separate bug about that kind of problems (also seen of Firefox), namely bug 562978, I'll follow layout problems there. I haven't tried the default theme today yet, I'll set it at next restart.

Version numbers of extensions and themes are missing.

The "Show more" button near the right of the EM has no visible effect when clicked, other than alternate between "Show more" and "Show less".

If there is something else you want to have specifically checked, just say what.

Comment 33

[Tony Mechelynck [:tonymec]]

P.S. On further check, it seems that most disabled extensions had remained disabled but not (for instance) Firebug.

Comment 34

[Tony Mechelynck [:tonymec]]

Attachment: screenshot with default theme

OK, with the default theme the "crampedness" is less obvious but mainly because the left part is in larger type. I'd prefer to see more items (of smaller height).

The subjective "slow load" effect is still there, maybe because the new EM appears in two steps: chrome first, then (after a noticeable wait) the content, leading to a subjective reaction like "Oh!... Has it lost all my extensions?... I think it has... Ah no, it fills up." OTOH, IIRC the "old" EM appeared all at once.

Comment 35

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Tony, I would assume that you are seeing bug 562933.

Comment 36

[Tony Mechelynck [:tonymec]]

(In reply to comment #35)
> Tony, I would assume that you are seeing bug 562933.

ah, thanks for the pointer; though in my case it's when changing from Extensions to Themes or vice-versa, not so much when starting up with about:addons as well as other tabs.

Comment 37

[Robert Kaiser]

(In reply to comment #27)
> - The layout leaves much to be desired (or is it my favourite theme's fault?
> Kairo's EarlyBlue).

The theme has not yet been updated for this change, though I did some work for it yesterday (available in my public git repo, the 2.1a1 release doesn't have that yet).

> - The counts on the tabs in the new addons manager's sidebar are all zero

Also a theme issue, as they should be hidden, right now by the theme, when the number there is zero.

Comment 38

[Joseph Deck]

I am just a user, not a developer, so please forgive me if this comment is out of order in this venue.

From this user's POV:
- the new layout of the add-on manager looks nice to me; it's cleaner and more modern than the old one.
- however, it has lost functionality:
   a. version number of the extensions is not shown (echoing and endorsing #32)
   b. when an extension has its own preference setup, it's not obvious (no button appears until user double-clicks)

In short, I'd rather have the reduced clicking and better discovery of the old manager over the fresh look of the new one.

Wouldn't it be a better trade-off to make the listing for each extension slightly larger, if needed (I'm not sure it would be) and forget about the double-click? I don't think the effort of moving these functions off board (both programming effort and double-clickage for the user) is justified by the really minimal added complexity of leaving them on the initial page.

Thanks...

Comment 39

[Tony Mechelynck [:tonymec]]

Of course it will be necesary to make the SeaMonkey-Modern theme work as nicely with the new UI as the default theme (as nicely functionally, I mean, though maybe with a "no-nonsense" look and feel more in line with Netscape / MozSuite / SeaMonkey "traditions"; maybe even with a slightly different layout, such as e.g. a single row of buttons, [See more][Preferences]|elastic space|[Enable/Disable][Uninstall], at the bottom of each entry rather than a cramped set in the right column...). I suppose such work belongs in a different bug; or will it be done here?

Comment 40

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #38)
>    a. version number of the extensions is not shown (echoing and endorsing #32)

Bug 562052.

>    b. when an extension has its own preference setup, it's not obvious (no

Bug 562890.

Comment 41

[Justin Wood (:Callek)]

Turning this into a tracker, its getting hard to follow as it stands.

Comment 42

[Robert Kaiser]

Attachment: package extensions library [Checked in]

The extensions library has been missed in packaging, I stumbled over that when taking a look at package-compare...

Comment 43

[Justin Wood (:Callek)]

Comment on attachment 449565 [details] [diff] [review]
package extensions library [Checked in]

how'd I miss this.

Comment 44

[Robert Kaiser]

Comment on attachment 449565 [details] [diff] [review]
package extensions library [Checked in]

Pushed as http://hg.mozilla.org/comm-central/rev/8b7742f9c3e3

Comment 45

[Justin Wood (:Callek)]

The _TRACKER_ here does not block a2.

Comment 46

[Justin Wood (:Callek)]

Done here, Bug 568052 remains, and is a final blocker.