bpo-38439: Add 256px IDLE icon #17473
Conversation
|
cc @terryjreedy @csabella @serhiy-storchaka The icon looks great! It looks the same than Lib/idlelib/Icons/idle_48.png but with a much better resolution. I cannot see 256x256 pixels icon in https://bugs.python.org/issue1490384 : where does the file come from? I would like to get some more context to check the license. On the issue, I see that Andrew Clover signed the Contributor Agreement. Did Andrew draw them? The commit 4ade2d2 of https://bugs.python.org/issue20406 added the following icons (GIF and PNG formats): But I didn't understand where these files come from. |
I've tried to address this in the commit message. |
|
Oh wait, I missed https://bugs.python.org/issue38439 I confirm that the icon is drew by drew by Andrew Clover: https://bugs.python.org/issue38439#msg357852 For an unknown reason, the bot didn't add a link to https://bugs.python.org/issue38439 in the PR description. |
|
As a side note, I haven't added a gif, because I don't see a point in a 256px gif icon. |
|
I installed Xara Photo & Graphic Designer trial on Windows. I converted the Xara file to SVG at 300 dpi, but the compressed SVG is still 1.4 MiB which is quite big. I converted to 96 dpi compressed SVG: 796 KiB. Still quite big.
I edited the SVG to only keep the 4 IDLE icons: 152 KiB. That's more reasonable, no? If I only keep the IDLE icon "at 256x256 pixels" resolution (the one used to generated the PNG of this PR), it's 136 KiB: I don't have a strong opinion about only adding the PNG, or also add the SVG. |
|
I would also add the svg, possibly with all the variants. At least include t in the sources (not install it). So we don't have to search the Internets when we add a higher resolution icon in next decade. However, no idea where to put it in the source tree. |
|
And here is the scaleable icon I'd like to actually include in the installation (uncompressed). |
Uncompressed: 136 KiB. That sounds reasonable. I like the idea of installing it. But I'm not sure of the location. Do you expect to get it in /usr/share? Currently, Python only install icons in /usr/lib, no? I'm not sure how to modify the Makefile. Maybe start by adding the SVG in Lib/idlelib/Icons/ but don't install it with "make install". It's nice to have the source of PNG icons in the same directory. Linux vendors like Fedora could use this SVG icon if they want. Or maybe propose a following PR to install it, if you are motivated to hack the Makefile ;-) |
|
In Fedora, we currently do: Where The svg would go to |
|
How can move this forward? Is the higher resolution icon desired? Should I add the SVG "source" as well as the minimized scalable SVG? Where? We can carry those downstream, but I'd rather not. |
|
I am on holiday break with my family, but I will try to get to this, at least from the IDLE side, soon. |
|
Enjoy the break and don't worry. It's enough for me to know that you are aware of this. Thanks for the heads up. |
There was a problem hiding this comment.
LGTM. I'm fine with not adding the source SVG into Python, only PNG.
Note: the idle.ico has been modified to add a 8th icon into the file: the 256x256 pixels PNG flavor.
For the one who merges the change: please just ensure that the commit message is complete, see #17473 (comment)
|
Could someone with the SVG also create 44x44 and 150x150 sized versions? (Like the These are the sizes used for the tiles created by the Microsoft Store installer, and referenced from |
|
I cannot do it right now, but the SVG is attached here: https://github.com/python/cpython/files/3927351/idle-256px.svg.tar.gz |
|
and this one has scalable versions of the smaller icons, more likely to be useful for the 44x44 version: http://haypo.alwaysdata.net/tmp/idle.svgz |
|
@terryjreedy Should we still wait for your review? |
|
44x44:
150x150:
|
There was a problem hiding this comment.
As is, the patch seems incomplete and it is not clear what effects it will have. (Revised from first post.) The relevant code in idlelib.pyshell is
# set application icon
icondir = os.path.join(os.path.dirname(__file__), 'Icons')
if system() == 'Windows':
iconfile = os.path.join(icondir, 'idle.ico')
root.wm_iconbitmap(default=iconfile)
elif not macosx.isAquaTk():
ext = '.png' if TkVersion >= 8.6 else '.gif'
iconfiles = [os.path.join(icondir, 'idle_%d%s' % (size, ext))
for size in (16, 32, 48)]
icons = [PhotoImage(master=root, file=iconfile)
for iconfile in iconfiles]
root.wm_iconphoto(True, *icons)
The patch has an expanded multi-icon idle.ico for Windows. Since it will only be used on Vista+, It should have been made from the 4 .pngs. Was it?
An expanded multi-icon idle.icns is needed for macOS Aqua, made from the 4 .pngs. The mac installer must separately grab idle.icns and give it to the OS for whatever use it makes of it.
For *nix, either idle_256.gif must be added, and 256 added to the list in the code above, or the code needs revision to tolerate its absence.
Except on macOS Aqua, I am not sure if the new size will ever by used, as the code above only affects the little icon in upper-left corners of windows. (After I posted this, I notice that Fedora also grabs the icons from IDLE for use elsewhere.)
I don't know what the Content include lines do. idle.icns is installed even though not 'Content include'd. The .gif and .png files are not directly used, at least not by IDLE. But worrying about this is another issue.
It seems to me that since installing tkinter/IDLE is generally optional, icons needs by other than IDLE should be stored somewhere else always installed, and 'imported' as needed by IDLE.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
Thanks @hroncok for the PR, and @terryjreedy for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8. |
Icon author: Andrew Clover, bpo-1490384 (cherry picked from commit 3a69f3c) Co-authored-by: Miro Hrončok <miro@hroncok.cz>
|
GH-19646 is a backport of this pull request to the 3.8 branch. |
Icon author: Andrew Clover, bpo-1490384 (cherry picked from commit 3a69f3c) Co-authored-by: Miro Hrončok <miro@hroncok.cz>
|
GH-19647 is a backport of this pull request to the 3.7 branch. |
|
Please make a 2nd PR, same issue, for the .ico. Even if the 256 icon is not used, the png versions of the others may improve on the .gif versions. |
See pythonGH-17473 for the discussion.
Icon author: Andrew Clover, bpo-1490384 (cherry picked from commit 3a69f3c) Co-authored-by: Miro Hrončok <miro@hroncok.cz>
Icon author: Andrew Clover, bpo-1490384 (cherry picked from commit 3a69f3c) Co-authored-by: Miro Hrončok <miro@hroncok.cz>
|
|
@stratakis is that ours? Hardly related here. |
|
@hroncok indeed it is, but the failure seems unrelated. |
Sadly, there are many known buildbot failures. Search for bugs that I reopened which contain "test" in the title to have an idea :-) This one is likely this bug: https://bugs.python.org/issue39995 Reported one month ago, nobody investigated it yet. |
Tested now, works great on my Windows 10 machine. |
Icon author: Andrew Clover, bpo-1490384
Downloaded from https://www.doxdesk.com/software/py/pyicons.html
I would add the vector version as well, however:
https://bugs.python.org/issue38439