MenuIcon problem with latest version of NPP

Franco Stellari

Original code that causes the MenuIcons to disappear every time the menu text is updated by selecting/removing a recent file and changing the menu accelerators:

void Accelerator::updateMenuItemByCommand(CommandShortcut csc)
{
int cmdID = csc.getID();
// Ensure that the menu item checks set prior to this update remain in affect.
UINT cmdFlags = GetMenuState(_hAccelMenu, cmdID, MF_BYCOMMAND);
cmdFlags = MF_BYCOMMAND | ((cmdFlags&MF_CHECKED) ? MF_CHECKED : MF_UNCHECKED);
::ModifyMenu(_hAccelMenu, cmdID, cmdFlags, cmdID, csc.toMenuItemString().c_str());
}

The solution is “theoretically simple” (replace ::ModiyMenu with ::SetMenuInfo) but for mysterious reasons it corrupts the menus that contain the “&” accelerator:

void Accelerator::updateMenuItemByCommand(CommandShortcut csc)
{
MENUITEMINFO mii;
mii.cbSize = sizeof(MENUITEMINFO);
mii.fMask = MIIM_STRING;
mii.dwTypeData = sMenu;
mii.dwTypeData = const_cast<LPTSTR>(csc.toMenuItemString().c_str());
::SetMenuItemInfo(_hAccelMenu, csc.getID(), FALSE, &mii);
}

So the simple solution becomes ugly:

void Accelerator::updateMenuItemByCommand(CommandShortcut csc)
{
LPTSTR sMenu = new TCHAR[_tcslen(csc.toMenuItemString().c_str()) + 1];
_tcscpy_s(sMenu, _tcslen(csc.toMenuItemString().c_str()) + 1, (LPTSTR)csc.toMenuItemString().c_str());

MENUITEMINFO mii;
mii.cbSize = sizeof(MENUITEMINFO);
mii.fMask = MIIM_STRING;
mii.dwTypeData = sMenu;
::SetMenuItemInfo(_hAccelMenu, csc.getID(), FALSE, &mii);

delete[]sMenu;

}

The above works but it would be great if one could figure out how to make the simple one work… I searched the internet but could not find a solution.

Claudia Frank

@Franco-Stellari

not sure if this is the way to go, this pointer and const … stuff still confuses me,
but what about

void Accelerator::updateMenuItemByCommand(CommandShortcut csc)
{
	MENUITEMINFO mii;
	mii.cbSize = sizeof(MENUITEMINFO);
	mii.fMask = MIIM_STRING;
	wstring sMenu(csc.toMenuItemString().c_str());
	mii.dwTypeData = &sMenu[0];
	::SetMenuItemInfo(_hAccelMenu, csc.getID(), FALSE, &mii);
}

Unfortunately, I cannot test the code on linux but the conversion should be ok.

Cheers
Claudia

Yaron

Hello Claudia,

I hope you’re doing well.

I’ve been discussing this issue with Franco.
Your code works perfectly. Thank you very much.

Still, I’d like to further investigate why Franco’s original code does not work as expected.

default

Not referring to this point, I assume you can not test it or it did not arouse your curiosity. :)
Is that correct?

Best regards.

Yaron

Hello again,

Franco’s original code is as follows:

void Accelerator::updateMenuItemByCommand(CommandShortcut csc)
{
   MENUITEMINFO mii;
   mii.cbSize = sizeof(MENUITEMINFO);
   mii.fMask = MIIM_STRING;
   mii.dwTypeData = const_cast<LPTSTR>(csc.toMenuItemString().c_str());
   ::SetMenuItemInfo(_hAccelMenu, csc.getID(), FALSE, &mii);
}

Best regards.

Claudia Frank

Hi Yaron,

I assume you can not test it or it did not arouse your curiosity. :)

I’m curios about it but unfortunately can’t test it. :-)
This makes it very hard to see what happens under the hood.
I always need to read the documentation and afterwards do testing
in order to understand what is going on.

