Magnify app crashes

[atmartens]

I was playing with the magnifying application and it crashed on me after a right-click.

Here is a backtrace:

#0  0x0020b204 in TMagnify::MouseDown ()
#1  0x0036c2d0 in BWindow::DispatchMessage () from /boot/system/lib/libbe.so
#2  0x00368ad3 in BWindow::task_looper () from /boot/system/lib/libbe.so
#3  0x002ba904 in BLooper::_task0_ () from /boot/system/lib/libbe.so
#4  0x0062f08d in thread_entry () from /boot/system/lib/libroot.so
#5  0x78074fec in ?? ()

[atmartens]

The only problem is I can't seem to reproduce the bug. I'll keep at it. :-)

[stimut]

I was able to reproduce it: select a separator from the pop-up menu.

This is caused by the BPopupMenu::Go() function returning non-NULL when an item is selected even if the item has no BMessage associated with it (i.e. the item does nothing - such as for a separator).

I have created a simple patch so that it now returns NULL when the item selected has no message associated with it.

[stippi]

Nice work! You definitely identified the problem and correct location for the fix, only your fix is incorrect. BMenu should not care whether the BMenuItem has a BMessage or not, but I don't suppose it should return BSeparatorMenuItem instances. This should actually be checked against the BeOS implementation. I still have a ZETA installation which I can test on, but in general, I think one should return NULL in case dynamic_casting to BSeparatorMenuItem is successful.

[jackburton]

Replying to stippi:

Nice work! You definitely identified the problem and correct location for the fix, only your fix is incorrect. BMenu should not care whether the BMenuItem has a BMessage or not, but I don't suppose it should return BSeparatorMenuItem instances. This should actually be checked against the BeOS implementation. I still have a ZETA installation which I can test on, but in general, I think one should return NULL in case dynamic_casting to BSeparatorMenuItem is successful.

I seem to recall something like this having been fixed in the past: IOW: this chould be a regression.

[jackburton]

Check hrev17026.

[stippi]

Ah, that makes sense. The correct fix is thus "if (item->Frame().Contains(where) && item->IsEnabled())".

[stimut]

Replying to stippi:

Ah, that makes sense. The correct fix is thus "if (item->Frame().Contains(where) && item->IsEnabled())".

Yes, I agree. I have updated the patch in case someone wants to commit the changes.

[stimut]

Updated fix

[stippi]

Thanks a lot! Applied in hrev35248.

[jscipione]

This fix for this bug was not right because it caused #6610

[jscipione]

Fixed in hrev45977