411473 - Shortcut creation on Windows

Status: RESOLVED FIXED

(Mozilla Labs :: Prism, defect)

Description

[Matthew Gertner]

The current VBS code should be replace with something like http://mxr.mozilla.org/seamonkey/source/xpinstall/src/nsWinShortcut.cpp

Comment 1

[Matthew Gertner]

Attachment: Initial implementation of nsIDesktopEnvironment with shortcut generation for Windows

The implication, of course, is that I'll have to rewrite the Mac and Linux shortcut generation code in C++. Kind of silly but I don't see a good alternative if we're going to add new functionality in C++/Objective-C later (e.g. the dock badges).

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 296158 [details] [diff] [review]
Initial implementation of nsIDesktopEnvironment with shortcut generation for Windows

>Index: common/modules/WebAppInstall.jsm
>===================================================================
 
>   _createShortcutWindows : function(target, name, id, icon, location) {
>-    var dirSvc = Cc["@mozilla.org/file/directory_service;1"].getService(Ci.nsIProperties);
>-
>     var locations = location.split(",");
>+    var locationBitmask = 0;
> 
>-    var programs = dirSvc.get("Progs", Ci.nsIFile);
>-    programs.append("Web Apps");
>-    if (!programs.exists())
>-      programs.create(Ci.nsIFile.DIRECTORY_TYPE, 0755);
>+    if (locations.indexOf("desktop") > -1)
>+      locationBitmask |= Ci.nsIDesktopEnvironment.LOCATION_DESKTOP;
>+    if (locations.indexOf("programs") > -1)
>+      locationBitmask |= Ci.nsIDesktopEnvironment.LOCATION_APPLICATIONS;
>+    if (locations.indexOf("quicklaunch") > -1)
>+      locationBitmask |= Ci.nsIDesktopEnvironment.LOCATION_LAUNCHBAR;

Is this really worth it? The new constants I mean. Seems like we are replicating nsIDirectoryService.

>+  /**
>+    * Returns a path for the specified special location
>+    */
>+  AUTF8String getSpecialLocationPath(in PRUint8 location);

I'd like this to be AString for unicode file paths

>+
>+  /**
>+   * Create a shortcut for the application.
>+  **/
>+  void createApplicationShortcut(
>+    in AUTF8String name,
>+    in AUTF8String targetPath,
>+    in PRUint8 location,
>+    in AUTF8String linkPath,
>+    in AUTF8String workingPath,
>+    in AUTF8String arguments,
>+    in AUTF8String description,
>+    in nsIFile icon
>+  );
>+};

* createShortcut is likely long enough.
* let's use AString here and use the wide "W" API for IShellLink in the implementation

>Index: components/src/windows/Makefile.in

> 
> include $(DEPTH)/config/autoconf.mk
> 
>-MODULE            = $(MOZ_APP_NAME)
>-LIBRARY_NAME      = $(MOZ_APP_NAME)
>-IS_COMPONENT      = 1
>-USE_STATIC_LIBS   = 1
>-EMBED_MANIFEST_AT = 2

why remove USE_STATIC_LIBS? I had to add that to get the DLL to register on Windows. We should  be able to remove EMBED_MANIFEST_AT.

Comment 3

[Matthew Gertner]

Attachment: Change to UTF-16 and other tweaks

(In reply to comment #2)
> (From update of attachment 296158 [details] [diff] [review])
> >Index: common/modules/WebAppInstall.jsm
> >===================================================================
> 
> >   _createShortcutWindows : function(target, name, id, icon, location) {
> >-    var dirSvc = Cc["@mozilla.org/file/directory_service;1"].getService(Ci.nsIProperties);
> >-
> >     var locations = location.split(",");
> >+    var locationBitmask = 0;
> > 
> >-    var programs = dirSvc.get("Progs", Ci.nsIFile);
> >-    programs.append("Web Apps");
> >-    if (!programs.exists())
> >-      programs.create(Ci.nsIFile.DIRECTORY_TYPE, 0755);
> >+    if (locations.indexOf("desktop") > -1)
> >+      locationBitmask |= Ci.nsIDesktopEnvironment.LOCATION_DESKTOP;
> >+    if (locations.indexOf("programs") > -1)
> >+      locationBitmask |= Ci.nsIDesktopEnvironment.LOCATION_APPLICATIONS;
> >+    if (locations.indexOf("quicklaunch") > -1)
> >+      locationBitmask |= Ci.nsIDesktopEnvironment.LOCATION_LAUNCHBAR;
> 
> Is this really worth it? The new constants I mean. Seems like we are
> replicating nsIDirectoryService.

I don't think all of the necessary items are in nsIDirectoryService (though I couldn't find definitive documentation), and the bitmask is useful. Having to schlep around and parse a list of strings would be a drag, I think.

> >+  /**
> >+    * Returns a path for the specified special location
> >+    */
> >+  AUTF8String getSpecialLocationPath(in PRUint8 location);
> 
> I'd like this to be AString for unicode file paths

Done.

> >+
> >+  /**
> >+   * Create a shortcut for the application.
> >+  **/
> >+  void createApplicationShortcut(
> >+    in AUTF8String name,
> >+    in AUTF8String targetPath,
> >+    in PRUint8 location,
> >+    in AUTF8String linkPath,
> >+    in AUTF8String workingPath,
> >+    in AUTF8String arguments,
> >+    in AUTF8String description,
> >+    in nsIFile icon
> >+  );
> >+};
> 
> * createShortcut is likely long enough.
> * let's use AString here and use the wide "W" API for IShellLink in the
> implementation