From documentation dwTypeData

When using with the SetMenuItemInfo function, this member should contain a value whose type is specified by the fType member.

dwTypeData is used only if the MIIM_STRING flag is set in the fMask member

For me it is not 100% clear what it means, so testing would have, maybe, shed some light on it,
but without environment …
Maybe you could give it a try and add

mii.fType = MFT_STRING

in addition it could be that cch member needs to be specified as well.

You see, testing …

Cheers
Claudia

Franco Stellari250

My understanding is that cch is need only for the GetMenuInfo… to tell how big is your string. It’s not needed for the SetMenuInfo since you are passing a null terminated string. Anyhow, I did try to play with it with no result.

I also tried your other hypothesis that using the fType may have an effect but I also could not get it to work.

I think the use of the SetMenuInfo is correct… I just don’t understand why the string gets mangled… and only related to the “&”… may be there is some char type conversion going on. At the end the char pointer returned by the wstring or the c_str should be theoretically identical.

Yaron

Hello Claudia,

Thank you very much. I appreciate your help.
I’ll keep discussing it with Franco.

Best regards.

Claudia Frank

@Franco-Stellari250, @Yaron

I tried to find something which could explain the behavior but
was out of luck. In general I would expect that it works as Franco initially
wrote, const_cast removes or adds the “constness” of a variable.
Maybe you wanna check the memory window while debugging npp,
like settings a breakpoint before SetMenuItemInfo and check the location where
mii.dwTypeData refers to - maybe this gives a hint what is going on.
Or compare with ModifyMenu function like

void Accelerator::updateMenuItemByCommand(CommandShortcut csc)
{
    int cmdID = csc.getID();
    // Ensure that the menu item checks set prior to this update remain in affect.
    UINT cmdFlags = GetMenuState(_hAccelMenu, cmdID, MF_BYCOMMAND);
    cmdFlags = MF_BYCOMMAND | ((cmdFlags&MF_CHECKED) ? MF_CHECKED : MF_UNCHECKED);
    LPTSTR test =  const_cast<LPTSTR>(csc.toMenuItemString().c_str());
    ::ModifyMenu(_hAccelMenu, cmdID, cmdFlags, cmdID, csc.toMenuItemString().c_str());
}

if test variable is behaving the same? Maybe give it a try with

void Accelerator::updateMenuItemByCommand(CommandShortcut csc)
{
    int cmdID = csc.getID();
    // Ensure that the menu item checks set prior to this update remain in affect.
    UINT cmdFlags = GetMenuState(_hAccelMenu, cmdID, MF_BYCOMMAND);
    cmdFlags = MF_BYCOMMAND | ((cmdFlags&MF_CHECKED) ? MF_CHECKED : MF_UNCHECKED);
    LPCTSTR test =  csc.toMenuItemString().c_str();
    ::ModifyMenu(_hAccelMenu, cmdID, cmdFlags, cmdID, test);
}

to see if there is a problem when assigning to a new variable.

I know, nothing specific and a lot of fishing in the dark. Sorry.

Cheers
Claudia

pnedev

Hello guys,

@Franco-Stellari250 ,

I’m sorry if you have already tried this but the correct usage according to MENUITEMINFO specification should be

mii.fMask = MIIM_TYPE;
mii.fType = MFT_STRING;

Best Regards,
Pavel

Yaron

Hello Claudia and Pavel,

Both LPTSTR test = and LPCTSTR test = have the same result as Franco’s original code.

mii.fMask = MIIM_TYPE;
mii.fType = MFT_STRING;

I’ve tried that and some other combinations: again, the same result.

Thank you both. I appreciate your kind help.

Best regards.

Claudia Frank

@Yaron

so you are saying when assinging csc.toMenuItemString().c_str() to a variable
even ModifyMenu corrupts the menu?
Doesn’t this mean that there is something wrong with the expected result of
csc.toMenuItemString().c_str().
Franco already had the idea that there might be some kind of conversion going on
but I didn’t find it in the code (but this means nothing could be easily overlooked).

