#2039 WinMerge 2.13.14 don't show menu icons

[Tim Gerundt]

I tested WinMerge 2.13.14 on a German Windows 7 and it don't show icons in the menus. WinMerge 2.13.13 had no problem to show it.

Maybe had Takashi's WinMerge fork a solution?

Workaround for broken menu icons in WinMerge compiled with Visual Studio 2010
http://bitbucket.org/sdottaka/winmerge-v2/changeset/b852f0587157

Workaround for bug in CImageList::DrawIndirect in MFC100
http://bitbucket.org/sdottaka/winmerge-v2/changeset/9afc2e477285

Greetings,
Tim

[Kimmo Varis]

Can confirm missing icons. Do you have time to check if Takashi's changes fix the issue?

[Tim Gerundt]

merge type changes from WinMerge JP

[Tim Gerundt]

The two changesets from bitbucket don't fix the problem, but it must some problem with type converting.

If I merge only the type changes like UINT -> int (no code changes) I the icons works again in WinMerge.

[Tim Gerundt]

Ok, problem must be one or more type changes in r7215:
http://winmerge.svn.sourceforge.net/viewvc/winmerge?view=revision&revision=7215

If I revert the changes, it works.

[Kimmo Varis]

Very good that you found the problematic commit.

However, a bit sad it is that commit... It fixes many real warnings/bugs by using correct types. But there must be a bug somewhere...

[Tim Gerundt]

If you want, I can look at the weekend, if I find the real bug so we can keep the real fixes.

[Kimmo Varis]

It is not so simple as trying to remove one or more of those changes. As you see the type must be changed for a chain of calls. So removing some diff breaks the chain and causes of course failure which has nothing to do with original bug.

So I just need to read through the code carefully, more probably there is some conversions missing than too many conversions. Because -1 of int and INT_PTR are different... int is 32 bits and INT_PTR 64 bits for x64.

[Tim Gerundt]

patch to fix the problem

[Tim Gerundt]

Ok, I found the bug: You rewrite the function AddToGlobalImageList() to return INT_PTR but use internal UINT on two places.

This patch fix the problem for me:

Index: Src/BCMenu.cpp

--- Src/BCMenu.cpp (Revision 7410)
+++ Src/BCMenu.cpp (Arbeitskopie)
@@ -3178,8 +3178,8 @@
GetDisabledBitmap(bmp3);
pWnd->ReleaseDC(pDC); // Release the DC
}
- UINT numcurrent=(UINT)m_AllImagesID.GetSize();
- UINT existsloc = -1;
+ INT_PTR numcurrent=m_AllImagesID.GetSize();
+ INT_PTR existsloc = -1;
for(UINT i=0;i<numcurrent;++i){
if(m_AllImagesID[i]==nID){
existsloc=i;

[Kimmo Varis]

Hmm.. You certainly found one bug. existsloc being UINT gets different value than it being INT_PTR and its been used as intereger later on. So yes, good catch!

But I think you also add new warning since now in next loop (shown in patch below) you again compare UINT and INT_PTR values. But since all values of 'i' are positive I don't think there is a bug - just compiler warning. But perhaps the correct thing would be to also use INT_PTR for 'i' in that loop...

[Tim Gerundt]

Ok, I committed the patch (with your modification) to SVN (In Revision 7413).

Should I close this bug, or will you first search for other possible problems?

[Kimmo Varis]

Just close this bug - I've no time to review that whole file now - it is LONG file.