Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix Icon Support for Shortcut Creation #2242

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Fix Icon Support for Shortcut Creation #2242

wants to merge 8 commits into from
+82 −11

Conversation

og-mrk
Copy link
Contributor

@og-mrk og-mrk commented Jul 1, 2024 •
edited
Loading

Issues this PR tries to resolve

Resolves: #2241

Screenshot(s)

image

@og-mrk og-mrk mentioned this pull request Jul 1, 2024
Open
@og-mrk og-mrk force-pushed the fix-shortcut-icon-support branch from 9669126 to 970f4cf Compare July 1, 2024 21:47
Marterich
Marterich reviewed Jul 2, 2024
View reviewed changes
Copy link
Contributor

@Marterich Marterich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm correct, the icon is used multiple times during the execution of winutil. There could be potential for optimization to make sure that if the icon already exists on disk, nothing needs to be downloaded and converted again

functions/public/Invoke-WPFShortcut.ps1 Outdated Show resolved Hide resolved
@og-mrk og-mrk requested a review from Marterich July 3, 2024 03:52
Marterich
Marterich reviewed Jul 3, 2024
View reviewed changes
Copy link
Contributor

@Marterich Marterich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a few notes in the code comments about what I think could still be optimized:)

functions/private/ConvertTo-Icon.ps1 Outdated Show resolved Hide resolved
functions/public/Invoke-WPFShortcut.ps1 Outdated Show resolved Hide resolved
functions/private/ConvertTo-Icon.ps1 Outdated Show resolved Hide resolved
@MyDrift-user
Copy link
Contributor

If we are already trying to fix the multiple requests to the same image, I would suggest modifying:

@og-mrk
Copy link
Contributor Author

og-mrk commented Jul 3, 2024

If we are already trying to fix the multiple requests to the same image, I would suggest modifying:

Thanks for pointing these issues out @MyDrift-user
Thankfully, and after some luck, I'd solved these issues some time during this day (when initially working on this PR), but I haven't pushed & made a PR for it.

Will try and compose a new PR that will provide a base system for managing resources and, in particular, images.

og-mrk and others added 2 commits July 3, 2024 21:04
@og-mrk @Marterich
Change behavior of one case from throwing an exception to doing an ea…
3c696b1 
…rly return

Co-authored-by: Martin Wiethan <[email protected]>
@og-mrk
Update Examples for 'ConvertTo-Icon' Functions & Add a new Example, a…
e8ca90e 
…s well as some other changes

Besides the updated documentation for 'ConvertTo-Icon' Function, the icon file path has changed from '$env:TEMP\cttlogo.ico' into '$env:LOCALAPPDATA\winutil\cttlogo.ico', and add edge-case of Folder not being found for the Icon File in 'ConvertTo-Icon' Code.
@og-mrk og-mrk requested a review from Marterich July 3, 2024 19:00
Marterich
Marterich approved these changes Jul 3, 2024
View reviewed changes
Copy link
Contributor

@Marterich Marterich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me (haven't run it but only glanced at the code and the changes)

MyDrift-user
MyDrift-user approved these changes Jul 3, 2024
View reviewed changes
Copy link
Contributor

@MyDrift-user MyDrift-user left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good & works well on my end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Marterich Marterich Marterich approved these changes

@MyDrift-user MyDrift-user MyDrift-user approved these changes

@ChrisTitusTech ChrisTitusTech Awaiting requested review from ChrisTitusTech ChrisTitusTech is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Icon for WinUtil shortcut
3 participants
@og-mrk @MyDrift-user @Marterich