electron-userland / electron-wix-msi

:dvd: Create traditional MSI installers for your Electron app
MIT License
319 stars 93 forks source link

Should `UninstallDisplayIcon` optionally use the Application's icon? #171

Closed gregcotten closed 8 months ago

gregcotten commented 1 year ago

Maybe add a param to the creator called useExeIconForUninstaller or something that points to the app's exe instead of msiexec.exe's icon?

https://github.com/electron-userland/electron-wix-msi/blob/2339d98a765417e7236dbf72340ff1b3e0ade87e/src/creator.ts#L784C13-L784C13

gregcotten commented 1 year ago

Here's the suggested change:

diff --git a/node_modules/electron-wix-msi/lib/creator.d.ts b/node_modules/electron-wix-msi/lib/creator.d.ts
index ea0f1ed..2dc98e9 100644
--- a/node_modules/electron-wix-msi/lib/creator.d.ts
+++ b/node_modules/electron-wix-msi/lib/creator.d.ts
@@ -5,6 +5,7 @@ export interface MSICreatorOptions {
     description: string;
     exe: string;
     icon?: string;
+    useExeIconForUninstaller?: boolean;
     extensions?: Array<string>;
     lightSwitches?: Array<string>;
     cultures?: string;
@@ -73,6 +74,7 @@ export declare class MSICreator {
     description: string;
     exe: string;
     icon?: string;
+    useExeIconForUninstaller: boolean;
     extensions: Array<string>;
     lightSwitches: Array<string>;
     cultures?: string;
diff --git a/node_modules/electron-wix-msi/lib/creator.js b/node_modules/electron-wix-msi/lib/creator.js
index 0860c84..3bd4ace 100644
--- a/node_modules/electron-wix-msi/lib/creator.js
+++ b/node_modules/electron-wix-msi/lib/creator.js
@@ -60,6 +60,7 @@ class MSICreator {
         this.description = options.description;
         this.exe = options.exe.replace(/\.exe$/, '');
         this.icon = options.icon;
+        this.useExeIconForUninstaller = options.useExeIconForUninstaller || false;
         this.extensions = options.extensions || [];
         this.lightSwitches = options.lightSwitches || [];
         this.cultures = options.cultures;
@@ -499,13 +500,21 @@ class MSICreator {
             value: 'MsiExec.exe /X {{{ProductCode}}}',
             forceDeleteOnUninstall: 'yes'
         });
+
+        let uninstallIconValue = '';
+        if (this.useExeIconForUninstaller) {
+            uninstallIconValue = '[APPLICATIONROOTDIRECTORY]{{ApplicationBinary}}.exe';
+        } else {
+            uninstallIconValue = this.arch === 'x86' ? '[SystemFolder]msiexec.exe' : '[System64Folder]msiexec.exe';
+        }
+
         registry.push({
             id: 'UninstallDisplayIcon',
             root: 'HKMU',
             name: 'DisplayIcon',
             key: uninstallKey,
             type: 'expandable',
-            value: this.arch === 'x86' ? '[SystemFolder]msiexec.exe' : '[System64Folder]msiexec.exe',
+            value: uninstallIconValue,
             forceDeleteOnUninstall: 'yes'
         });
         if (this.autoUpdate) {

This issue body was partially generated by patch-package.

noahheck commented 1 year ago

Our teams would use this.

@gregcotten I have a production app to test against if you could use some help testing 👍

gregcotten commented 1 year ago

@noahheck Works in my production app and I am using this patch right now :)

noahheck commented 1 year ago

I made it back to this today. This change seems to work well for my project.

What I would question is if there's a reason this shouldn't be the default. If we've already established that we have an icon for the application, is there any reason to not have that icon represent the app in the "Uninstall Application" list?