fix: app launcher performance on mac #1166
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dodgez
commented
Mar 16, 2023
Comment on lines
+45
to
+48
| let extractIcon = | ||
| process.platform === "win32" | ||
| ? (await npm("get-app-icon")).extractIcon | ||
| : () => Promise.resolve(undefined) |
There was a problem hiding this comment.
I define this up here instead of the loop below for performance and I return a function that returns a promise when on other platforms because we .catch down below.
|
I think it could be a good compromise for now. I found another way to extract windows icons, and it's a little better at extracting icons from links. At least in my tests. I took the liberty of opening a discussion #1168 to document this effort to make it work cross-platform. |
johnlindquist
approved these changes
Mar 22, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previous discussion: #1161
I decided to "revert" some of the changes in that PR and just add a check for win32 so we can keep the performance on a mac and have support for win32.
I considered forking get-app-icon, but realistically it doesn't really support arrays of files to get icons for and would be a large amount of effort when this code can fix it and this is a breaking problem on my work computer.
Curious to get your thoughts @johnlindquist @fcrespo82