MenuIcon problem with latest version of NPP

Franco Stellari250

It has been reported and I have confirmed that the menu icons added by MenuIcon plugin disappear when:

recent file list is cleared
recent file is opened or changed
menu shortcuts get updated

I performed some investigations that it seems to me (amateur programmer) that the issue may be related to the use (twice) of ::ModifyMenu(…) in shortcut.cpp.

I did perform some basic test and it seems that changing the text with ::ModifyMenu wipes everything.
The use of the more modern ::SetMenuInfo instead seems to maintain the check marks and bitmaps.

I would like to kindly suggest these two changes (untested), or some variations of them, in the code:

At line 631 I think the intention is to update the text of the menu and keep the check state:

// 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());

This should become something like this (untested)… which should keep the icon and the check state:

MENUITEMINFO mii;
mii.cbSize = sizeof mii;
mii.fMask = MIIM_STRING;
mii.dwTypeData = _csc.toMenuItemString().c_str();
mii.cch = _tsrlen(mii.dwTypeData);
::SetMenuItemInfo(_hAccelMenu, cmdID, FALSE, &mii);

At line 780

 ::ModifyMenu(_hAccelMenu, id, MF_BYCOMMAND, id, menuItem.c_str());

Should become:

MENUITEMINFO mii;
mii.cbSize = sizeof mii;
mii.fMask = MIIM_STRING;
mii.dwTypeData = menuItem.c_str();
mii.cch = _tsrlen(mii.dwTypeData);
::SetMenuItemInfo(_hAccelMenu, id, FALSE, &mii);

May be somebody with more experience than me can form the githup repository and test these changes and make a pull request.

I home this helps.

Franco

chcg

Seems related to https://notepad-plus-plus.org/community/topic/13262/menuicons-plugin-1v21-released/12

Franco Stellari250

Yes it is but for whatever reason I could not add a message to the thread.

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