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.

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

Updated generate-icon script & added new icon #9

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

tech189
Copy link

@tech189 tech189 commented Jan 7, 2022

Closes #8

Tested locally, hope it is okay.

Copy link

@p-schneider p-schneider left a comment

Choose a reason for hiding this comment

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

The method to convert png to ico is incorrect, the output (including the committed file in tools/icon.ico) is not a proper .ico file!

Comment on lines 26 to 28
Assert-ProgramExist -ProgramName 'magick' -Reason 'to convert png to ico'

magick convert $ICON_XPM -filter point -resize 128x $ICON_PNG
Remove-Item $ICON_XPM

Assert-ProgramExist -ProgramName 'png2ico' -Reason 'to convert png to ico'
png2ico $ICON_ICO $ICON_PNG
magick convert $ICON_PNG -filter point $ICON_ICO

Choose a reason for hiding this comment

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

This does not convert the png into an ico, it just converts the png into an png with the .ico file extension - but that's just a filename, the image itself ist still a png.
You should use the png2ico command to convert from png to ico like before. - (Or a proper way of doing it using other tools like magick, but I'm not sure if magick is able to handle the ico file format.)

Copy link
Author

Choose a reason for hiding this comment

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

png2ico doesn't support the large size it seems:

...\AppData\Local\Temp\icon.png: Width must be multiple of 8 and <256. Height must be <256.

I could first resize it using magick and send it to png2ico but since I've found a way to create a proper ico file with ImageMagick, it makes more sense to do it all in one line - try out my modified version

My command makes an icon that contains all the sizes required here: https://docs.microsoft.com/en-us/windows/win32/uxguide/vis-icons#size-requirements. But I can change it to just 256x256 if that's all Windows needs nowadays.

Choose a reason for hiding this comment

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

In Readme.md png2ico is mentioned as build dependency. If that's no lognger the case that could also be changed in the Readme.

Copy link
Author

Choose a reason for hiding this comment

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

Done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New App Icon
2 participants