-
Notifications
You must be signed in to change notification settings - Fork 37
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
add windows icon #111
add windows icon #111
Conversation
I don't care about the file size its still probably the smallest application I'd use on windows. But that is on @cnlohr to decide. The ci fails here, probably needs to run the icon building prior to the exe building. Should be fixable in Makefile. |
I always think icons are an improvement BTW 👍 |
I have removed all icon sizes except 128px and 32px. But i dont know if windows requires some sizes inbetween for correct display. But the binary is now "only" 140kB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm, not tested in Windows yet
I don't see the actual .ico file in this CL. |
Please also check it in as an artifact |
I have now manually compressed the icon using Gimp. The ico is now only On my windows11 system the created exe with icon looks great. But gimp warns about using compression. Maybe older windows systems have problems with that. What's the oldest windows we want to support? If windows is not able to load the compressed icon would the application still work? I can check if i have an winXP or Win7 vm where i can test that. EDIT:I tested it on winXP and win7 vms. winXP:the icon is not displayed in the compressed version, but cnping does not work at all.
win7:The icon is displayed in compressed and uncompressed forms. cnping does start, but is crashing after creating the window. I belive its the same bug as in #83 . |
This comment was marked as outdated.
This comment was marked as outdated.
Makefile
Outdated
@@ -33,7 +33,7 @@ cnping-wingdi.exe : cnping.c ping.c httping.c resources.o | |||
cnping.exe : cnping.c ping.c httping.c resolve.c resources.o | |||
$(MINGW32)gcc -g -fno-ident -mwindows -m32 -DCNFGOGL $(CFLAGS) -o $@ $^ -lgdi32 -lws2_32 -s -D_WIN32_WINNT=0x0600 -DWIN32 -liphlpapi -lopengl32 -DMINGW_BUILD $(ADMINFLAGS) | |||
|
|||
resources.o : resources.rc | |||
resources.o : resources.rc cnping.ico |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we are now committing this file, I'd say we shouldn't require it here to avoid confusion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i have removed it by editing the commit
from e4c2dc6
to fix ci; reduce amount of resolutions
Never mind my comment about artifacts, you already removed that part again in the last commit 👍 |
I am good with this at this point. @dreua do you feel it's ready? |
Haven't tested it myself but I trust mrbesen's tests, ready to merge from my side. |
I added the stuff required to give the exe file an icon.
The problem: It increases the filesize from
69.1kB
to208.4kB
- Wtf?I dont know if thats worth for you. Maybe you want to provide both, a icon exe file and an non icon exe file?
It might be possible to reduce the exe filesize by removing some icon resolutions. I don't know which are required.