Add a data argument to menus

[powerlord]

I nearly forgot about this. Basically, this adds a data argument to CreateMenu, CreateMenuEx, and CreateDataMenu. This data argument is passed to MenuHandler and VoteResultCallback.

There was some discussion of whether we wanted to let programmers assign the data variable after the fact, but it is my opinion that it should be created in the Menu constructor / CreateMenu call. Feel free to comment on this if you disagree.

This patch also changes the return type of CreateMenuEx and CreateDataMenu to Menu (it was Handle) as this prevents a warning when you try to assign the return value from one of these to a Menu variable.

This also adds CreateDataMenuEx which combines CreateMenuEx and CreateDataMenu.

Now, some notes as to the design of this: You'll note that the IPluginContext is saved when a menu is created. This is because most Handle types require the owner identity be passed in when you call FreeHandle on them. You can see this behavior in smn_KillTimer when a Timer has the TIMER_DATA_HNDL_CLOSE flag.

The userData != NULL check shouldn't be necessary as we always create a MenuUserData struct, but I left it in just in case.

[dvander]

You're going to hate me for this - but I was crossing my fingers and praying to the heavens we wouldn't take API extensions like this for 1.7. The reason is that passing opaque values is not compatible post-transitional-syntax, since there is no way to track the liveness of the value. To do that, we need an extension to the C++ SourcePawn API, and I was hoping to save that for 1.8 (though I could take a stab at it now).

If the change was local to scripted API, it'd be easier to deal with. But changing the menu C++ API makes it more troublesome, since we guarantee binary compatibility.

[powerlord]

@dvander Unfortunately, I can't really think of a way of doing it that wouldn't touch the C++ Menu API in some way. In this case, it's modifying IMenuStyle's CreateMenu and adding a convenience method to IBaseMenu (which could just as easily be a stock in the .inc), but the alternative was to have Get and Set methods in IBaseMenu. Either way, the menu interface would be changed and the interface number presumably bumped.

This is because the Interface isn't just used by extensions, but also internally by natives.

Not adding this isn't a huge deal... but it was a requested feature on multiple occasions.

On a site note... if we're discouraging opaque values in the 1.7 syntax, what is going to happen with CreateTimer / Timers in general which also pass in opaque values?

[dvander]

An excellent question! Short answer, I don't have concrete plans yet, but basically the any keyword will have to be deprecated. I tried to do that in 1.7 but ran out of time, so it'll have to be for 1.8. The long-term plan would be something like this:

1. Introduce a new type to replace any, called object or box or something. This would be a multi-part value, in C++ looking something like:

   enum ValueType {
       ValueType_Int,
       ValueType_Float,
       ValueType_Handle
   };
   struct Value {
       ValueType type;
       union {
           cell_t intValue;
           float floatValue;
           Handle handleValue;
       };
   };
   

2. Define new coercion rules, such as int -> object, to make sure old primitive types can be safely made opaque. When the compiler tries to pass, say, 3.5 into an object, internally it would construct a ValueBox(ValueType_Float, 3.5) and pass that instead.

3. When a native uses an object type, give it a new signature that can only be bound using a new C++ API. This API would allow consumers to safely hold and consume typed opaque values. Old plugins would stay compatible since they bind to the earlier, untyped version of the native. This API would look something like:

   struct TimerData {
       IPluginFunction *fun;
       Rooted<Value> data;
   };
   static bool Timer_CreateTimer(IPluginContext *cx, Arguments &args, Value *rval) {
       TimerData *data = new TimerData;
       data->fun = ...
       data->data = args[3];
       Handle_t handle = CreateHandle(hTimerType, data);
       *rval = ValueFromHandle(handle);
       return true;
   }
   

4. Update all natives to use this new API.

5. Make Rooted<Value> inc/decref handles that are Values.

6. CloseHandle is now a no-op.

The most important part is step 3. Without the new C++ API we can't do anything, but we can always define it ahead of time and just make it wrap the older functionality.