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

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

adding myself as QA contact to track for project

reassigning to Lori Kaplan. Spec posted at: http://client.mcom.com/machv/xpinstall/desktop_integration.html Lori, is this ok?

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.

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

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

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.

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? +

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

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.

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

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.

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 on attachment 70387 [details] [diff] [review] 0.2 for ns (complete) r=ssu (given what we talked about)

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.

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 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.

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?

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?

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

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.

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

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.

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 on attachment 71766 [details] [diff] [review] 0.4 for ns r=ssu

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 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

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.)

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

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 on attachment 72851 [details] [diff] [review] Removes readme and license file links. r=dprice

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

Checked in patch 72851

build 2002031903

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