Done.

> >Index: components/src/windows/Makefile.in
> 
> > 
> > include $(DEPTH)/config/autoconf.mk
> > 
> >-MODULE            = $(MOZ_APP_NAME)
> >-LIBRARY_NAME      = $(MOZ_APP_NAME)
> >-IS_COMPONENT      = 1
> >-USE_STATIC_LIBS   = 1
> >-EMBED_MANIFEST_AT = 2
> 
> why remove USE_STATIC_LIBS? I had to add that to get the DLL to register on
> Windows. We should  be able to remove EMBED_MANIFEST_AT.

Didn't mean to change this. Must have had an older version of the file in my tree for some reason. I put EMBED_MANIFEST_AT back in. I'll leave it to you to remove it if you decide it isn't needed.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 296182 [details] [diff] [review]
Change to UTF-16 and other tweaks

>+  /**
>+    * Returns a path for the specified special location
>+    */
>+  AString getSpecialLocationPath(in PRUint8 location);

Ok, this might have some uses. I am wondering if we should make a directory service provider and "add" the new locations that way. The directory provider seems a better fit for Mozilla.

If we were to keep this, returning an nsIFile seems more appropriate.

>+
>+  /**
>+   * Create a shortcut for the application.
>+   *
>+   * @param name Name of the shortcut
>+   * @param targetPath Path to the application we are linking to
>+   * @param location Set to LOCATION_PATH to use the linkPath
>+   *    parameter, otherwise to one or more special locations.
>+   *    For some locations the linkPath parameters may provide
>+   *    additional path information (e.g. Program Files on Windows).
>+   * @param linkPath Directory where the shortcut should be created
>+   * @param workingPath Directory the application should be run in
>+   *    when the shortcut is executed
>+   * @param arguments Command-line parameters to pass to the app
>+   * @param description Human-readable description
>+   * @param icon Icon to use for the shortcut
>+  **/
>+  void createShortcut(
>+    in AString name,
>+    in AString targetPath,
>+    in PRUint8 location,
>+    in AString linkPath,
>+    in AString workingPath,
>+    in AString arguments,
>+    in AString description,
>+    in nsIFile icon
>+  );
>+};

I noticed that this function takes a bit field for a location, and so, is limited to the locations listed above. I think the location param should be a nsIFile instead so this method is more useful.

For the sake of progress, I'll r=mfinkle this for now, but we need to revisit

Comment 5

[Matthew Gertner]

Attachment: Okay, okay, I decided not to reinvent nsDirectoryService

Comment 6

[Matthew Gertner]

Attachment: Remove addShortcutToLocation

Okay, I'm going to deal with the Mac dock differently so I guess we don't need addShortcutToLocation. I also fixed a bug with the favicon downloader (it didn't work on sites that redirected to somewhere else).

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 296989 [details] [diff] [review]
Remove addShortcutToLocation


>Index: common/modules/WebAppInstall.jsm
>+    var shortcut = null;
>+    var directory = null;
>+    for (var i=0; i<locations.length; i++)
>+    {
>+      if (locations[i] == "desktop")
>+        directory = dirSvc.get("Desk", Ci.nsIFile);
>+      else if (locations[i] == "programs")
>+      {
>+        directory = dirSvc.get("Progs", Ci.nsIFile);
>+        directory.append("Web Apps");
>+      }
>+      else if (locations[i] == "quicklaunch")
>+        directory = dirSvc.get("QuickLaunch", Ci.nsIFile);
>+      else
>+        continue;

please add {} on all branches since we have them on one and it helps reading the code since the "if" isn't a simple one

>Index: components/public/nsIDesktopEnvironment.idl

>+  /**
>+   * Create a shortcut for the application.
>+   *
>+   * @param name Name of the shortcut.
>+   * @param targetPath Path to the application we are linking to
>+   * @param location Directory where the shortcut should be created.
>+   * @param workingPath Directory the application should be run in
>+   *    when the shortcut is executed.
>+   * @param arguments Command-line parameters to pass to the app.
>+   * @param description Human-readable description.
>+   * @param icon Icon to use for the shortcut.
>+  **/
>+  nsIFile createShortcut(
>+    in AString name,
>+    in AString targetPath,
>+    in nsIFile location,
>+    in AString workingPath,
>+    in AString arguments,
>+    in AString description,
>+    in nsIFile icon
>+  );
>+};

I'm thinking targetPath should be a nsIFile too

>Index: components/src/windows/nsDesktopEnvironmentWin.cpp

>+    return NS_OK;
>+  }
>+  else
>+    return NS_ERROR_NOT_AVAILABLE;

wrap in {} please

>+  // Query IShellLink for the IPersistFile interface for saving the
>+  // shortcut in persistent storage.
>+  hres = psl->QueryInterface(IID_IPersistFile, (LPVOID FAR *)&ppf);
>+  if(SUCCEEDED(hres))
>+  {
>+      // Save the link by calling IPersistFile::Save.
>+      hres = ppf->Save(lpszFullPath, TRUE);
>+      ppf->Release();
>+  }
>+  psl->Release();

2 spaces in the "if" block

r=mfinkle with these nits

Comment 8

[Matthew Gertner]

Attachment: Changes that slipped through the cracks

I pulled a new tree with SSH in order to commit and somehow the last changes I made got lost. Here they are. Just give the thumbs up and I'll commit them.