-
-
Notifications
You must be signed in to change notification settings - Fork 21.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Expose PopupMenu
get_item_multistate()
and set/get_item_multistate_max()
#87395
Conversation
ef4a4d6
to
e04ef84
Compare
Please open a proposal to track the support and details of this feature 🙂 |
Opened here godotengine/godot-proposals#8918 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
e04ef84
to
534e627
Compare
I've updated the PR with an example usage |
534e627
to
78fd36d
Compare
78fd36d
to
724fa26
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 I would personally rename |
@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 |
… set_item_multistate_max()
724fa26
to
c9bdccf
Compare
Okay! Done! Renamed:
|
PopupMenu
get_item_multistate()
and set/get_item_multistate_max()
An [param id] can optionally be provided, as well as an accelerator ([param accel]). If no [param id] is provided, one will be created from the index. If no [param accel] is provided, then the default value of 0 (corresponding to [constant @GlobalScope.KEY_NONE]) will be assigned to the item (which means it won't have any accelerator). See [method get_item_accelerator] for more info on accelerators. | ||
[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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, this can be resolved later 🙃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You were too late :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think 11 minutes should be enough, I'll endeavour to be faster next time :þ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😄
Thanks! |
I noticed that while you can set and toggle a
PopupMenu
multistate item withPopupMenu.set_item_multistate()
andPopupMenu.toggle_item_multistate()
there was no way to get thestate
of the item. I also noticed that there was no way to get or set themax_states
so I've exposed them to the scripting languages.