Opened 8 years ago

Closed 8 years ago

#13138 closed bug (fixed)

BInputServerMethod::SetMenu doesn't work

Reported by: jalopeura Owned by: nobody
Priority: normal Milestone: Unscheduled
Component: Servers/input_server Version: R1/Development
Keywords: Cc:
Blocked By: Blocking:
Platform: All

Description

BInputServerMethod::SetMenu doesn't actually add a submenu to the item in the input server menu. The problem seems to be in _BMethodAddOn_::SetMenu:

if (menu)

menu->Archive(&menuMsg);

should be

if (menu)

menu->Archive(&menuMsg, true);

Without that second argument, the menu items are not included.

(I have been unable to test this, since PM won't let me simply overwrite the input server with my newly compiled version; I'm going to have to build an entire system and install it to a partition.

I will attach a patch, but if nobody else gets to this, I will try to find time to build an entire installation to test this.

Attachments (1)

input_server_menu.patch (1.8 KB ) - added by jalopeura 8 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 by jalopeura, 8 years ago

patch: 01

comment:2 by jalopeura, 8 years ago

mmu_man and jessicah introduced me to the ways of blacklisting, and I was able to test this. My original patch was not the proper solution (that parameter defaults to true, so it actually didn't change the behavior at all).

However, I have uploaded a second patch that does work. You can test it with the t9 input method add-on.

comment:3 by jessicah, 8 years ago

Can you please use static_cast<BMenu*> instead of a C-style cast?

comment:4 by axeld, 8 years ago

I don't mind a C-style (we pretty much use it everywhere), but there are a few minor coding style issues:

  • "msg" is not a good name, especially, when there already is a "message". A more descriptive name that gives you an idea of what it is used for would be appreciated.
  • Also, we're using boolean expressions in if-clauses so it's fMenu != NULL and not just fMenu.

Also, is there any reason why BMenu::Instantiate() does not return a BMenu?

by jalopeura, 8 years ago

Attachment: input_server_menu.patch added

comment:5 by jalopeura, 8 years ago

1) Sorry if any of you are following this ticket and just got a bunch of update emails; it's 5am here and I kept making stupid mistakes - like uploading the wrong file or adding a static_cast to a piece of code I had not previously changed, etc, and then not realizing my mistake until I looked at the color-coded diff the ticket page shows.

2) The chunk of code in MethodReplicant::AddMethod was actually copied almost entirely unchanged from MethodReplicant::UpdateMethodMenu, so the coding and naming issues came from there, and in _BMethodAddOn_::SetMenu I kept the same style. I have changed them in my new code, but not in their original location.

3) All versions of<whatever>::Instantiate are required to return a BArchivable. I believe this is so the code in Archivable.cpp can build a mangled function name in order to load the proper symbol.

in reply to:  5 ; comment:6 by axeld, 8 years ago

Replying to jalopeura:

3) All versions of<whatever>::Instantiate are required to return a BArchivable. I believe this is so the code in Archivable.cpp can build a mangled function name in order to load the proper symbol.

The return type is not part of the function signature.

in reply to:  6 comment:7 by jalopeura, 8 years ago

Replying to axeld:

The return type is not part of the function signature.

The Be Book says

Instantiate() must return a BArchivable*, regardless of the actual class in which it's implemented.

If it's not for mangling...ah, it's because it's a virtual method inherited from BArchivable, so the vtable wouldn't find it if it didn't have the right return type.

in reply to:  4 comment:8 by jessicah, 8 years ago

Replying to axeld:

I don't mind a C-style (we pretty much use it everywhere), but there are a few minor coding style issues:

C-style casts are a coding style issue :p

"Use C++ casts (dynamic_cast, static_cast, const_cast, reinterpret_cast) over C casts."

Although, perhaps one should be using dynamic_cast here; reading up, I don't think can use static_cast with virtual inheritance.

Last edited 8 years ago by jessicah (previous) (diff)

comment:9 by axeld, 8 years ago

I plead guilty, however, it would still be nice to get away with that cast completely.

comment:10 by pulkomandy, 8 years ago

Resolution: fixed
Status: newclosed

Applied in hrev50868.

Note: See TracTickets for help on using tickets.