But when using ModifyMenu and csc.toMenuItemString().c_str() as parameter
which is expected to be a LPCTSTR what is the difference when using

LPCTSTR test =  csc.toMenuItemString().c_str();
::ModifyMenu(_hAccelMenu, cmdID, cmdFlags, cmdID, test);

???

Cheers
Claudia

Yaron

Hello Claudia,

so you are saying when assinging csc.toMenuItemString().c_str() to a variable
even ModifyMenu corrupts the menu?

Indeed.

But when using ModifyMenu and csc.toMenuItemString().c_str() as parameter
which is expected to be a LPCTSTR what is the difference when using…?

That’s a good question. It requires some serious investigation (and investigator). :)

Thanks again.
Have a nice weekend.

pnedev

Hi again,

If the code below is the original one that is not working as expected I might have an explanation why it is not behaving well.

void Accelerator::updateMenuItemByCommand(CommandShortcut csc)
{
    MENUITEMINFO mii;
    mii.cbSize = sizeof(MENUITEMINFO);
    mii.fMask = MIIM_STRING;
    mii.dwTypeData = const_cast<LPTSTR>(csc.toMenuItemString().c_str());
    ::SetMenuItemInfo(_hAccelMenu, csc.getID(), FALSE, &mii);
}

The problem is that when SetMenuItemInfo is called the mii.dwTypeData is not valid.
That’s because mii.dwTypeData is a pointer to a TCHAR array - it is not the actual array and something needs to hold the actual data. But const_cast<LPTSTR>(csc.toMenuItemString().c_str()) is not holding anything because toMenuItemString() returns temporal std::basic_string<TCHAR> object that is not saved in our scope and this temporal string ceases to exist the moment its pointer is stored in mii.dwTypeData. At the moment SetMenuItemInfo is called the mii.dwTypeData is pointing to a free memory that is on the stack and is overwritten the moment a new automatic variable is created.

That’s why Claudia’s solution with local variable that is holding the new string is working - mii.dwTypeData is pointing to local array that is still valid when SetMenuItemInfo is called.

BR,
Pavel

Yaron

Hello Pavel,

Thank you for the explanation. I appreciate it.

There’s still a “missing link”:
What’s the difference between

::ModifyMenu(_hAccelMenu, cmdID, cmdFlags, cmdID, csc.toMenuItemString().c_str());

and

LPCTSTR test =  csc.toMenuItemString().c_str();
::ModifyMenu(_hAccelMenu, cmdID, cmdFlags, cmdID, test);

?

Best regards.

pnedev

Hello Yaron,

The problem is the same.

In

LPCTSTR test =  csc.toMenuItemString().c_str();
::ModifyMenu(_hAccelMenu, cmdID, cmdFlags, cmdID, test);

test is a pointer to const TCHAR that is pointing to the data of a temporary std::basic_string<TCHAR> object returned by toMenuItemString(). The problem is that the string object (the one actually holding the data) is temporary - it is not saved by the assignement to test and it is destroyed immediately after the assignment.

That code is practically equivalent to:

int returnInteger()
{
    int ret = 5;
    return ret;
}

const int* pInt = returnInteger();

Here pInt is pointing to nonexistent int object.

Now

::ModifyMenu(_hAccelMenu, cmdID, cmdFlags, cmdID, csc.toMenuItemString().c_str());

is a function call which takes a pointer to temporary object. I’m not sure how this will behave and if it will behave consistently if the code is compiled with different compilers but that might work. That’s because here the temporary might still be alive even though it is not stored anywhere - it might still live on the stack. For sure the temporary ceases to exist on the next line (as is the case with the ‘in-between’ assignment).
As an illustration (continued from the above one) it is equivalent to

void useIntPointer(const int* pInt)
{
    ...
}

useIntPointer(returnInteger());

