521141 - Start menu / programs shortcuts pinned to the taskbar don't group correctly

Status: RESOLVED FIXED

(Firefox :: Installer, defect, P2)

Description

[Brian Carpenter [:geeknik]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20091007 Minefield/3.7a1pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20091007 Minefield/3.7a1pre

Behavior of the Windows 7 taskbar if you pin a program to it is that if you click on that program, it will use that icon for managing that program. However, something in the 7 October build of Minefield has broken that behavior and when you click on the pinned icon of Minefield, it adds another Minefield icon to the taskbar.

Reproducible: Always

Comment 1

[A.]

confirmed

Comment 2

[Jim Mathies [:jimm]]

What version did you pin to the taskbar before that build? If it was a previous nightly, unpin the old and repin the new build, we updated our taskbar registration. If that addresses the issue please close this bug out.

Comment 3

[Brian Carpenter [:geeknik]]

Okay, did as Jim said and it looks to be fixed, so I'll close this out. Thanks.

Comment 4

[A.]

@jim
your solution did work ,but unfortunately it look like jumplist is broken now 

thanks anyhow

Comment 5

[Jim Mathies [:jimm]]

(In reply to comment #4)
> @jim
> your solution did work ,but unfortunately it look like jumplist is broken now 
> 
> thanks anyhow

What jump lists? We just added platform support for jump lists in this patch. Are you running an extension that adds support for them maybe?

Comment 6

[robg]

@jim

Pinning the active task-bar icon works fine in Windows 7 but is very counter-intuitive, if you pin minefield to the task-bar from the program folder and/or from the start menu which most users probably will, the second minefield icon still spawns. 

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20091008 Minefield/3.7a1pre Firefox/3.5.3

Comment 7

[Jim Mathies [:jimm]]

reopening due to symptoms in comment 6. Likely due to issues with  ExplicitAppUserModelID app registration and the shortcuts the installer creates. Bug 505925.

Comment 8

[[not reading bugmail]]

(In reply to comment #6)
> @jim
> 
> Pinning the active task-bar icon works fine in Windows 7 but is very
> counter-intuitive, if you pin minefield to the task-bar from the program folder
> and/or from the start menu which most users probably will, the second minefield
> icon still spawns. 
> 
> Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20091008
> Minefield/3.7a1pre Firefox/3.5.3

Pretty sure I saw this too trying to check into bug 521304.

Comment 9

[Jim Mathies [:jimm]]

Moving this to the installer component since that's where these are created.

Comment 11

[Brian Carpenter [:geeknik]]

This was fixed at one point, but now it's broken again. I've noticed that if you right click on the Minefield desktop shortcut and select pin to taskbar, it'll put the icon in the taskbar, but if you click on the taskbar icon, it doesn't group with it. If you remove the taskbar icon while Minefield is running, and then right click on the running Minefield icon and select pin to taskbar, and then close Minefield and re-launch Minefield from the new taskbar icon, it is grouped properly. I thought that maybe it's the fact that every day we have a new nightly build, different build #, etc, was causing this to break, however, I have the Chrome dev channel installed and it updates all the time and it's icon still groups properly, so that isn't it.

Comment 15

[Jim Mathies [:jimm]]

Attachment: add applicationid plugin

Prelim patch. Looks like I'll have to make modifications for unicode support. Will get to that once I get to testing. Also emailed the author for contrib. approval.

Comment 17

[Jim Mathies [:jimm]]

Attachment: patch v.1

Comment 18

[Jim Mathies [:jimm]]

This rev of the dll was built using visual studio 2005 sp1, which matches our build requirements. I'm wondering though if I should go ahead and link the runtime into it, to avoid dependencies. Good for testing though.

Comment 20

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

(In reply to comment #18)
> This rev of the dll was built using visual studio 2005 sp1, which matches our
> build requirements. I'm wondering though if I should go ahead and link the
> runtime into it, to avoid dependencies. Good for testing though.
Jim, I'd prefer to build it with VC 6 if possible... I have a system I can compile it on if you see no problems / are ok with my doing that.

Comment 21

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

Another option to avoid unsatisfied dependencies on earlier version of Windows is to only load / call the plugin on Windows 7 and greater since it won't have any affect on earlier versions, etc.

Comment 22

[Jim Mathies [:jimm]]

(In reply to comment #20)
> (In reply to comment #18)
> > This rev of the dll was built using visual studio 2005 sp1, which matches our
> > build requirements. I'm wondering though if I should go ahead and link the
> > runtime into it, to avoid dependencies. Good for testing though.
> Jim, I'd prefer to build it with VC 6 if possible... I have a system I can
> compile it on if you see no problems / are ok with my doing that.

I think I have a copy of vc 6 still, so let me see if I can get that installed on an image.

Comment 23

[Jim Mathies [:jimm]]

(In reply to comment #21)
> Another option to avoid unsatisfied dependencies on earlier version of Windows
> is to only load / call the plugin on Windows 7 and greater since it won't have
> any affect on earlier versions, etc.

Hrm, our current version code for nsis in mozilla-build doesn't support identifying win7. I guess I'll add some code just to be safe to the actual add-in.

Comment 24

[Jim Mathies [:jimm]]

(In reply to comment #20)
> (In reply to comment #18)
> > This rev of the dll was built using visual studio 2005 sp1, which matches our
> > build requirements. I'm wondering though if I should go ahead and link the
> > runtime into it, to avoid dependencies. Good for testing though.
> Jim, I'd prefer to build it with VC 6 if possible... I have a system I can
> compile it on if you see no problems / are ok with my doing that.

Hmm, this presents a problem. None of the latest SDKs support vc6 since it's depreciated. I think we're going to have to use 2005.

Will NSIS fail gracefully if the dll can't be loaded?

Comment 25

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

iirc it doesn't

Comment 26

[Jim Mathies [:jimm]]

(In reply to comment #25)
> iirc it doesn't

Turns out this is not an issue, with the runtime compiled in, there are no dependencies.  Confirmed this on a win2K image using depends. The com calls fail gracefully.

Comment 27

[Jim Mathies [:jimm]]

Attachment: patch v.2

Ok, here's a new patch with the following updates: 

- added a Clear method to purge resources related to the taskbar on uninstall
- added a dll resource for ApplicationID
- built a new rev of the dll without runtime dependencies

Tested on 2K for installer/uninstaller load issues just to be sure.

Comment 28

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

Comment on attachment 442453 [details] [diff] [review]
patch v.2

>diff --git a/browser/installer/windows/nsis/installer.nsi b/browser/installer/windows/nsis/installer.nsi
>--- a/browser/installer/windows/nsis/installer.nsi
>+++ b/browser/installer/windows/nsis/installer.nsi
>@@ -30,19 +30,20 @@
> # decision by deleting the provisions above and replace them with the notice
> # and other provisions required by the GPL or the LGPL. If you do not delete
> # the provisions above, a recipient may use your version of this file under
> # the terms of any one of the MPL, the GPL or the LGPL.
> #
> # ***** END LICENSE BLOCK *****
> 
> # Required Plugins:
>-# AppAssocReg http://nsis.sourceforge.net/Application_Association_Registration_plug-in
>-# ShellLink   http://nsis.sourceforge.net/ShellLink_plug-in
>-# UAC         http://nsis.sourceforge.net/UAC_plug-in
>+# AppAssocReg   http://nsis.sourceforge.net/Application_Association_Registration_plug-in
>+# ShellLink     http://nsis.sourceforge.net/ShellLink_plug-in
>+# UAC           http://nsis.sourceforge.net/UAC_plug-in
>+# ApplicationID http://nsis.sourceforge.net/ApplicationID_plug-in
nit: place ApplicationID after AppAssocReg so it is sorted

>diff --git a/other-licenses/nsis/Contrib/README b/other-licenses/nsis/Contrib/README
>--- a/other-licenses/nsis/Contrib/README
>+++ b/other-licenses/nsis/Contrib/README
>@@ -49,8 +49,14 @@ UAC NSIS plugin v0.0.10a
> http://nsis.sourceforge.net/UAC_plug-in
> The source files already support Unicode but there is no Unicode distribution
> so the plugin had to be compiled with Unicode support. A diff of the changes to
> the source are available at:
> https://bugzilla.mozilla.org/show_bug.cgi?id=473348
> https://bugzilla.mozilla.org/attachment.cgi?id=357014
> 
> -------------------------------------------------------------------------------
>+
>+ApplicationID v1.0
>+http://nsis.sourceforge.net/ApplicationID_plug-in
>+Win7 taskbar registration plugin. See bug 521141.
>+
Since this might end up somewhere where it is impossible to tell which bug system is referred to please use full urls as is done in the rest of this file

Please remove "All of these plugin dll's have been compiled with VC6." from the top of the README since this will no longer be true and instead make a note regarding what it was compiled with for each plugin.

Please include a note as to why this plugin is here as is done for the other plugins. In this instance (please attach a patch to this bug with just the changes to the plugin and change the attachment link below to point to it.

ApplicationID v1.0
http://nsis.sourceforge.net/ApplicationID_plug-in
Unicode support and Jump List deleteion was added for this plugin. A diff of
the changes to the source are available at:
https://bugzilla.mozilla.org/show_bug.cgi?id=521141
https://bugzilla.mozilla.org/attachment.cgi?id=

>+-------------------------------------------------------------------------------

Comment 29

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

Comment on attachment 442453 [details] [diff] [review]
patch v.2

Looks good. In the reference patch for the plugin changes please include the removal of AppID.vcproj and ApplicationID.sln along with the new files.

Comment 30

[Jim Mathies [:jimm]]

Attachment: patch v.3

Comment 31

[Jim Mathies [:jimm]]

I still need to do a final diff, add a link to the readme, and test some recent changes for bug 526663. So this still needs one last review once I'm done with the whole series of patches.

Comment 32

[Jim Mathies [:jimm]]

Attachment: patch v.4

Added a new api for unpinning shortcuts.

Comment 33

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

Comment on attachment 443361 [details] [diff] [review]
patch v.4

>diff --git a/browser/installer/windows/nsis/uninstaller.nsi b/browser/installer/windows/nsis/uninstaller.nsi
>--- a/browser/installer/windows/nsis/uninstaller.nsi
>+++ b/browser/installer/windows/nsis/uninstaller.nsi
>@@ -220,16 +220,19 @@ Section "Uninstall"
>     RmDir "$APPDATA\Mozilla"
>   ${EndIf}
> 
>   SetShellVarContext current  ; Set SHCTX to HKCU
>   ${un.RegCleanMain} "Software\Mozilla"
>   ${un.RegCleanUninstall}
>   ${un.DeleteShortcuts}
> 
>+  ; Unregister resources associated with Win7 taskbar jump lists.
>+  ApplicationID::UninstallJumpLists "${AppUserModelID}"
There isn't much in the way of documentation for RemoveFromList. Do you know if it matter whether it is called before or after the removal of shortcuts?

Comment 34

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

(In reply to comment #33)
> (From update of attachment 443361 [details] [diff] [review])
> >diff --git a/browser/installer/windows/nsis/uninstaller.nsi b/browser/installer/windows/nsis/uninstaller.nsi
> >--- a/browser/installer/windows/nsis/uninstaller.nsi
> >+++ b/browser/installer/windows/nsis/uninstaller.nsi
> >@@ -220,16 +220,19 @@ Section "Uninstall"
> >     RmDir "$APPDATA\Mozilla"
> >   ${EndIf}
> > 
> >   SetShellVarContext current  ; Set SHCTX to HKCU
> >   ${un.RegCleanMain} "Software\Mozilla"
> >   ${un.RegCleanUninstall}
> >   ${un.DeleteShortcuts}
> > 
> >+  ; Unregister resources associated with Win7 taskbar jump lists.
> >+  ApplicationID::UninstallJumpLists "${AppUserModelID}"
> There isn't much in the way of documentation for RemoveFromList. Do you know if
> it matter whether it is called before or after the removal of shortcuts?
s/RemoveFromList/DeleteList/

Comment 35

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

I was searching a bit too specifically and now have found it.

Can you file a followup on calling that when we clear history or if there is already one comment about it here?
http://msdn.microsoft.com/en-us/library/dd378400%28VS.85%29.aspx
"When the user clears history from within the application."

Is this referring essentially to private browsing?
"When the user disables destination tracking in the application's Settings or Options pages."

Also of note:
http://msdn.microsoft.com/en-us/library/dd378413%28VS.85%29.aspx

Comment 36

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

Comment on attachment 443361 [details] [diff] [review]
patch v.4

I haven't found anything about whether DeleteList should be called before or after shortcut / pinned item deletion but the following makes me think it should be called before.
http://msdn.microsoft.com/en-us/library/dd378400%28VS.85%29.aspx
"A pointer to the AppUserModelID of the process whose taskbar button representation displays the custom Jump List."

Comment 37

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

Comment on attachment 443361 [details] [diff] [review]
patch v.4

minusing until comment #36 is answered

Comment 38

[Jim Mathies [:jimm]]

(In reply to comment #35)
> I was searching a bit too specifically and now have found it.
> 
> Can you file a followup on calling that when we clear history or if there is
> already one comment about it here?
> http://msdn.microsoft.com/en-us/library/dd378400%28VS.85%29.aspx
> "When the user clears history from within the application."
> 
> Is this referring essentially to private browsing?
> "When the user disables destination tracking in the application's Settings or
> Options pages."
> 
> Also of note:
> http://msdn.microsoft.com/en-us/library/dd378413%28VS.85%29.aspx

This is implemented by the jump list jsm under browser/components/wintaskbar.

Comment 39

[Jim Mathies [:jimm]]

(In reply to comment #36)
> (From update of attachment 443361 [details] [diff] [review])
> I haven't found anything about whether DeleteList should be called before or
> after shortcut / pinned item deletion but the following makes me think it
> should be called before.
> http://msdn.microsoft.com/en-us/library/dd378400%28VS.85%29.aspx
> "A pointer to the AppUserModelID of the process whose taskbar button
> representation displays the custom Jump List."

It can be called regardless, the jump list is a resource tied to a specific app id, not the shortcut. You can set / clear jump lists without pinned items.

Comment 40

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

Comment on attachment 443361 [details] [diff] [review]
patch v.4

Thanks for the explanation.

Comment 41

[Jim Mathies [:jimm]]

http://hg.mozilla.org/mozilla-central/rev/2074183c0d6c

dev doc - when installing, the application user model id (bug 505925) must be set on shortcuts. We have routines for this setup through our shared nsis code. (which landed in this patch)

Comment 42

[Eric Shepherd [:sheppy]]

Can someone jot down some notes explaining how to do this? I'd like to get docs updated, but I don't know enough about nsi to read this stuff and be confident that I'm getting it right.