Pick ICO entries in order

[RunDevelopment]

ICO image files can contain any number of entries (BMP or PNG images). This is a problem for us, because we need to pick one of those entries to display. Luckily, it's pretty obvious which one to pick in most cases. Most ICO files have exactly one entry with the greatest size and greatest number of bits per pixel, ergo, the best quality.

Unfortunately, there isn't always one best entry. There might be multiple entries with the same size, same bpp, and even same number of bytes on disk. So. Which one would you pick?

Since there is no obvious way to pick one entry out of multiple equally good ones, programs do it differently. From my testing, they behave as follows:

1. Some pick the entry that appears first in the list of entries. Those are Chrome and MS Paint.
2. Some pick the entry that appears last in the list of entries. Those are Firefox and Windows Photo Viewer.
3. Windows explorer will pick the one that appears last in the list. Unless it needs wants to display a 16x16 icon, then it will display the one that appears first in the list.

And then there's this crate. Our ICO decoder will pick the best entry that appears first in the list, unless the very last entry of the list is one of the best entries.

So I changed the behavior of our decoder to always pick the one that appears first in the list of entries to be consistent with MS Paint. And Chrome, I guess. Again, there is no real right way to do this, but I would say that what we did before was definitely wrong, even if it arguably is a degenerate case.

For tests, I create a new ICO file with 2 entries of the same size and bpp. The first entry is green and the second is red. This makes it easy to see which one is picked. Here it is in GIMP:

---

resolves #1739

The last 2 icons attached in #1739 that were still loaded incorrectly were icons that have this exact problem. Here are the different entries in GIMP:

As you can see, the icon has 2 layers with the same bpp for each size. I suspect that this is an error by whoever created the icon, because the second entry for each size uses reduced colors (while still being encoded as 32 bpp).

In any case, our ICO decoder now decodes all icons from #1739 correctly, so the issue is fixed.

[197g]

Great investigation.

We can definitely do this in the 1.0 switch. Agreeing with Chrome and imagemagick is very reasonable. I wonder if it were worth it to have an option on BmpDecoder for the selection algorithm? Well I'm mostly thinking of the 16x16 thumbnail here—but then we could also say to use the AnimationDecoder (implying we fix it to just mean image sequence, with no implied order relationship but rather an explicit one for formats that are animations).

LGTM but before merging I'd like to hear you opinion on the thought.

[RunDevelopment]

As I see it, icons with multiple entries being the same size and bpp are a degenerate case. So I don't think it's worth having a selection strategy option just for them. AFAIK, icons only have entries with the same size but different bpp to support older operating systems (see wikipedia).

I'd rather implement the decoder implements AnimationDecoder (or similar for sequences), so users can get all entries. This would enable more use cases and uses could use whatever strategy they want. They could even display all entries like GIMP for example.

[197g]

Sounds good to me. @fintelia A bit contentious so I'd like to check if that seems fine.

@RunDevelopment Would it be possible for you to start the Changelog for 1.0 and note the change in ICO down, I think this is something that people would like to know changed. Just in case it's been in a test suite somewhere.

[fintelia]

This change seems fine to me

[RunDevelopment]

Would it be possible for you to start the Changelog for 1.0 and note the change in ICO down

What does "start" mean exactly? Do you only want me to add a single entry for the ICO change or do you want me to do the changelog up to new including the ICO change (so basically the whole changelog sans future PRs)?

I'm asking because you guys don't squash commits on merge, which makes the commit history on main a mess to decipher. Any unimportant rustfmt, clippy fix, and review suggestion commit preserved forever. How do you guys even make changelogs normally? Do you look at the list of merged PRs instead of the commit history?

[197g]

I was just referring to the Changelog.md file, don't worry about it. When I write changelogs, I go through the auto-created merges for their issue number (git log --first-parent works for both rebase and merge strategies) and grab the title / contents from the Github API. Even well-done commit notes are summarized as how-and-why but that is not optimal as the user facing documentation anyways, we discuss this in the PR thread if anywhere.