Extend menu handler & item callbacks (bug 6002)

[Amaroq7]

This feature was requested by connorr here.

We can directly get item info like access, info etc. without calling menu_item_getinfo.

[Javivi]

I would also mark menu_item_getinfo as deprecated so people become aware of this feature.

[Amaroq7]

I thought about the same but if we did that there won't be any way to get item's info outside of menu handler/item callback.

[Javivi]

I don't know if it is possible, but I would add a warning message to the deprecated tag saying : "Usage of menu_item_getinfo is deprecated inside menu handlers and menu callbacks, ignore this if you're not using it in that way".

I don't know what would be worse, if having to ignore a warning every time the function is used outside a handler/callback (few cases for doing so), or having people use outdated code from other plugins or old guides because they're not aware of the changes.

[Nextra]

I definitely say no to the deprecation. It is neither a broken nor an obsolete native, and the deprecation warning would be awkward in addition to being technically wrong.

As a general comment I do think that this PR makes the callback prototype a bit too crazy, for not too much benefit. I'd probably choose pass at most the info string if I felt like changing this at all, and I think it even would be fine to stay as-is.

Should we decide put all that stuff in there the info string should probably come first so that the other parameters (which are rarely used in my experience) can be omitted.

[Amaroq7]

I agree it's a little bit messy but the point of this PR is to not need call menu_item_getinfo inside those callbacks. Move info at first sounds logical because it's probably most used parameter. What about this prototype function(id, menu, item, const info[], const name[], access, callback)?

[Javivi]

I doubt menu_item_getinfo gets any use outside the menu handlers, and there would be no point using it if the function definition gets updated to include the new parameters. How does this not leave menu_item_getinfo obsolete ?

Also I would not like the parameter order to be different across the function definition and menu_item_getinfo, you can always do the _ _ dance to only get the parameters that you need.

[Amaroq7]

After some thinking I come with other idea that will simplify the callback a bit and give us ability to pass different types of data as item info not just string. Since AMXX introduced datapacks, so we can take advantage of them. I've already done some work on it.

New natives:

• menu_additem2
  menu_additem2(menu, const name[], DataPack:data = Invalid_DataPack, paccess = 0, callback = -1)
• menu_item_setdata
  menu_item_setdata(menu, item, DataPack:data) (old datapack will be automatically freed if any) We can use ResetPack native with clear set to true.

Callback prototype: function(id, menu, item, DataPack:data)
Each datapack passed as item's info will be automatically freed when menu is destroyed.
Scripter must take care of resetting pack position in handler and callback. (or should AMXX do it instead?)

Deprecations:
Should menu_additem & menu_item_setcmd be marked as deprecated along with menu_item_getinfo or stay as is? I marked them as deprecated for now.

[rsKliPPy]

menu_additem shouldn't get deprecated. It's not broken, it's just that the new API will make for simpler and cleaner code.

In menu_additem2 third param shouldn't strictly be DataPack:, but any: instead. Let users pass whatever handle they want instead of having them enclose other handles into DataPacks. They would probably just re-tag as it doesn't change anything in this case which leads to unnecessary tags and dirtier code.
int menuitem::datapack should be renamed to reflect that.

You shouldn't have removed/added whitespaces to newmenus.inc to stylize code nor fixed typos and left it for another PR as it bloats the diff making it harder to review changes. Let it slide now anyway, but keep that in mind.

menu_item_getinfo is still useful if you want to get item name, access and callback, no? Also if old menu_additem isn't deprecated (and it should't be), then this shouldn't be either.

I do fail to find any use case for menu_item_setcmd(), but it could've been useful for static menus, maybe. It's to be discussed if we want to leave that and add menu_item_setdata (or menu_item_setcmd2) or deprecate it.

[rsKliPPy]

There's an unnecessary unused value left over.

[Amaroq7]

Oops, I must have missed that.

[Amaroq7]

Summary

• Removed deprecation for menu_additem & menu_item_getinfo & menu_item_setcmd natives
• Changed data tag parameter in menu_additem2 from DataPack: to any:
• Added menu_item_setdata native, since we have menu_item_setcmd, i don't know why we shouldn't have replacement for it, also as it was stated above it could be useful for static menus

Both of them have special parameter named dp_data, so AMXX will know if data passed by scripter was a datapack handler and then it can take care of freeing it at the end (destroying the menu).

[Arkshine]

It's definitively much better and saner than what you proposed initially. 👍 (did not review yet)

I'm not sure to like the menu_additem2 name, _ex would probably better but since others natives with "2" have been added to this API, I guess it's okay.

[Arkshine]

Looks like you need to rebase.

[Amaroq7]

Done

[gtteamamxx]

What do you think about merging this into master?
I think it is very userful

[gtteamamxx]

Ok, now we can move one with this.
Waiting for merge ;>

[vinaghost]

lol 3 years later, what happen with this pr ?

[kidd0x]

still nothing :(