Expose PopupMenu get_item_multistate() and set/get_item_multistate_max()

[mrcdk]

I noticed that while you can set and toggle a PopupMenu multistate item with PopupMenu.set_item_multistate() and PopupMenu.toggle_item_multistate() there was no way to get the state of the item. I also noticed that there was no way to get or set the max_states so I've exposed them to the scripting languages.

• Bugsquad edit, closes: Be able to access the PopupMenu item's state and get and set the item's max_state godot-proposals#8918

[AThousandShips]

Please open a proposal to track the support and details of this feature 🙂

[mrcdk]

Please open a proposal to track the support and details of this feature 🙂

Opened here godotengine/godot-proposals#8918

[bruvzg]

Most likely these items were not exposed since it's not entirely obvious how to use them. Unlike checked property, multistate is not auto updated or validated on menu item clicks, so you need to handle it manually. It's OK to expose them, but a usage example probably should be added to the documentation.

[mrcdk]

Most likely these items were not exposed since it's not entirely obvious how to use them. Unlike checked property, multistate is not auto updated or validated on menu item clicks, so you need to handle it manually. It's OK to expose them, but a usage example probably should be added to the documentation.

I've updated the PR with an example usage

[YuriSizov]

If existing exposed methods have multistate in their name, these new ones should too. Specifically when you consider the pair set_item_multistate/get_item_state. I don't see the underlying properties used outside of this multistate functionality, so I don't think there is a reason to use a disassociated name.

The max state methods could be called set_item_multistate_max/get_item_multistate_max, but they are probably fine as is. They also currently match the global menu API. So up to you.

[mrcdk]

If existing exposed methods have multistate in their name, these new ones should too. Specifically when you consider the pair set_item_multistate/get_item_state. I don't see the underlying properties used outside of this multistate functionality, so I don't think there is a reason to use a disassociated name.

The max state methods could be called set_item_multistate_max/get_item_multistate_max, but they are probably fine as is. They also currently match the global menu API. So up to you.

I thought about that but internally in the engine the PopupMenu API was using get_item_state() and there's also DisplayServer.global_menu_get_item_state() so I kept it like that. Technically any item can be a multistate item by setting the max_states variable with set_item_max_states() (added in this PR). DisplayServer's global_menu exposes them like DisplayServer.global_menu_get_item_max_states() and DisplayServer.global_menu_set_item_max_states()

I would personally rename set_item_multistate() to set_item_state() like what global_menu does (DisplayServer.global_menu_set_item_state()) and toggle_item_multistate() to advance_item_state() or something similar but that would break compatibility. Maybe we could deprecate those functions and rename them?

[YuriSizov]

@mrcdk The internal name doesn't really matter as it wasn't added with the public name in mind, clearly. Us exposing it now is the perfect time to consider the name. This is also a very minor reason to break compatibility, so I would rather we named one new method to match the existing one.

Matching DisplayServer exactly would be nice, but not critical.

[mrcdk]

Okay! Done! Renamed:

• get_item_state() to get_item_multistate()
• get_item_max_states() to get_item_multistate_max()
• set_item_max_states() to set_item_multistate_max()

[AThousandShips]

Suggested change

	[b]Note:[/b] Multistate items don't update their state automatically and must be done manually. See [method toggle_item_multistate], [method set_item_multistate] and [method get_item_multistate] for more info on how to control it.
	[b]Note:[/b] Multistate items don't update their state automatically and must be done manually. See [method toggle_item_multistate], [method set_item_multistate], and [method get_item_multistate] for more info on how to control it.

Please use Oxford comma.

[AThousandShips]

Well, this can be resolved later 🙃

[YuriSizov]

You were too late :P

[AThousandShips]

I think 11 minutes should be enough, I'll endeavour to be faster next time :þ

[YuriSizov]

You'd think so, but between me checking out this PR and queuing it up, then 20 others, then building it all to test everything is superficially okay, and then merging — it's nothing 😄

[YuriSizov]

Thanks!