To be perfectly safe there’s nothing wrong with doing that:

std::basic_string<TCHAR> menuItem =  csc.toMenuItemString();
::ModifyMenu(_hAccelMenu, cmdID, cmdFlags, cmdID, menuItem.c_str());

Now the menuItem variable is actually holding the whole string data (it is not just a pointer) and it will be valid in our current scope.
The new compilers might not even make a copy of the string (there might be no assignment penalty) as they might simply apply move operation and directly reuse the string data returned by toMenuItemString().

BR,
Pavel

Yaron

Hello Pavel,

Thank you for the detailed and well built explanation.

The issue is clear.
With your permission, I’ll try to slightly sharpen it.

int * returnIntPointer()
{
    int ret = 5;
    return &ret;
}

void useIntPointer(const int* pInt)
{
    ...
}

useIntPointer(returnIntPointer());

ret is stored and valid for the duration of useIntPointer().
It should lose its validity only when we reach the end of useIntPointer(returnIntPointer());; i.e. when useIntPointer() ends.

Is that correct?

Best regards.

pnedev

Hello Yaron,

You are welcome.

No, the assumption is not correct.

ret is destroyed (stops existing) the moment we exit returnIntPointer():
ret is a local variable of returnIntPointer() after all.

If you return it by value (as in my example) a temporary copy of ret is returned where the call to returnInteger() is. This temporary exist only in that place and if it is not immediately copied then it is lost.

BR

Yaron

Hello Pavel,

ret is a local variable of returnIntPointer() after all.

I understand that.
My idea was that returnIntPointer() is not destroyed until the following ; is reached.
IOW: since we use useIntPointer(returnIntPointer());, it’s valid until we return to );.

Otherwise, is a new temporary copy of ret created?
And what’s the exact definition? You can copy it just at the very beginning of the function? Isn’t it a bit far fetched?

Another option is that you assume there’s not enough time to destroy ret because of the events proximity.
But that doesn’t make sense either.

I suppose this question can be tested.

Thanks again. I appreciate your contribution to MenuIcons as well. :)
BR

pnedev

Hello Yaron,

My idea was that returnIntPointer() is not destroyed until the following ; is reached.
IOW: since we use useIntPointer(returnIntPointer());, it’s valid until we return to );

That’s true for returnInteger() not for returnIntPointer() and returnIntPointer() is a function - it is not destroyed. The value (variable) it returns is not destroyed until the next ;.

Let’s look at returnIntPointer()'s code:

int * returnIntPointer()
{
    int ret = 5;
    return &ret;
}

When we enter the function, ret is created on the stack, the stack pointer is moved (let’s say incremented for clarity).
Now we return ret’s address and exit the function. When we exit, ret is freed -> the stack pointer is decremented.
Now the returned pointer points to free stack memory that will hold the old ret’s value until another local (automatic) variable is created and that usually happens when you enter another function. In our case this function is useIntPointer(). So in useIntPointer() our pointer points to invalid data.

When you return ret by value (as in returnInteger()) the returned value is either in processor register or on the stack before all returnInteger() local variables are put there and later - destroyed. IOW there is no copy performed - it is simply a compiler matter.

Your assumption is correct to the point that whatever variable a function returns (temporary) lives until the next ; where the function is called.
In returnIntPointer() the returned variable is int* and it lives until next ; after the call but it points to what? Some free memory that is overwritten perhaps and assigned to another variable.
In returnInteger() the returned variable is int and it’s just what you need (it is not extra-copied on return, don’t worry, it’s just created by the compiler in the appropriate way). You can assign it to another variable to store it or directly give it to another function (copying it or using it’s address - doesn’t matter) - it is valid until the next ; after the call.

Hope that clarifies.

BR

Yaron

Hello Pavel,

The issue is clearer now.
Some of the terms you’ve used are new to me. I’ll have to expand my basic knowledge. :)

Thank you for the interesting explanation. As always, I appreciate your patience.

BR