116967 - [rfe]Use unique icons for items in the Start Menu

Status: VERIFIED FIXED

(SeaMonkey :: Installer, enhancement)

Description

[Curt Patrick (gone)]

We want each item in the Start Menu to have a unique icon.  Before this can be
done we need to have the icons created and I KNOW that mozilla doesn't want  me
to do that!  I'm going to let Gregg figure out who does that work around here,
then I'll have the installer use them

Comment 1

[André Dahlqvist]

When these icons are created, will they work equally well on Linux, such as in
the KDE menu?

Comment 2

[Grace Bush]

adding myself as QA contact to track for project

Comment 3

[Gregg Landskov (gone)]

reassigning to Lori Kaplan.  Spec posted at: 
http://client.mcom.com/machv/xpinstall/desktop_integration.html

Lori, is this ok?

Comment 4

[Lori Kaplan]

I'll have to talk with Sol and Todd. I know that we have moved away from a
separate brand for each component so I think we need to consider what having
unique icons means. 

Comment 5

[Lori Kaplan]

reassigning to Marlon. Consulted with Todd. We all agree this is a good
strategy. Will develop the icons in the 1.0 timeframe. 

Comment 6

[marlon bishop]

i assume this meant "Mozilla" icons.  Lori -  we have netscape component icons
already, just need to make use of them

Comment 7

[Curt Patrick (gone)]

Gregg and Joe pointed me to the icons for Netscape.  I still need icons for
Mozilla, though (or, at least, pointed to them).

I need the command-lines for the following new shortcuts, and verification as to
which component each shortcut should be installed with:
- AIM : when nim.xpi is installed?
- Composer : when browser.xpi is installed?
- Address Book : when mail.xpi is installed?
- Help : when browser.xpi is installed?

...and I need verification that we still want shortcuts to the readme and license.

Comment 8

[Curt Patrick (gone)]

Attachment: 0.1 patch for ns

This has all icons for everything except Help.	I still haven't figured out the
command-line for that yet.  But it will be a pretty straightforward addition. 
Also, this includes links to readme and licencs 'cause noone has told me to
remove them.

In the meantime, I need to get the hard stuff reviewed, especially in nim.jst. 
It did not have any code for creating shortcuts.  I had to cut and paste from
another jst that did, but found a bug in the code along the way.  So it is not
a simple cut and paste and a pretty careful review.  The bug had to do with
setting the variables winreg, is_winnt, restrictedAccess.  I made them globals
and created some helper functions so they only get set once and, hopefully,
correctly.

One other thing:  Do the Descriptons which show up in the Start menu need to be
resourced for intl so they can be translated?
+

Comment 9

[Curt Patrick (gone)]

*** Bug 117105 has been marked as a duplicate of this bug. ***

Comment 10

[Sean Su]

Comment on attachment 70166 [details] [diff] [review]
0.1 patch for ns

Things I noticed:

A nit picky thing.  I saw that you renamed a var from 'scProfileDescParam' to
'scProfileParam' in browser.jst.  In the mail.jst, for consistence, could you
rename 'scMailDescParam' to 'scMailParam'?


In mail.jst, in the function 'createShortcut', there is a part of code that is
commented out:

     //
-    // Disabled for now because mail does not have a different shortcut icon
from Netscape 6
+    // Disabled for now
     //
     //// create shortcut in the Quick Launch folder
     //if(folderQuickLaunchExists)
-    //  File.windowsShortcut(fileExe, fFolderQuickLaunch, scExeDesc, fProgram,
 "", fileExe, 0);
+    //  File.windowsShortcut(fileExe, fFolderQuickLaunch, scMailDesc,
fProgram,  "", fileExe, 0);

Please uncomment out this code.  I noticed that you only updated the
'scMailDesc' var in the call to File.windowsShortcut(). Make sure you update
the rest of the parameters too when uncommenting out the code.
Same thing for nim.jst in its 'createShortcut' function.


In nim.jst try to start denoting the globals appropriately.  I know I did a
horrible job at it, but you should start doing it correctly tho.  I would
suggest 'gWinReg' instead of 'winreg', 'gIsWinnt' instead of 'is_winnt', and so
on...

Also make sure that in the functions that will use them, they check that the
globals are valid (where appropriate) before using them.

Comment 11

[Curt Patrick (gone)]

