470182 - Create separate log file for shortcuts

Status: RESOLVED FIXED

(Firefox :: Installer, defect, P2)

Description

[Robert Strong (they/them - no direct email)]

Spinoff of bug 305039.

Comment 1

[Robert Strong (they/them - no direct email)]

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?

Comment 2

[(no longer active)]

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

Comment 3

[(no longer active)]

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.

Comment 4

[Robert Strong (they/them - no direct email)]

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.

Comment 5

[Robert Strong (they/them - no direct email)]

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?

Comment 6

[(no longer active)]

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.

Comment 7

[Robert Strong (they/them - no direct email)]

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.

Comment 8

[Robert Strong (they/them - no direct email)]

Attachment: patch in progress

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

Comment 9

[Robert Strong (they/them - no direct email)]

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 10

[Robert Strong (they/them - no direct email)]

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 11

[(no longer active)]

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

Comment 12

[Robert Strong (they/them - no direct email)]

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

Comment 13

[(no longer active)]

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

[Frank Wein [:mcsmurf]]

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.

Comment 15

[Robert Strong (they/them - no direct email)]

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

Comment 16

[Frank Wein [:mcsmurf]]

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 17

[Robert Strong (they/them - no direct email)]

Attachment: patch in progress rev2

Comment 18

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

Comment 19

[Robert Strong (they/them - no direct email)]

Attachment: patch - Seamonkey rev1

Comment 20

[Robert Strong (they/them - no direct email)]

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 21

[Robert Strong (they/them - no direct email)]

Attachment: patch - Thunderbird rev1

Comment 22

[Robert Strong (they/them - no direct email)]

Attachment: patch - Sunbird rev1

Comment 23

[cmtalbert]

Comment on attachment 355671 [details] [diff] [review]
patch - Sunbird rev1

=ctalbert Looks good to me.  Thanks!

Comment 24

[Mike Beltzner [:beltzner, not reading bugmail]]

a191=beltzner a priori!

Comment 25

[Robert Strong (they/them - no direct email)]

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

Comment 26

[Frank Wein [:mcsmurf]]

Will review today.

Comment 27

[Frank Wein [:mcsmurf]]

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

Comment 28

[Robert Strong (they/them - no direct email)]

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 29

[Frank Wein [:mcsmurf]]

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.

Comment 30

[Frank Wein [:mcsmurf]]

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

Comment 31

[Robert Strong (they/them - no direct email)]

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

Comment 32

[Robert Strong (they/them - no direct email)]

Attachment: patch -Sunbird additional

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 33

[Robert Strong (they/them - no direct email)]

Attachment: patch -Thunderbird additional

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 34

[Robert Strong (they/them - no direct email)]

Attachment: patch -SeaMonkey additional

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 35

[Robert Strong (they/them - no direct email)]

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…

Comment 36

[cmtalbert]

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

[Robert Strong (they/them - no direct email)]

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

Comment 38

[Robert Strong (they/them - no direct email)]

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.

Comment 39

[Robert Strong (they/them - no direct email)]

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.