Spinoff of bug 305039.

One reason to leave it as ANSI is bug 470499. I may just morph this bug into better handling of shortcuts in the uninstall.log and leave the file as ANSI due to this. Ehsan, any reason you can think of to not leave it as ANSI?

(In reply to comment #1) > Ehsan, any reason you can think of to not leave it as ANSI? Yeah, file paths containing Unicode characters, and start menu folders containing Unicode characters. We could go with UTF-8, but that may also cause problems with non-Unicode installers when downgrading if they don't understand UTF-8 (which I don't know they do or not). However, if we decide not to allow path names with Unicode characters, we can leave uninstall.log as ANSI. One question: would it be sane to try to store the 8.3 version of the Unicode paths in uninstall.log? I'm not sure if the OS guarantees the same conversion between 8.3 and Unicode path names every time in both directions though.

One other question: how did the non-Unicode installer handle Unicode path names? This is quite serious - we don't want to prevent people who want to downgrade 3.1 with 3.0. So, if there's no way of reliably dealing with Unicode path names, I'd suggest going with the ANSI uninstall.log file. But at the very least we have to alert the users properly if they choose Unicode path names (in case we already don't) so that they wouldn't end up with an uninstallable installation.

8.3 paths aren't always available since 8.3 name generation can be disabled. The Unicode paths are converted to their ANSI equivalent but this would fail when the path contains multiple charsets... it is an edgecase but one we should fix if possible. I'm leaning toward using relative paths from well known NSIS dirs like so. User's DESKTOP with a key of something like @UD@ All User's DESKTOP with a key of something like @AD@ User's STARTMENU with a key of something like @UM@ All User's STARTMENU with a key of something like @AM@ QUICKLAUNCH with a key of something like @QL@ Using a short key that is the same length will make coding this much easier. This would need a conversion routine during App Update and when installing on top of an existing install.

After thinking about this while getting the Thunderbird, Seamonkey, and Sunbird installers updated I'm leaning toward recording the shortcut info in a separate file. This file would always contain the names of the shortcuts for the Quicklaunch / Desktop and the Startmenu directory path from the Start Menu -> Programs directory along with the names of the shortcuts in that directory. This way the uninctaller can check both the User and All User locations for both the Start menu and the Desktop as well as the User Quick Launch locations even when the user doesn't add shortcuts. It would also verify that the shortcuts are for the installation that is being uninstalled. The file would also be written in UTF-16LE. I should have a patch finished to do this on Monday and then finish the review of the plugin code. Sound good?

Yeah, this way we can have the best of both worlds (for both non-Unicode and Unicode installers). I'll try to make the changes to the main patch which you requested in time for you to review.

The following will be the structure for the ini file (shortcuts.ini) [StartMenu] PathFromProgramsDir= Shortcut1= Shortcut2= Shortcut3= etc. [Desktop] Shortcut1= Shortcut2= Shortcut3= etc. [QuickLaunch] Shortcut1= Shortcut2= Shortcut3= etc.

I want to add cleanup of existing shortcuts specified in the log for the shortcuts... other than that this is good to go.

Comment on attachment 354391 [details] [diff] [review] patch in progress Frank, if you get a chance I'd appreciate your comments on this approach. Thanks

Comment on attachment 354391 [details] [diff] [review] patch in progress Ehsan, if you get a chance I'd appreciate your comments on this approach. Thanks

Comment on attachment 354391 [details] [diff] [review] patch in progress Looks pretty good to me. This is much better than the uninstall.log approach, and in fact I wonder why such an approach was not being used from the beginning. :-) The only thing which I think can be improved is using more meaningful names than Shortcut1, etc. This is a very small point, which may not matter that much given that this file is not supposed to be read and interpreted by humans anyway...

