Re: [PATCH 2/2] shell32/iconcache: Break as soon as one shortcut failed to overlay

[Gabriel Ivăncescu]

On Thu, Nov 29, 2018 at 1:13 PM Huw Davies  wrote:
> On Mon, Nov 26, 2018 at 12:22:04PM +0200, Gabriel Ivăncescu wrote:
> > There is no need to initialize and destroy shortcut icons that we haven't
> > even processed.
> > Signed-off-by: Gabriel Ivăncescu
> >
> > dlls/shell32/iconcache.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/dlls/shell32/iconcache.c b/dlls/shell32/iconcache.c
> > index 44422ab..b9264bb 100644
> > --- a/dlls/shell32/iconcache.c
> > +++ b/dlls/shell32/iconcache.c
> > @@ -361,7 +361,6 @@ static BOOL get_imagelist_icon_size(int list, SIZE *size)
> >  static INT SIC_LoadIcon (const WCHAR *sourcefile, INT index, DWORD flags)
> >  {
> >      HICON hicons[ARRAY_SIZE(shell_imagelists)] = { 0 };
> > - HICON hshortcuts[ARRAY_SIZE(hicons)] = { 0 };
> > unsigned int i;
> > SIZE size;
> > INT ret = -1;
> >
> > @@ -376,6 +375,7 @@ static INT SIC_LoadIcon (const WCHAR *sourcefile, INT index, DWORD flags)
> >  if (flags & GIL_FORSHORTCUT)
> >  {
> > -
> >    HICON hshortcuts[ARRAY_SIZE(hicons)];
> >    BOOL failed = FALSE;
> >
> >    for (i = 0; i < ARRAY_SIZE(hshortcuts); i++)
> >
> > @@ -384,13 +384,15 @@ static INT SIC_LoadIcon (const WCHAR *sourcefile, INT index, DWORD flags)
> >              {
> >                  WARN("Failed to create shortcut overlaid icons.\n");
> >                  failed = TRUE;
> > -
> >            break;
> >        }
> >    }
> >
> >    if (failed)
> >    {
> > -
> >        for (i = 0; i < ARRAY_SIZE(hshortcuts); i++)
> > -
> >            DestroyIcon(hshortcuts[i]);
> > -
> >        unsigned int k;
> > -
> >        for (k = 0; k < i; k++)
> > -
> >            DestroyIcon(hshortcuts[k]);
> >        flags &= ~GIL_FORSHORTCUT;
> >    }
> >    else
>
> Rather than optimizing the failure case, we should focus on addressing
> the real problem.
>
> Huw.

By real problem, you mean the icon not being reconstructed from the
available ones? I planned to do that after, because the patch that
does it is a bit convoluted. Now I just sent the easy bits first.

I mean, I *totally* understand why that patch feels like a semi-hack.
At worst I was planning to get it into wine-staging, at least until we
find a better way to do it.

BTW, I asked Nikolay before but he didn't answer -- I'm not sure how
to make tests for that patch's scenario, because I don't know how to
reproduce the failure of only some icon sizes in a wine test (as in
the bug report via TurnItOn.exe). I mean, it's trivial to reproduce it
with TurnItOn.exe but I can't just ship that with the test obviously.