-
-
Notifications
You must be signed in to change notification settings - Fork 22k
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
Fix nested array editor not being updated on type change. #87140
Fix nested array editor not being updated on type change. #87140
Conversation
@@ -230,8 +230,7 @@ void EditorPropertyArray::_change_type_menu(int p_index) { | |||
Variant array = object->get_array().duplicate(); | |||
array.set(changing_type_index, value); | |||
|
|||
emit_changed(get_edited_property(), array, "", true); | |||
update_property(); | |||
emit_changed(get_edited_property(), array); |
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.
This changes a relevant part of the original code, was the removal of the arguments intentional? It's not the same as the rest, unsure if the true
for p_changing
is relevant here
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.
This is the main fix the argument here was preventing the updates from being fired from the root Inspector which was the main reasons of the bug here. The rest is mainly preventing multiple updates and removing default arguments from function call ("" and false are the default values). I just realized with your comment that there was only here that it was to true and that therefore only changing type was not working.
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.
Do you mean the removal of update_property
? Or did you just also remove the default arguments at the same 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.
the removal of update property is to prevent multiple updates.
Removing the default argument ("" and false) does nothing and is just a by-product of how I made my search and replace.
The main fix is to remove the true argument which change the value from true to the default false.
#80706 was not marked as cherrypick (maybe wrongly) and this fix a regression introduced there so either this should not be cherrypicked or the other should too. |
3ef1dcb
to
8115924
Compare
Discovered the double update is needed until the same is done for Dictionnaries as it breaks without it for now when you nest an array under a dictionnary. This because not using the double update rely on the holding |
Superseded by #88231. |
Fix #87139