(In reply to comment #11) > (From update of attachment 354391 [details] [diff] [review]) > Looks pretty good to me. This is much better than the uninstall.log approach, > and in fact I wonder why such an approach was not being used from the > beginning. :-) The main goal for the implementation of the NSIS installer for Firefox 2.0 was to duplicate the pre-existing behavior of the xpinstall based installer as possible. After that was done then bugs that still existed were fixed where possible and other improvements were made. Since this never came up as something that needed to be fixed the shortcuts continued to be logged and removed using the uninstall.log. > The only thing which I think can be improved is using more meaningful names > than Shortcut1, etc. This is a very small point, which may not matter that > much given that this file is not supposed to be read and interpreted by humans > anyway... The difficulty with making the names more friendly is that other apps could and do have different shortcuts and as you noted this isn't meant to be human readable. Thanks for looking at this.

(In reply to comment #12) > The difficulty with making the names more friendly is that other apps could and > do have different shortcuts and as you noted this isn't meant to be human > readable. Ah, right. In that case the current naming convention is perfectly fine. > Thanks for looking at this. Thanks for working on this!

Comment on attachment 354391 [details] [diff] [review] patch in progress What about the HideShortcuts and ShowShortcuts macros, will those do the right thing? I did not test this now, just wondering by code inspection.

The hide and show shortcuts macros only work on the desktop and quicklaunch shortcuts so yes they will do the right thing.

Comment on attachment 354391 [details] [diff] [review] patch in progress Yes, looks like a approach which can be used :). One thing: + ; Delete the existing shortcuts.ini file if it exists + ${DeleteFile} "$INSTDIR\uninstall\shortcuts_log.ini" you mean shortcuts_log.ini in the comment?

Comment on attachment 355663 [details] [diff] [review] patch - Seamonkey rev1 The following comment ; Prior to Firefox 3.1 the Start Menu directory was written to the registry and ; this value can be used to set the Start Menu directory. has been replaced with ; Prior to Unicode installer the Start Menu directory was written to the ; registry and this value can be used to set the Start Menu directory.

Comment on attachment 355671 [details] [diff] [review] patch - Sunbird rev1 =ctalbert Looks good to me. Thanks!

a191=beltzner a priori!

Hey Frank, I am going to try to land this over the weekend along with bug 305039. Can I get you to review the Seamonkey patch today? Thanks

Will review today.

Comment on attachment 355658 [details] [diff] [review] patch rev1 >+!macro FindSMProgramsDir >+ >+ !ifndef FindSMProgramsDir [...] >+ ; This callback MUST use labels vs. relative line numbers. >+ Function FindSMProgramsDirRelPath >+ Push 0 >+ ${TrimNewLines} "$R9" $R9 >+ StrCpy $R4 "$R9" 5 >+ >+ StrCmp "$R4" "File:" +1 end_FindSMProgramsDirRelPath >+ StrCpy $R9 "$R9" "" 6 >+ StrCpy $R4 "$R9" 1 >+ >+ StrCmp "$R4" "\" end_FindSMProgramsDirRelPath +1 >+ >+ SetShellVarContext all >+ ${GetLongPath} "$SMPROGRAMS" $R4 >+ StrLen $R2 "$R4" >+ StrCpy $R1 "$R9" $R2 >+ StrCmp "$R1" "$R4" +1 end_FindSMProgramsDirRelPath >+ IfFileExists "$R9" +1 end_FindSMProgramsDirRelPath >+ ShellLink::GetShortCutTarget "$R9" >+ Pop $R0 >+ StrCmp "$INSTDIR\${FileMainEXE}" "$R0" +1 end_FindSMProgramsDirRelPath >+ ${GetParent} "$R9" $R3 >+ IntOp $R2 $R2 + 1 >+ StrCpy $R3 "$R3" "" $R2 Is this code here correct (only judging by code inspection, did not test this)? As far as I can tell, $R3 contains something like C:\Dokumente und Einstellungen\All Users\Startmenü\Programme\ after this line and then this one is passed back as result (via exch $R3). But we actually want "SeaMonkey" or something like that passed back as result? >+ >+ Pop $R4 ; Remove the previously pushed 0 from the stack and >+ push "StopLineFind" ; push StopLineFind to stop finding more lines. >+ >+ end_FindSMProgramsDirRelPath: >+ ClearErrors >+ >+ FunctionEnd >+ >+ !verbose pop >+ !endif >+!macroend >+

Are you sure? I get the relative path to the directory from the Start Menu Programs directory. Can you verify this for the install case? I did find a bug when calling this from PostUpdate where the shell link plugin isn't loaded.

Comment on attachment 355663 [details] [diff] [review] patch - Seamonkey rev1 Looks good. Will test the other patch, but first I need to see when that code path gets used.

Ok, nevermind, I confused the maxlen and start_offset parameter. This macro works finre.

(In reply to comment #28) >... > I did find a bug when calling this from PostUpdate where the shell link plugin > isn't loaded. This has to do with the plugin trying to load the shortcut's data with STGM_READWRITE when the user doesn't have write access. :( Shouldn't be that important for the use cases we need but I'll look into what it will take to fix this.

Ehsan found a bug in the custom page ini creation code which this patch addresses. The corresponding patch is attachment #356393 [details] [diff] [review] in bug 305039.

Ehsan found a bug in the custom page ini creation code which this patch addresses. The corresponding patch is attachment #356393 [details] [diff] [review] in bug 305039.

Ehsan found a bug in the custom page ini creation code which this patch addresses. The corresponding patch is attachment #356393 [details] [diff] [review] in bug 305039.

Comment on attachment 356435 [details] [diff] [review] patch -Sunbird additional Clint, I am also going to save the calendar updater.ini as UTF-8 and as follows: -Info=Sunbird is installing your updates and will start in a few moments... +Info=Sunbird is installing your updates and will start in a few moments…

(In reply to comment #35) > (From update of attachment 356435 [details] [diff] [review]) > Clint, I am also going to save the calendar updater.ini as UTF-8 and as > follows: > -Info=Sunbird is installing your updates and will start in a few moments... > +Info=Sunbird is installing your updates and will start in a few moments… These changes also look good. r=ctalbert

Comment on attachment 355658 [details] [diff] [review] patch rev1 Pushed to mozilla-central http://hg.mozilla.org/mozilla-central/rev/db430def4ce6 I'm going to wait for one cycle of green before marking fixed and landing on mozilla-1.9.1 along with the rest of the patches on comm-central

Pushed to mozilla-1.9.1 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b88b42c854ec Pushed to comm-central Calendar http://hg.mozilla.org/comm-central/rev/30975188e0f4 Seamonkey http://hg.mozilla.org/comm-central/rev/2eb04c5e52fb Thunderbird http://hg.mozilla.org/comm-central/rev/e4fc968a9ee1 Still going to leave this open until I test an hourly installer.

I tested with an hourly trunk Shredder build including going through the steps to test the plugins and everything so far looks good so resolving -> fixed and fixed1.9.1.