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

[Huw Davies]

On Thu, Nov 29, 2018 at 01:17:57PM +0200, Gabriel Ivăncescu wrote:
> On Thu, Nov 29, 2018 at 1:13 PM Huw Davies <huw at codeweavers.com> 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 <gabrielopcode at gmail.com>
> > > ---
> > >  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.

Err, what this patch does is to optimize things when
SIC_OverlayShortcutImage() fails (and unnecessarily move around a
variable definition).  So the real problem would be why that function
fails.

FWIW the other patch was a bit of a hack, but having the imagelists go
out of sync seemed worse than a missing icon, so I signed off on that
one.

Huw.