Opened 10 years ago

Closed 11 months ago

Last modified 11 months ago

#1732 closed defect (fixed)

"Window" menu not taken into account by indices in wxMDIParentFrame menu bar

Reported by: dodywijaya Owned by:
Priority: normal Milestone:
Component: wxMSW Version:
Keywords: mdi Cc: dodywijaya
Blocked By: Blocking:
Patch: no

Description

mb = self.GetMenuBar()
mb.Insert(2,newmenu,'Tool')

results:
File Edit Window Tool Help

mb.Insert(1,newmenu,'Tool')

results:
File Tool Edit Window Help

is this considered a bug in wx?

Looks like it. Enter a bug report about it with a category
of wxMSW
specific.

--
Robin Dunn
Software Craftsman

Attachments (2)

mdi-sample.patch download (2.8 KB) - added by robind 18 months ago.
1732.patch download (4.0 KB) - added by oneeyeman 18 months ago.
Patch to fix the bug

Download all attachments as: .zip

Change History (22)

comment:1 Changed 3 years ago by oneeyeman

C++ menu sample works OK on SVN TRUNK.

Can be closed.

comment:2 Changed 3 years ago by vadz

Could you please attach the patch you used for testing this? I'm curious because I don't remember fixing anything that could explain this bug.

comment:3 Changed 3 years ago by oneeyeman

In the menu sample just change line 712 (rev. 70307) to become:

GetMenuBar()->Insert(2, menu, title);

and then:

GetMenuBar()->Insert(1, menu, title);

comment:4 Changed 3 years ago by vadz

Yes but the menu sample doesn't use MDI... I'm pretty sure that the problem was MDI-specific (in particular, due to the wrong handling of "Window" menu), no wonder you can't reproduce it in the menu sample.

comment:5 Changed 19 months ago by oneeyeman

  • Component changed from wxMSW to wxPython

It looks like this bug is wxPython specific.
I have SVN TRUNK rev. 73677 and tried to modify an mdi sample to reproduce the bug.

Inserting the following code:

wxMenu *menu = new wxMenu;
GetMenuBar()->Insert( 2, menu, "Foo" );

in either OnAbout() or OnNewWindow() does not produce extra menu in the menu bar.

comment:6 Changed 19 months ago by robind

  • Component changed from wxPython to wxMSW

It's not that there is an extra menu, but that the new menu is showing up after the MDI-generated Window menu instead of before it. In other words, if you have these menus in the menubar: [File Edit Window Help] and then do menubar->Insert(2, menu, "NEWMENU"); then the expected result is [File Edit NEWMENU Window Help] but what you get instead is [File Edit Window NEWMENU Help].

comment:7 Changed 19 months ago by robind

Let's make that a little more readable...

Starting menubar: [File Edit Window Help]

Execute: menubar->Insert(2, menu, "NEWMENU");

Expected result: [File Edit NEWMENU Window Help]

Actual result: [File Edit Window NEWMENU Help]

comment:8 follow-up: Changed 19 months ago by oneeyeman

Robin,
It's not that I don't understand this.
All I'm saying is that I tried to reproduce this in C++ sample and failed.
Inserting those 2 lines mentioned after the child window creation does not produce extra menu anywhere.
If I insert those 2 lines in OnAbout() and then run the sample, create the child and select "Help->About" I will still see the same menu. No new menu item will be shown.

Do you have a way of reproducing this in C++?

comment:9 in reply to: ↑ 8 Changed 18 months ago by robind

  • Status changed from new to confirmed

Replying to oneeyeman:

Robin,
It's not that I don't understand this.
All I'm saying is that I tried to reproduce this in C++ sample and failed.
Inserting those 2 lines mentioned after the child window creation does not produce extra menu anywhere.
If I insert those 2 lines in OnAbout() and then run the sample, create the child and select "Help->About" I will still see the same menu. No new menu item will be shown.

Do you have a way of reproducing this in C++?

