#15662 closed defect (fixed)

wxMenuBar::Insert() puts the new menu in between window and help menu eventually

Reported by: laro Owned by:
Priority: normal Milestone: 3.0.1
Component: wxMSW Version: 3.0.0
Keywords: wxMenuBar::Insert, window menu, help menu, MSWPositionForWxMenu Cc:
Blocked By: Blocking:
Patch: yes

Description

Insert() should put the new menu in front of the window menu instead, e.g. not in between window and help menu. As the window menu is "hidden" to the programmer, window and help menu should be seen as a unit, and there is no other option to insert new menus in front of it.

Version should be 3.0-svn (WX_3_0_BRANCH).

Attachments (2)

menu.cpp.patch download (770 bytes) - added by laro 13 months ago.
menu.cpp.2.patch download (803 bytes) - added by laro 13 months ago.

Download all attachments as: .zip

Change History (11)

Changed 13 months ago by laro

Changed 13 months ago by laro

comment:1 Changed 13 months ago by laro

Second attachement (menu.cpp.2.patch) is "untabified".

comment:2 follow-up: Changed 13 months ago by vadz

  • Milestone set to 3.0.1
  • Status changed from new to confirmed
  • Version changed from 3.0.0-rc2 to 3.0.0

Thanks for the patch! Does this fix the issue of #1732 too by chance?

In any case, this patch should be fine for 3.0 branch (even though a bit too late for 3.0.0 itself). In the trunk it would be better to add a virtual function for the menu insertion to wxFrame and override it to perform this index adjustment in wxMDIParentFrame only, this would make the code cleaner.

comment:3 in reply to: ↑ 2 Changed 13 months ago by laro

Replying to vadz:

Does this fix the issue of #1732 too by chance?

Yes, #1732 is indeed exactly my scenario, I just didn't find it in the custom query. So should be fixed.

(Is there an option to search in summary, keywords, and description all at once?)

comment:4 Changed 12 months ago by vadz

Actually I've looked at this code and realized that I have really no idea about what is it doing. The call to MSWPositionForWxMenu() here doesn't seem to make any sense. So what about simply removing it and doing this instead:

  • src/msw/menu.cpp

    diff --git a/src/msw/menu.cpp b/src/msw/menu.cpp
    index 8a25776..3d3ea7e 100644
    a b bool wxMenuBar::Insert(size_t pos, wxMenu *menu, const wxString& title) 
    13671367        (GetHmenu() != 0); 
    13681368#endif 
    13691369 
    1370     int mswpos = (!isAttached || (pos == m_menus.GetCount())) 
    1371         ?   -1 // append the menu 
    1372         :   MSWPositionForWxMenu(GetMenu(pos),pos); 
    1373  
    13741370    if ( !wxMenuBarBase::Insert(pos, menu, title) ) 
    13751371        return false; 
    13761372 
    bool wxMenuBar::Insert(size_t pos, wxMenu *menu, const wxString& title) 
    13981394            wxLogLastError(wxT("TB_INSERTBUTTON")); 
    13991395            return false; 
    14001396        } 
    1401         wxUnusedVar(mswpos); 
    14021397#else 
    1403         if ( !::InsertMenu(GetHmenu(), mswpos, 
     1398        // We have a problem with the index if there is an extra "Window" menu 
     1399        // in this menu bar, which is added by wxMDIParentFrame to it directly 
     1400        // using Windows API (so that it remains invisible to the user code), 
     1401        // but which does affect the indices of the items we insert after it. 
     1402        // So we check if any of the menus before the insertion position is a 
     1403        // foreign one and adjust the insertion index accordingly. 
     1404        int mswExtra = 0; 
     1405 
     1406        // Skip all this if the total number of menus matches (notice that the 
     1407        // internal menu count has already been incremented by wxMenuBarBase:: 
     1408        // Insert() call above, hence -1). 
     1409        int mswCount = ::GetMenuItemCount(GetHmenu()); 
     1410        if ( mswCount != -1 && 
     1411                static_cast<unsigned>(mswCount) != GetMenuCount() - 1 ) 
     1412        { 
     1413            wxMenuList::compatibility_iterator node = m_menus.GetFirst(); 
     1414            for ( size_t n = 0; n < pos; n++ ) 
     1415            { 
     1416                if ( ::GetSubMenu(GetHmenu(), n) != GetHmenuOf(node->GetData()) ) 
     1417                    mswExtra++; 
     1418                else 
     1419                    node = node->GetNext(); 
     1420            } 
     1421        } 
     1422 
     1423        if ( !::InsertMenu(GetHmenu(), pos + mswExtra, 
    14041424                           MF_BYPOSITION | MF_POPUP | MF_STRING, 
    14051425                           (UINT_PTR)GetHmenuOf(menu), title.t_str()) ) 
    14061426        { 

This seems to work for me, does anybody see any problems with this?

comment:5 Changed 12 months ago by VZ

  • Resolution set to fixed
  • Status changed from confirmed to closed

(In [75241]) Fix problem with inserting menus in wxMDIParentFrame menu bar.

Account for the "Window" menu (and any other foreign windows lurking in our
menu bar) correctly in wxMenuBar::Insert() instead of the old code which
apparently tried to do something similar but didn't make much sense and didn't
work when trying to insert the menu at the position actually occupied by the
"Window" menu in the menu bar.

Closes #15662, #1732.

comment:6 Changed 12 months ago by VZ

(In [75243]) Fix problem with inserting menus in wxMDIParentFrame menu bar.

Account for the "Window" menu (and any other foreign windows lurking in our
menu bar) correctly in wxMenuBar::Insert() instead of the old code which
apparently tried to do something similar but didn't make much sense and didn't
work when trying to insert the menu at the position actually occupied by the
"Window" menu in the menu bar.

Closes #15662, #1732.

comment:7 Changed 12 months ago by laro

  • Resolution fixed deleted
  • Status changed from closed to reopened

No offense, but I can't see how your code solves the core problem of inserting a menu left of the window menu.

My code simply checks whether the wxMenu left of the given position is the "Windows" menu. If it is, then I decrement the MSW-position by one to "skip" it. (The check would be simpler if we had an m_windowMenu, BTW.)

You replaced MSWPositionForWxMenu(), which is used in four other places anyway, with a local loop. We may better improve its documenation or its interface instead.

comment:8 Changed 12 months ago by vadz

  • Status changed from reopened to infoneeded_new

I don't see how to reproduce the problem, could you please explain? The only test case I had was the one from #1732 and it passed after my changes. If you have another still failing test case, please attach it here. TIA!

As for MSWPositionForWxMenu(): yes, this is a stupid hack and should be replaced by providing a virtual function to do the index translation which would simply be overridden in wxMDIParentFrame, as I already wrote both in #1732 and here. But at least in another places it works because the menu already exists. Calling it before the menu is inserted doesn't make any sense to me, considering that its implementation searches for the pointer to this menu in the menu bar. So this is why I dropped it here. Perhaps I'm missing something, of course, but if so, please explain what exactly is it?

comment:9 Changed 12 months ago by laro

  • Resolution set to fixed
  • Status changed from infoneeded_new to closed

Sorry, my fault.
Everything works as expected.

Note: See TracTickets for help on using tickets.