112869 - No UI to uninstall language packs

Status: VERIFIED FIXED

(SeaMonkey :: General, defect, P1)

Description

[tao]

Currently, there is no UI in the product, including ProfileManager, Prefs dlg,
and View menu to un-install the obsolete packs.

Comment 1

[dbragg]

Attachment: Added uninstall and UI code to pref-contentpacks.xul and .dtd

The UI was getting pretty cramped so the uninstall buttons are over on the
left.  Otherwise the list boxes get very small and the dialog gets just plain
ugly.

Comment 2

[dbragg]

Attachment: Removed the code that prevents obsolete pack selection

On the first patch I forgot that I made it so that the user couldn't
accidentally select an obsolete pack.  This prevents them from selecting a pack
to uninstall. Catch-22.  Tao and I believe that since there's a "(needs
update)" message next to obsolete packs it would be best to allow the user to
select that pack for uninstallation.

Comment 3

[tao]

Comment on attachment 87288 [details] [diff] [review]
Removed the code that prevents obsolete pack selection

r=tao

Comment 4

[Daniel Veditz [:dveditz]]

There may someday be a XPInstall package uninstaller that will do this.

Comment 5

[Daniel Veditz [:dveditz]]

Comment on attachment 87288 [details] [diff] [review]
Removed the code that prevents obsolete pack selection

Has the UI change gotten mcarlson and adt approval?

Has making this cramped dialog more cramped gotten UE
approval? Seems late for this kind of change w/out checking
with a lot of folks. Please post a screen shot of the
updated dialog and check w/Lori's team (and probably
Trudelle's team who own the prefs dialogs as a whole).

>+    function UninstallContentPack()
>+    {
>+      var contentList = document.getElementById("contentPackList");
>+      var selectedContentItem = contentList.selectedItems[0];
>+      var contentPackName = selectedContentItem.getAttribute("value");
>+      dump("The content name is: " + contentPackName + "\n");
>+      var chromeRegistry = Components.classes["@mozilla.org/chrome/chrome-registry;1"].getService(Components.interfaces.nsIXULChromeRegistry);
>+      chromeRegistry.uninstallLocale(contentPackName, false);
>+      contentList.selectedIndex = 0;
>+    }

Why do you assume global chrome? The language pack install
scripts will put them in the profile if they can't write
to the global location (fairly common on Unix). I don't know
how you could tell if a locale was global or profile ahead of
time, but you should at least check for errors from the call
to uninstall. Maybe you could try the other one after that?

When does the item get removed from the list? Does the uninstall
call itself force a UI refresh on the data source or something?

What happens if you uninstall the currently selected locale?

What happens if you uninstall the last locale?

Excuse my paranoia, but I remember there being some reason we
don't allow users to uninstall the global skins, and skins
are much easier to deal with in the chrome system than locales.

>+    function UninstallLanguagePack()

Same worries.

>+      dump("The language name is: " + languageName + "\n");

Only leave dumps if you are fully prepared for a visit from
nazis^H^H^H^H^Hrepresentatives from the Dump-dump() campaign.

Comment 6

[dbragg]

Good review Dan.  I was trying to get this done before I leave for my new group
but now I'm thinking that it needs to be thought out more.  You raised some good
points. (Your paranoia is well founded.)

In the current patch (87288) I had forgotten that locales can be installed into
the profile.  My system obviously doesn't have that situation so I set the
second parameter to "true" for global.

The item is removed from the list immediately.

I didn't test removal of the currently selected locale or the last one.  Again,
haste makes waste.

Let me talk to Tao about what he wants to do with this.  It seems like there
should be a bunch of players involved (UE, xpfe, etc.)

Comment 7

[tao]

new owner ->tao

Comment 8

[[e-mail] (away - not reading bugmail)]

Tao,

I might be a good owner for this, tentatively reassigning.

Comment 9

[[e-mail] (away - not reading bugmail)]

Attachment: New Content & Language Pack Preference Panel

screenshot capturing the proposed modifications to the preference panel

Comment 10