Yes. Since the MDI sample assigns menubars to the child frames then you do not see the problem in the parent's menubar. Apply the attached patch which comments out that part of the code and adds some test code for inserting a new menu. Run the sample, add at least one child window, then press F1. After the message box is dismissed you should see a new menu after the Window menu, but it should have been inserted before it.

Changed 18 months ago by robind

Changed 18 months ago by oneeyeman

Patch to fix the bug

comment:10 Changed 18 months ago by oneeyeman

  • Patch set

Attached please find the patch that fixes the bug.
It includes the sample modifications in order to test that the bug is fixed.

comment:11 follow-up: Changed 18 months ago by vadz

Sorry, you'll have to explain your changes because I don't understand them at all. In particular, the one in src/common/menucmn.cpp and the first one in src/msw/mdi.cpp.

comment:12 in reply to: ↑ 11 Changed 18 months ago by oneeyeman

Vadim,
Replying to vadz:

Sorry, you'll have to explain your changes because I don't understand them at all. In particular, the one in src/common/menucmn.cpp and the first one in src/msw/mdi.cpp.

wxMenuBar::Append()/Insert()/Remove() calls Attach()/Detach() and so the calls to Attach()/Detach() has to be removed, otherwise the code will assert.

About the menucmn.cpp. Most likely it is from the other ticket and can be removed. But I want to make sure so I'd need to test. Problem is I will not have access to that machine until Monday. So, if you can remove this from the patch it would be great. Otherwise I will check that Monday.

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

Actually, thinking more about this, I believe the original intention of not using wx-level methods was to avoid making "Window" menu appear in the list of menu bar menus. If we want it to appear at wx level, then a lot more code dealing with it could probably be simplified...

But I am not sure if it's desirable to have it at wx level or not.

comment:14 in reply to: ↑ 13 Changed 18 months ago by oneeyeman

Vadim,
Replying to vadz:

Actually, thinking more about this, I believe the original intention of not using wx-level methods was to avoid making "Window" menu appear in the list of menu bar menus. If we want it to appear at wx level, then a lot more code dealing with it could probably be simplified...

But I am not sure if it's desirable to have it at wx level or not.

What is the difference between "Window" menu and "File" menu from wx point of view?
AFAICS, the current code makes it inconsistent and this patch bring it in sync. Besides it somewhat eliminates code duplication.

Is there a link on the discussion about this?

comment:15 Changed 18 months ago by vadz

The difference is that the "File" menu is created by the program, i.e. user code itself. "Window" menu is not.

comment:16 Changed 18 months ago by oneeyeman

Problem is they are both just menus. And adding "Window" menu in the current code brings the internal wx structure out of sync.
So from wx point of view "Window" menu and "File" menu are not different.
They are different from the user perspective because user have control over "File" menu - what goes in and what goes out, when to enable and when to disable "File" submenu's. (S)He can even call it "Foo" instead of "File" if the task at hand needs the first menu to be called "Foo". But he don't have control over "Window" menu. User can turn it off completely or turn it on completely, but it will still be a "Window" menu and (s)he won't have control over what goes in and how it handles everything.

If we can agree on this, then this patch needs to be applied.

Also, I don't know about the Mac but on Linux/GTK every MDI application (standard GNOME Text Editor comes to mind) has "Document" menu in the same position as "Window" menu. Is there a plan to implement this?

comment:17 Changed 12 months ago by vadz

  • Keywords mdi added
  • Patch unset
  • Summary changed from inserting menu at menubar MDIParentFrame to "Window" menu not taken into account by indices in wxMDIParentFrame menu bar

I'm almost certain that this patch will result in other problems, due to a "foreign" menu appearing in our menu bar. A much safer (and probably also preferable, from cross-platform compatibility point of view) solution would be to just add an index translation function to wxFrame and override it in wxMDIParentFrame to take the "Window" menu into account, but still hide it from the user code.

comment:18 Changed 11 months ago by vadz

See also #15662.

comment:19 Changed 11 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:20 Changed 11 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.

Note: See TracTickets for help on using tickets.