Opened 11 years ago

Closed 10 years ago

Last modified 10 years ago

#11644 closed defect (fixed)

check menu items report incorrect state

Reported by: BuschnicK Owned by:
Priority: normal Milestone: 2.9.3
Component: wxMSW Version: 2.9.0
Keywords: menu item, check Cc:
Blocked By: Blocking:
Patch: yes

Description

I'm using two types of menus on WindowsXP: a normal application menu attached to the frame and context menus. Both menus contain a check menu item with the same identifier which triggers the same OnItemClicked handler. However there is no way to figure out the item state in the menu handler. The menus differ like so:

regular application menu, item is checked and gets unchecked:

  • event.IsChecked() returns false
  • FindItem()->IsChecked() returns false

regular application menu, item is unchecked and gets checked:

  • event.IsChecked() returns true
  • FindItem()->IsChecked() returns true

context menu, item is checked and gets unchecked:

  • event.IsChecked() returns true
  • FindItem()->IsChecked() returns true

context menu, item is unchecked and gets checked:

  • event.IsChecked() returns true
  • FindItem()->IsChecked() returns false

So there are two things wrong here:
The event in the context menu is useless because even.IsChecked always returns true no matter what the actual item state is.
Depending on whether the event handler is called from the app menu or the context menu FindItem()->IsChecked() will either return the new state or the previous state of the item.

Basically there is currently no way to write correct code.

Note that this is a regression bug as it used to work with 2.8.10.

Attachments (2)

wxTest.cpp download (2.0 KB) - added by BuschnicK 11 years ago.
minimal sample demonstrating the problem
menu.cpp.patch download (554 bytes) - added by oc_sielenkemper 10 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 follow-up: Changed 11 years ago by vadz

  • Priority changed from high to normal
  • Status changed from new to infoneeded_new

I don't see this in the menu sample, event.IsChecked() there seems to work correctly. Please provide a patch to it allowing to reproduce the problem.

TIA!

P.S. Please let us decide about the priority of the bugs, thank you.

comment:2 in reply to: ↑ 1 ; follow-up: Changed 11 years ago by BuschnicK

  • Status changed from infoneeded_new to new

I don't see this in the menu sample, event.IsChecked() there seems to work correctly. Please provide a patch to it allowing to reproduce the problem.

Actually no. It works in the menu, but the context menu check item is broken as described above.

This is the log from the menu sample:

A menu has been opened.
A popup menu has been closed.
Menu command 2002 (the item is currently not checked)
A menu has been opened.
A popup menu has been closed.
Menu command 2002 (the item is currently not checked)
A menu has been opened.
A popup menu has been closed.
Menu command 2002 (the item is currently not checked)
A menu has been opened.
A popup menu has been closed.
Menu command 2002 (the item is currently not checked)
A menu has been opened.
A popup menu has been closed.

As you can see it represents an impossible sequence.

P.S. Please let us decide about the priority of the bugs, thank you.

Will do, sorry. Since I could change it I thought I was allowed to.

comment:3 in reply to: ↑ 2 Changed 11 years ago by vadz

  • Status changed from new to infoneeded_new

Replying to BuschnicK:

I don't see this in the menu sample, event.IsChecked() there seems to work correctly. Please provide a patch to it allowing to reproduce the problem.

Actually no. It works in the menu, but the context menu check item is broken as described above.

I don't see why...

As you can see it represents an impossible sequence.

Have you missed the line

        menu.Check(Menu_Popup_ToBeChecked, true);

in the sample? It checks the item before showing the menu, this is why it's always checked initially. Remove this line and you will see that the item is unchecked initially and becomes checked when you select it.

Of course, the state of the item is not preserved as a new menu is created every time.

P.S. Please let us decide about the priority of the bugs, thank you.

Will do, sorry. Since I could change it I thought I was allowed to.

Well, it may be used to select lower severity too.

Changed 11 years ago by BuschnicK

minimal sample demonstrating the problem

comment:4 Changed 11 years ago by BuschnicK

  • Status changed from infoneeded_new to new

Thank you for looking into it Vadim. Please see the attached file for a minimal sample which shows the problem I'm having. Note that the exact same code used to work with 2.8.10. So even if my usage may be wrong wx used to be able to handle it just fine.

comment:5 Changed 11 years ago by vadz

Thanks for the sample but it would be really great if you could reproduce the problem in the menu sample though. I don't understand what is the code supposed to do at the first reading so it will need time to just understand what is the expected behaviour and then understand why doesn't it behave like this.

comment:6 follow-up: Changed 11 years ago by BuschnicK

There are only ~5 relevant lines in that code snippet - the rest is the canonical wx hello world.

I create a menu bar with a single menu and a single check menu item.
I create a popup menu with a single check item which has the same id (!) as the item in the main menu.
The menu event handler just pops up a dialog showing the checked state as queried from the event and from the main menu item.

Please at least take a look at the code. It is far simpler than the menu sample.

comment:7 in reply to: ↑ 6 Changed 11 years ago by vadz

  • Resolution set to worksforme
  • Status changed from new to closed

Replying to BuschnicK:

There are only ~5 relevant lines in that code snippet - the rest is the canonical wx hello world.

Yes, and this is exactly why we appreciate so much receiving patches to the "hello world" (a.k.a. minimal sample) instead of whole files...

I create a menu bar with a single menu and a single check menu item.
I create a popup menu with a single check item which has the same id (!) as the item in the main menu.

