580298 - Hide version numbers for lightweight themes

Status: VERIFIED FIXED

Description

[Ryan Doherty (:rdoherty)]

The add-ons manager shows the version number next to the add-on/theme name, which is fine for traditional add-ons, but lightweight themes don't have versions. There is effectively only one version of the lightweight theme. 

The version # that getpersonas.com supplies is the last modified timestamp. It's used to determine if there's an update. It should not be displayed as it's not useful for users.

http://grab.by/5vT5

Comment 1

[Henrik Skupin (:whimboo)]

It 's a highly visible ui issue. As Ryan said we shouldn't confuse users here. Asking for blocking fx4.

Comment 2

[Mano (::mano, needinfo? for any questions; not reading general bugmail)]

Hrm, I don't see the version number in my build after installing a lightweight theme.  However, i do see an empty version field in the details view.

Comment 3

[Ryan Doherty (:rdoherty)]

(In reply to comment #2)
> Hrm, I don't see the version number in my build after installing a lightweight
> theme.  However, i do see an empty version field in the details view.

I still see them in nightlies. Have you tried installing a new persona from getpersonas.com?

Comment 4

[Dave Townsend [:mossop] (he/him)]

This problem may only reveal itself after an update check is performed for the persona. We perform a check only for the active persona once a day.

Comment 5

[Mano (::mano, needinfo? for any questions; not reading general bugmail)]

Note: the fix here is held until some major theme-provider changes are landed.

Comment 6

[Dave Townsend [:mossop] (he/him)]

Those changes will be made in bug 585339

Comment 7

[Dave Townsend [:mossop] (he/him)]

I mean bug 520124

Comment 8

[Mano (::mano, needinfo? for any questions; not reading general bugmail)]

Created attachment 476590 [details] [diff] [review]
patch

Comment 9

[Dão Gottwald [::dao]]

Comment on attachment 476590 [details] [diff] [review]
patch

>+function shouldShowVersionNumber(aAddon) {
>+  if (!aAddon.version)
>+    return false;
>+
>+  // The version number is hidden for lightweight themes.
>+  if (aAddon.type == "theme")
>+    return !/@personas\.mozilla\.org/.test(aAddon.id);
>+}

This looks like it should return true at the bottom.

Comment 10

[Dão Gottwald [::dao]]

Also, adding $ to the regexp probably wouldn't hurt.

Comment 11

[Dave Townsend [:mossop] (he/him)]

Comment on attachment 476590 [details] [diff] [review]
patch

I agree with both of dao's comments. Could we get an automated test too?

Comment 12

[Mano (::mano, needinfo? for any questions; not reading general bugmail)]

Could you point me to some other tests for theme-type addons?

Comment 13

[Dave Townsend [:mossop] (he/him)]

(In reply to comment #12)
> Could you point me to some other tests for theme-type addons?

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/browser_bug562899.js is a test that uses a lightweight theme. You can probably get away with just simulating a normal theme using the MockProvider like most of the other browser chrome tests.

Comment 14

[Siddharth Agarwal [:sid0] (inactive)]

Created attachment 506845 [details] [diff] [review]
patch addressing review comments + test

How's this?

Comment 15

[Dave Townsend [:mossop] (he/him)]

Comment on attachment 506845 [details] [diff] [review]
patch addressing review comments + test

Looks good, thanks

Comment 16

[Siddharth Agarwal [:sid0] (inactive)]

Created attachment 507047 [details] [diff] [review]
patch + more tests

I wrote tests for the details view too. (There's also a typo fix in there -- addon -> aAddon).

Comment 17

[:Ehsan Akhgari]

http://hg.mozilla.org/mozilla-central/rev/a6a96e15c369

Comment 18

[Henrik Skupin (:whimboo)]

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b11pre) Gecko/20110127 Firefox/4.0b11pre by manually setting an older version number via about:config.