Closed jeffbargmann closed 4 years ago
@JBLatenight, thanks for your PR! By analyzing the annotation information on this pull request, we identified @imlucas, @Vj3k0 and @kangas to be potential reviewers
By the way looks like your CI is jammed up on rake call?
Thanks for providing this lib!
Ugh. Squirrel has line "if (zp.IconUrl != null && !File.Exists(targetIco))" in UpdateManager.InstallHelpers.cs, wherein it decides to bail entirely on the Uninstall entry having an icon if the icon already exists. https://github.com/Squirrel/Squirrel.Windows/blob/913b3e730070f2d0461902cd65a6f0cba1f2fcb5/src/Squirrel/UpdateManager.InstallHelpers.cs#L53 Would PR in to adjust that behavior, but that routine's dependency on a remote url for uninstall icon warrants more work than just this fix o.O (Flaky connection on install == no uninstall icon??)
Anywho -- updated to use app's name as icon name (or _app if conflict)
@JBLatenight thanks for submitting this! I have seen those shortcut icons go bad and I had no idea why it was happening. Great catch.
Breaking change is fine, not a concern.
Please "npm i" and run "npm run check". I see a number of linter errors currently on this branch:
$ npm run check
> electron-squirrel-startup@1.0.0 check /Users/kangas/workspace/electron-squirrel-startup
> mongodb-js-precommit
index.js
16:2 error Split 'var' declarations into multiple statements one-var
17:0 warning Line 17 exceeds the maximum line length of 100 max-len
21:37 error Expected '===' and instead saw '==' eqeqeq
27:4 warning Unexpected sync method: 'writeFileSync' no-sync
27:33 warning Unexpected sync method: 'readFileSync' no-sync
37:4 error Split 'var' declarations into multiple statements one-var
50:6 error Expected an assignment or function call and instead saw an expression no-unused-expressions
55:6 error Expected an assignment or function call and instead saw an expression no-unused-expressions
60:6 error Expected an assignment or function call and instead saw an expression no-unused-expressions
Update: please rebase on latest master, then "npm i" to get our latest & greatest linter rules.
› Running eslint on 1 files…
✖ 12 eslint errors detected
───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
✖ 1 of 3 check(s) failed:
Please fix the 12 error(s) below.
/Users/kangas/workspace/electron-squirrel-startup/index.js
16:3 error Split 'var' declarations into multiple statements one-var
17:20 error Strings must use singlequote quotes
21:5 error Expected space(s) after "if" keyword-spacing
21:37 error Expected '===' and instead saw '==' eqeqeq
23:1 error Trailing spaces not allowed no-trailing-spaces
25:5 warning Unexpected sync method: 'writeFileSync' no-sync
25:34 warning Unexpected sync method: 'readFileSync' no-sync
27:5 error Expected space(s) after "catch" keyword-spacing
35:5 error Split 'var' declarations into multiple statements one-var
41:1 error Trailing spaces not allowed no-trailing-spaces
43:7 error Expected space(s) after "if" keyword-spacing
46:7 error Expected an assignment or function call and instead saw an expression no-unused-expressions
51:7 error Expected an assignment or function call and instead saw an expression no-unused-expressions
56:7 error Expected an assignment or function call and instead saw an expression no-unused-expressions
✖ 14 problems (12 errors, 2 warnings)
@JBLatenight
By the way looks like your CI is jammed up on rake call?
Thanks for pointing this out! Travis setup sorted in #11
Hey @kangas sorry I've been off on other work.
Just checked this out and it seems the project has conflicting lint rules; Each time I fix one-var I get a no-mixed-requires, and vice versa. Thoughts?
https://github.com/eslint/eslint/issues/3091 https://github.com/eslint/eslint/issues/6175
Cleaning up old PR's. Please reopen if this should be reconsidered.
As-is, this module creates shortcuts that point the the EXE in the version folder. This means that all shortcuts have invalid icons after each upgrade, and all shortcuts must be updated. Unfortunately, Windows' shell caches some of this information, does fancy stuff for shortcut-to-exe matching, etc. Further, the user may have copied a shortcut, in which case it will just go bad.
A better solution is to use Squirrel's --icon flag and copy an icon to a known place. Squirrel already uses app.ico in the Update.exe directory for uninstall icon, so seems like a good place. (Though-- that icon will fail to be placed if it doesn't download at time of install, oddly.)
NOTE: This is a breaking change so not really eligible for merge :( What do you recommend / Any review suggestions?
This change is![Reviewable](https://reviewable.io/review_button.svg)