You seem to be under mistaken impression that just because the menu item has the same id it will be updated in the main menu bar when it is checked in the popup menu. This doesn't work like this, of course.

The menu event handler just pops up a dialog showing the checked state as queried from the event and from the main menu item.

Please at least take a look at the code. It is far simpler than the menu sample.

I ran the code and I see no problems in it except for the leak of the popup menu which you allocate on the heap without ever freeing. Other than that everything works just fine for me, please expect how do you expect it to work in concrete steps, e.g.

  1. Run the app
  2. Check the item in the main menu
  3. Show the popup menu
  4. Expected: something, but something else happens

Changed 10 years ago by oc_sielenkemper

comment:8 Changed 10 years ago by oc_sielenkemper

  • Patch set
  • Resolution worksforme deleted
  • Status changed from closed to reopened

Hi,

the project BuschniK was working on has moved over to me. Browsing the code I found a link to this ticket. I did some debugging and here is what I found:

Both cases toggle the checked state of the menu item, but on different code paths. For the frame menu it happens in bool wxFrameBase::ProcessCommand(wxMenuItem*) like this:

if (item->IsCheckable())
{
    item->Toggle();

    // use the new value
    commandEvent.SetInt(item->IsChecked());
}

The context menu does it in bool wxMenu::MSWCommand(WXUINT, WXWORD) like this

// update the check item when it's clicked
wxMenuItem * const item = FindItem(id);
if ( item && item->IsCheckable() )
    item->Toggle();

// get the status of the menu item: note that it has been just changed
// by Toggle() above so here we already get the new state of the item
UINT menuState = ::GetMenuState(GetHmenu(), id, MF_BYCOMMAND);
SendEvent(id, menuState & MF_CHECKED);

The event object is created in SendEvent with the second argument passed on to SetInt as above. This leads to a minor difference in the command event object: the frame menu sets m_commandInt to 0 or 1, the context menu to 0 or 8.

But the cause of my problem is a different one: one id passed to wxMenu::MSWCommand as WXWORD is 0x8343 which is treated as a signed short and assigned to an int (the id in the fragment). Hence the value of id is 0xffff8343.

This value is then passed to ::GetMenuState which complains about an unknown id by returning -1 (0xffffffff) and causing the command event to always return true when asked IsChecked().

I was able to fix this by replacing the

         // get the status of the menu item: note that it has been just changed
         // by Toggle() above so here we already get the new state of the item
         UINT menuState = ::GetMenuState(GetHmenu(), id, MF_BYCOMMAND);
         SendEvent(id, menuState & MF_CHECKED);
     }

with

         // get the status of the menu item: note that it has been just changed
         // by Toggle() above so here we already get the new state of the item
         UINT menuState = item ? ::GetMenuState(GetHmenu(), item->GetMSWId(), MF_BYCOMMAND) : 0;
         SendEvent(id, menuState & MF_CHECKED);
     }

in src\msw\menu.cpp, line 859.

The code is still the same in version 2.9.1 (at line 973) and version 2.9.2 (at line 934).

comment:9 Changed 10 years ago by vadz

  • Milestone set to 2.9.3
  • Status changed from reopened to confirmed

Thanks for the analysis of this bug, I do see the problem now. However it looks like the simplest fix would be to

  • src/msw/menu.cpp

    diff --git a/src/msw/menu.cpp b/src/msw/menu.cpp
    index fe8b4a5..543ed55 100644
    a b bool wxMenu::MSWCommand(WXUINT WXUNUSED(param), WXWORD id_) 
    956956
    957957        // get the status of the menu item: note that it has been just changed
    958958        // by Toggle() above so here we already get the new state of the item
    959         UINT menuState = ::GetMenuState(GetHmenu(), id, MF_BYCOMMAND);
     959        //
     960        // Also notice that we must pass unsigned id_ and not sign-extended id
     961        // to ::GetMenuState() as this is what it expects.
     962        UINT menuState = ::GetMenuState(GetHmenu(), id_, MF_BYCOMMAND);
    960963        SendEvent(id, menuState & MF_CHECKED);
    961964    }
    962965

Or am I missing something? Could you please check if this fix works for you?

I'll also fix the 1 vs 8 inconsistency separately, thanks for reporting it as well.

Ideal, of course, would be to have only one place for generating this event, for both normal and popup menus, but unfortunately I don't have time to do this refactoring now. Any patches doing this would be definitely welcome though.

Thanks again!

comment:10 Changed 10 years ago by oc_sielenkemper

Wow, that was a quick response! Thanks!

And you're right, your fix is even better (not just one line but one char!) and it works for me.

I wholeheartedly agree with your "call for refactoring" but I too don't have the time to do it now, sorry.

Best regards,
Marvin

comment:11 Changed 10 years ago by VZ

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

(In [69099]) Fix checked state for the popup menu items in the events generated by them.

We incorrectly passed the sign-extended int id to ::GetMenuState() function
that expects an unsigned WORD id, so it never found the item if the WORD id
had the high bit set. Fix this by correctly passing the unsigned id to it.

Closes #11644.

comment:12 Changed 10 years ago by VZ

(In [69100]) Fix int field of wxCommandEvent generated by popup menu items in wxMSW.

The intention of the code generating the event for popup menu items was to
pass false (0) or true (1) in the int field of wxCommandEvent to indicate
whether the item was checked or not but, because wxMenu::SendEvent() takes int
as second argument and not book, we passed either 0 or MF_CHECKED (== 8).

Fix this by correctly passing a boolean for checkable items.

See #11644.

Note: See TracTickets for help on using tickets.