Skip to content
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 label update for dict on item deletion. #95364

Conversation

ajreckof
Copy link
Member

Fixes #95348
it was effectively a missing update of the labels. So simply added the update that was missing on deletion.

@kleonc
Copy link
Member

kleonc commented Aug 10, 2024

From #95348:

Steps to reproduce

  1. Open the MRP. Alternatively, export a dictionary and add arbitrary values to it (tested with int, string keys).
  2. Open test.tscn in the editor. Look at the exported dictionary.
  3. Delete any value from the dictionary other than the bottom-most one.
  4. Add a new value to the dictionary other than the one that was just deleted.

It's still broken after the last step, just differently (new key shows as null):

setup removed "B" keyed entry entered key/value added new entry
godot windows editor dev x86_64_EvKpnqGIKF RFr4hrMSOO 4rAEt6hPzD eKi6q99HxI

@ajreckof ajreckof force-pushed the fix-label-update-for-dict-on-item-deletion branch from 0646438 to 6577a39 Compare August 10, 2024 21:25
@ajreckof
Copy link
Member Author

Thanks I didn't see the second part of the issue which was another place where an update was missing( ie adding an element). it wasn't happening on first addition because it was creating the editor property hence generating then. There was also another way to trigger the same one. Adding the 22nd item with the same value type as the 2nd would show the 2nd key instead of 22nd.

@akien-mga akien-mga requested a review from kleonc August 12, 2024 15:18
Copy link
Member

@kleonc kleonc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested a little, couldn't make it break / seems to work.

Looking at the code I'm not fully sure what/when updates though, e.g. why object->set_dict(dict) (added for adding/removing key-value pair) is not needed when changing value type of an existing key-value pair. So yeah, TIWAGOS. 🙃

@ajreckof
Copy link
Member Author

ajreckof commented Aug 12, 2024

To give a bit more insight on what happens the function update_prop_or_index is the one doing the update. Setting the dict is just a way to have the dict already set to the object as the name is taken from the object. The dict would have been set there anyway by the emit_changed which call update_property which set the dict.
Now that I'm writing I'm wondering if I should just call_deferred on each of them. It might be a bit more call but will account for people modifying the dict in setter/getter. I'll try it in an hour or so
Edit: this can be merged as is and improvement I just thought of can be merged later as it is way less likely to appear.

@akien-mga akien-mga merged commit e231d04 into godotengine:master Aug 12, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks!

@ajreckof
Copy link
Member Author

To give a bit more insight on what happens the function update_prop_or_index is the one doing the update. Setting the dict is just a way to have the dict already set to the object as the name is taken from the object. The dict would have been set there anyway by the emit_changed which call update_property which set the dict. Now that I'm writing I'm wondering if I should just call_deferred on each of them. It might be a bit more call but will account for people modifying the dict in setter/getter. I'll try it in an hour or so Edit: this can be merged as is and improvement I just thought of can be merged later as it is way less likely to appear.

Forgot to update on this making the function use call deferred would be hard as it is a function on a slot which is a struct so not possible would need to create a function that would update the labels and call deferred on this one. Anyway I remembered I had seen another update problem with modifying values on setter/getter so I'll recheck everything and post a full bug report on what parts don't work and go from there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inspector displays incorrect keys in exported dictionaries after deletion of entries
3 participants