Community

    • Login
    • Search
    • Recent
    • Tags
    • Popular
    • Users
    • Groups
    • Search

    MenuIcon problem with latest version of NPP

    Plugin Development
    6
    25
    15232
    Loading More Posts
    • Oldest to Newest
    • Newest to Oldest
    • Most Votes
    Reply
    • Reply as topic
    Log in to reply
    This topic has been deleted. Only users with topic management privileges can see it.
    • Franco Stellari250
      Franco Stellari250 last edited by

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

      1. recent file list is cleared
      2. recent file is opened or changed
      3. 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:

      1. 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);
      
      1. 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

      1 Reply Last reply Reply Quote 0
      • chcg
        chcg last edited by

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

        1 Reply Last reply Reply Quote 0
        • Franco Stellari250
          Franco Stellari250 last edited by

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

          1 Reply Last reply Reply Quote 0
          • Franco Stellari
            Franco Stellari last edited by

            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 1 Reply Last reply Reply Quote 0
            • Claudia Frank
              Claudia Frank @Franco Stellari last edited by

              @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

              1 Reply Last reply Reply Quote 0
              • Yaron
                Yaron last edited by

                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.

                1 Reply Last reply Reply Quote 0
                • Yaron
                  Yaron last edited by

                  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.

                  1 Reply Last reply Reply Quote 0
                  • Claudia Frank
                    Claudia Frank last edited by 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

                    1 Reply Last reply Reply Quote 0
                    • Franco Stellari250
                      Franco Stellari250 last edited by

                      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.

                      1 Reply Last reply Reply Quote 0
                      • Yaron
                        Yaron last edited by

                        Hello Claudia,

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

                        Best regards.

                        1 Reply Last reply Reply Quote 0
                        • Claudia Frank
                          Claudia Frank last edited by

                          @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

                          1 Reply Last reply Reply Quote 0
                          • pnedev
                            pnedev last edited by

                            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

                            1 Reply Last reply Reply Quote 0
                            • Yaron
                              Yaron last edited by

                              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 1 Reply Last reply Reply Quote 0
                              • Claudia Frank
                                Claudia Frank @Yaron last edited by

                                @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

                                1 Reply Last reply Reply Quote 0
                                • Yaron
                                  Yaron last edited by

                                  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.

                                  1 Reply Last reply Reply Quote 0
                                  • pnedev
                                    pnedev last edited by

                                    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

                                    1 Reply Last reply Reply Quote 1
                                    • Yaron
                                      Yaron last edited by

                                      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.

                                      1 Reply Last reply Reply Quote 0
                                      • pnedev
                                        pnedev last edited by

                                        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

                                        1 Reply Last reply Reply Quote 1
                                        • Yaron
                                          Yaron last edited by

                                          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.

                                          1 Reply Last reply Reply Quote 0
                                          • pnedev
                                            pnedev last edited by

                                            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

                                            1 Reply Last reply Reply Quote 0
                                            • First post
                                              Last post
                                            Copyright © 2014 NodeBB Forums | Contributors