As per email with Steve, the Help icon will be titled "Help and Support".

Comment 12

[Curt Patrick (gone)]

Attachment: 0.2 for ns (complete)

This has all the shortcuts which I believe have been requested.  Further, since
it sounds like we're leaning toward leaving the readme and license shortcuts in
anyway (?) they are still in this patch.  If someone decides they don't belong
you can open another bug to remove them--which is what I'd have to do anyway. 
In the meantime, I did notice that I was not removing the readme and license
links before installing, which is the case for the other links, so I corrected
that in this patch.

Sean, this patch addresses all of the problems you identified also.

Comment 13

[Curt Patrick (gone)]

Oh yeah.  I didn't get an answer as to whether any of the descriptions need to
be translatable.  I don't see that the existing shortcut descriptions are
resourced.  On the other hand, "Help & Support" looks suspiciously like
something that some someone is going to want translated.

Comment 14

[Sean Su]

Comment on attachment 70387 [details] [diff] [review]
0.2 for ns (complete)

r=ssu (given what we talked about)

Comment 15

[Sean Su]

oh yeah, the translation question.  Well, normally, the descriptions should not
be localized, but as for 'Help & Support', that will probably need to be.

I don't think we have a way to allow the ability to localize strings easily in
install.js scripts.

You might want to think about creating a section in each .js file that groups
all the localizable strings in one place for convenience.  I believe that's how
it was done in 4.x .jar files.

Talk to the international folks and see what current tool set they use and what
they would be okay with.

Comment 16

[Daniel Veditz [:dveditz]]