[[e-mail] (away - not reading bugmail)]

Attachment: Updated patch

Albeit this patch should be good enough for 1.2a, we might want to clean up and
optimize the panel for 1.2b. I have some ideas for that and hope to be able to
return to it.

Dan, could you please comment on this?

Comment 11

[[e-mail] (away - not reading bugmail)]

Attachment: yet another patch iteration

Comment 12

[[e-mail] (away - not reading bugmail)]

Attachment: clean-up

Comment 13

[[e-mail] (away - not reading bugmail)]

nhotta/cmanske/timeless - I just got UI sign-off from Marlon. Would you have a
few minutes for a quick review?

This enhancement one of the top i18n/l0n requests. I also own bug 161508 now and
will try to complete it in the 1.2 time frame, but that's up in the air since
dprice left.

I'll file a follow-up bug to move more frequently used service references to
nsPrefwindow and weed out some of the unnecessary code from individual prefpanels.

Comment 14

[Robert Kaiser]

I just looked through the latest patch, and AFAICT it looks good, nice work!

What came to my mind when reading:
1) Is there a bug about implementing uninstall for globally installed themes? We
currently can only uninstall profile-installed themes and if it works for
locale, it should work for themes as well...
2) Wouldn't it be better to also do a reordering of that panel so that laguage
packs are above content packs? Should I open a new bug for that?

BTW, shouldn't you obsolete the attachment named "yet another patch iteration"?

Comment 15

[[e-mail] (away - not reading bugmail)]

Comment on attachment 96026 [details] [diff] [review]
yet another patch iteration

obsoleting

Comment 16

[[e-mail] (away - not reading bugmail)]

Attachment: another round: this should swap content/lang pack listbox order and iron out some nits

Comment 17

[[e-mail] (away - not reading bugmail)]

I've been pinging a number of people for a review. Timeless sent some private
comments, which have been largely incorporated into the patch. Clearly, some
progress on the reviewing front needs to occur, if we want to have this in 1.2.

Anyone up for a r=?

Comment 18

[Charles Manske]

I've looked it over and don't see any problems offhand, but my build is 
currently horked and I plan to update everything tomorrow. As soon as I have
bits to test tomorrow I'll be happy to provide the r=.

Comment 19

[[e-mail] (away - not reading bugmail)]

Charlie, that would be great :-)

Comment 20

[[e-mail] (away - not reading bugmail)]

Attachment: missed pref-contentpacks.dtd and nsPrefWindow.js in the last patch

Comment 21

[[e-mail] (away - not reading bugmail)]

Attachment: sigh, this has been bit rotting for too long now - synching up with a current trunk pull

Comment 22

[[e-mail] (away - not reading bugmail)]

Charlie, I've just addressed some bitrot issues - the patch applies cleanly to
and works well with a current trunk build...

We really need to get this on the 1.2 train ASAP.

Comment 23

[[e-mail] (away - not reading bugmail)]

looks like I might be able to get this r= later today :-)

cc'ing alecf, hopefully he might have some time to sr= this later this week.

Comment 24

[[e-mail] (away - not reading bugmail)]

Attachment: Most recent screenshot (reflects kairo's request to reverse listbox order)

Comment 25

[tao]

Comment on attachment 100011 [details] [diff] [review]
sigh, this has been bit rotting for too long now - synching up with a current trunk pull

please verify your patch on Linux and Mac.

Comment 26

[Alec Flett]

Comment on attachment 100011 [details] [diff] [review]
sigh, this has been bit rotting for too long now - synching up with a current trunk pull

sr=alecf

Comment 27

[[e-mail] (away - not reading bugmail)]

OK, closing this at last. 
Thanks everyone!

Comment 28

[Robert Kaiser]

OK, I just tested this on my recent CVS build:
I can uninstall the non-selected language pack (I tried en-US), it is removed
from the list, and it gets un-listed in chrome.rdf's locale:root sequence. The
components of it are still in chrome.rdf, but I think that's intended, as well
as the files still being left in the directory.

verifying