We actually do have a loadResources() call in the script language, but IIRC
there were problems using it in the wizard environment (used features that
weren't in xpcom.xpi). We could probably re-write it so it would work using
nsIPersistentProperties (implemented in xpcom) instead of string bundles.

Up to now there haven't been many strings to translate, and those were mostly in
the language packs. In those scripts the translatable strings were grouped as a
bunch of vars in one place to make it easy to make a copy and edit the file
directly.

But property files sure would be nice.

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 70387 [details] [diff] [review]
0.2 for ns (complete)

Is there a Mozilla patch? I guess this should have been a Bugscape bug -- oh
well.

>   fileReadme                = getFolder("file:///", fProgram + "readme.txt");
>   fileLicense               = getFolder("file:///", fProgram + "license.txt");
>+  fileProfileIcon           = getFolder("file:///", fProgram + "chrome/icons/default/profileWindow.ico");
>+  fileComposerIcon          = getFolder("file:///", fProgram + "chrome/icons/default/editorWindow.ico");
>+  fileHelpIcon              = getFolder("file:///", fProgram + "chrome/icons/default/help.ico");

Why are you using "file:///" here? you have fProgram already, do
  getFolder( fProgram, "readme.txt" );
or
  getFolder( "Program", "chrome/icons/blah" );
or
  getFolder( "Chrome", "icons/default/blah" );

getFolder("file:///") is a last resort to break out of our box of
well known locations.

>   scReadmeDesc              = "Readme";
>   scLicenseDesc             = "License";
>   scProfileDesc             = "Profile Manager";
>+  scComposerDesc            = "Composer";
>+  scHelpDesc                = "Help & Support";
>   scFolderName              = "$ShortcutDesc$";

Please file a bug to do something about localizing these. Maybe we
just hunt for and group them, maybe we make loadResources() work.
Don't worry about fixing it for purposes of this bug.

>     logComment("fileExe                 : " + fileExe);
>     logComment("fFolderPath             : " + fFolderPath);
>     logComment("scExeDesc               : " + scExeDesc);
>+    logComment("scProfileDesc           : " + scProfileDesc);
>+    logComment("scProfileParam          : " + scProfileParam);
>+    logComment("scComposerDesc          : " + scComposerDesc);
>+    logComment("scComposerParam         : " + scComposerParam);
>+    logComment("scHelpDesc              : " + scHelpDesc);
>+    logComment("scHelpParam             : " + scHelpParam);
>     logComment("fProgram                : " + fProgram);

Do we really need to clutter up the install log with these additional
items? Since those are largely printing out strings you've hardcoded
into the script I don't see even debugging value, let alone anything like
the tech-support value of having the user's "fProgram" logged.

The clutter of logComments() in our scripts is a peeve of mine.

>Index: mail.jst
>+  fTemp        = fProgram + "$MainExeFile$";
>+  fileExe      = getFolder("file:///", fTemp);
>+  fileMailIcon = getFolder("file:///", fProgram + "chrome/icons/default/messengerWindow.ico");

As above these three lines should be two:
  fileExe      = getFolder( fProgram, "$MainExeFile$");
  fileMailIcon = getFolder( "Chrome", "icons/default/etc...");

>     logComment("Folder FolderQuickLaunch: " + szFolderQuickLaunch);
>     logComment("fileExe                 : " + fileExe);
>     logComment("fFolderPath             : " + fFolderPath);
>-    logComment("scExeDesc               : " + scExeDesc);
>+    logComment("scMailDesc              : " + scMailDesc);
>+    logComment("scMailParam             : " + scMailParam);
>     logComment("fProgram                : " + fProgram);

ditto on these logComments

>Index: nim.jst
>===================================================================
>+function IsRestrictedAccess()

Maybe this should be added to common/windows.t and shared, it's
already duplicated in browser.jst, mail.jst and xpcom.jst

>+function IsWinnt()

ditto

>+  versionThreshold = "6.20.0.2001100100";

You should comment this magic number. Does it need to get updated
every build, is it fixed? Does this code even work now that we're
using 6.2.0 type versions instead of the extra padded form?

>+  rv = InstallTrigger.compareVersion("Nim", versionThreshold);
>+  keyVersion = InstallTrigger.getVersion("Nim");

The registry key for this is "Instant Messenger", not "Nim".

>+  fFileToCheck = getFolder("Program", "components/aimstat.dll");

   getFolder("Components", "aimstat.dll");

>+  if(fileFound && (rv < 0) && (keyVersion != null))
>+  {
>+    deleteThisFile("file:///",   szStartMenuPrograms + "\\Netscape 6\\Instant Messenger.lnk");
>+  }

The shortcuts are new for IM, right? so Why would there be one from
a build older than 6.2 to delete?

>+function registerProgramFolderKey(fFolderPath)
  ...
>+  valname = "Program Folder Path";
>+  value   = fFolderPath;
>+  err     = gWinReg.setValueString(subkey, valname, value);

Why does AIM need to do this? Didn't browser.jst already set this?

>+  fTemp                = fProgram + "$MainExeFile$";
>+  fileExe              = getFolder("file:///", fTemp);
>+  fileNimIcon          = getFolder("file:///", fProgram + "chrome/icons/default/AimApp.ico");

as before

>+    gWinReg.setRootKey(gWinReg.HKEY_CURRENT_USER);

>+    logComment("Folder StartMenuPrograms: " + szStartMenuPrograms);
>+    logComment("Folder StartMenu        : " + szStartMenu);
>+    logComment("Folder FolderDesktop    : " + szFolderDesktop);
>+    logComment("Folder FolderSendTo     : " + szFolderSendTo);
>+    logComment("Folder FolderQuickLaunch: " + szFolderQuickLaunch);
>+    logComment("fileExe                 : " + fileExe);
>+    logComment("fFolderPath             : " + fFolderPath);
>+    logComment("scNimDesc               : " + scNimDesc);
>+    logComment("scNimParam              : " + scNimParam);
>+    logComment("fProgram                : " + fProgram);

debugging cruft. You could wrap it in an if-block if it's helpful.

Comment 18

[Curt Patrick (gone)]

I assumed I would make a mozilla patch but noone has given me icons.  If I don't
get them I'll assume mozilla isn't interested in this feature and close the bug
with only the ns patch.

I'm taking out the logComments for the description stuff.  I think we should
leave in any logComments that might be useful for tech support, but right now
probably isn't the time to figure which that really applies to so I left all the
others in.  We could open a bug to clean these up according to some criteria
that we agree on, I suppose?

As for moving the new functions into common/windows.t, I have another bug open
to clean this up and get it consistant across the jst files.  The problem is
that it isn't, actually, duplicated.  It is done a bit differently in each of
the other, and is actually broke in at least one instance.  So I didn't want the
issue of getting the js files in sync to clutter up fixing this bug.

I don't know what the deal is with the versionThreshold variable.  I just
brought it over on blind faith from browser.jst.  Sean, can you tell us what the
story is on this?

Comment 19

[Curt Patrick (gone)]

Attachment: 0.3 for ns

Besides my comments above, I should note that Dan's implication that
registerProgramFolderKey() shouldn't be in nim.jst made sense to me--I assumed
that I had copied if over from browser.jst unnecassarily--so I cut it out. 
Then I noticed that it is also in mail.jst.  (Not to mention that it is called
redundantly, which I'm quite sure is not correct.)  Sean, is there a reason
this needs to be included in all the jst files, not just browser.jst?  Might
there be something about the xpi getting launched independantly that might make
this a requirement?

Comment 20

[marlon bishop]

cc'ing Dawn, she might be able to coordinate the mozilla icons 

Comment 21

[Curt Patrick (gone)]

Gregg, Rudman and I have agreed on the following:

    *  continue to install the readme and license where people can find them on
their file system if they so desire
    * put them both in the chrome jar file and link to them from he help about
page as we are doing for the license file now
    * Do NOT put a link to them from the Start menu, because 
          * the Start menu is beginning to become rather cluttered now and
          * these two files are not commonly going to be sought after
installation, i.e. the ARE mostly for installation questions but
          * if someone really does want to find them they'll have two ways to do
so, i.e. the help about dialog and searching the file system.

So I'll go ahead and remove the code which creates the links for license and readme.

Sean, I still need your feedback on Dan's comments.  Thanks.

Comment 22

[Curt Patrick (gone)]

BTW, I reopened bug 115901 to deal with getting the readme into the chrome jar.

Comment 23

[Sean Su]

The versionThreshold was used for detecting the shortcuts (and shortcut folders)
created with the version string of '6' or '6.0'.  The new string that marketing
wanted for the newer version of 6.x were '6.1' or '6.2', so we needed a way to
cleanup the old stuff.

The versionThreshold allows us to determine if we're installing over a version
of the browser that used the old version strings.  This is not necessary for NIM.

As for the registerProgramFolderKey() function, you do not need to have it in
the nim's install.js file.  It should only be in browser.jst.

Comment 24

[Curt Patrick (gone)]

Attachment: 0.4 for ns

Based on Sean's explanation, I removed DeleteObsoleteShortcuts() from nim.jst. 
I think that any other cleanup issues can best be dealt with from but 125419.

Sean, can you r= this before we do to Dan for another sr= attempt?

Comment 25

[Sean Su]

Comment on attachment 71766 [details] [diff] [review]
0.4 for ns

r=ssu

Comment 26

[Daniel Veditz [:dveditz]]

Comment on attachment 71766 [details] [diff] [review]
0.4 for ns

I found the bug you opened for translating the Shortcut text (bug 126534).

sr=dveditz

Comment 27

[Daniel Veditz [:dveditz]]

Comment on attachment 71766 [details] [diff] [review]
0.4 for ns

I found the bug you opened for translating the Shortcut text (bug 126534).

sr=dveditz

Comment 28

[Curt Patrick (gone)]

Checked in the ns fix, but I don't want to close this because I can still put in
the mozilla fix whenever I get icons.  None-the-less, I'm removing the nsbeta+
and the milestone because the bug originated from NS marketing and I haven't
heard any indication that Mozilla want to hold for this bug.  (Just noticed that
it is still assigned to Marlon.  Reassigning back to myself so I don't lose
track of it.)

Comment 29

[Curt Patrick (gone)]

Oops, I failed to remove the readme and license links as advertised, so I've got
to do some more work on this one.

Comment 30

[Curt Patrick (gone)]

Attachment: Removes readme and license file links.

Comment 31

[Curt Patrick (gone)]

Since mozilla is not linking to the readme and license files from the help file
like ns is, I'm going to leave the start menu links for readme and license files
in for mozilla, unless someone on the mozilla side tells me differently.

Comment 32

[dprice (gone)]

Comment on attachment 72851 [details] [diff] [review]
Removes readme and license file links.

r=dprice

Comment 33

[Daniel Veditz [:dveditz]]

Comment on attachment 72851 [details] [diff] [review]
Removes readme and license file links.

sr=dveditz

Comment 34

[Curt Patrick (gone)]

Checked in patch 72851

Comment 35

[Grace Bush]

build 2002031903

Comment 36

[Brant Gurganus]

[RFE] is deprecated in favor of severity: enhancement.  They have the same meaning.