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

Implement DictionaryPropertyEdit . #12015

Closed
wants to merge 1 commit into from

Conversation

jagt
Copy link
Contributor

@jagt jagt commented Oct 11, 2017

This PR implements DictionaryPropertyEdit for editting exported dictionary properties (#10018).
Here's a screenshot:

image

However there're some usability issues which I really need some help from you guys:

  1. Changing the size field won't work atm as I don't see a way for this to work for a dictionary. So currently there's no way to add/remove new items through the inspector. I think the most common way to handle this is to add a button/widget to add new items, but I don't find a way to add button in inspector/.
  2. Can't handle duplicated keys. If one change a key to be equal to a existing key, the dictionary will actually shrink the dictionary by one. I don't really know how to handle this through the UI. Should duplicate key changes be ignored or report a error? Or I should simply make keys readonly.
  3. Changing keys will change item order. Maybe this can be fixed by adding data to track item orders.

Most of the problems are related to key editing. Your input on this will be really helpful.

@27thLiz 27thLiz added this to the 3.0 milestone Oct 11, 2017
@akien-mga
Copy link
Member

There's a slight style issue with the ordering of the includes (it should be alphabetical), could you amend the commit to fix it? https://travis-ci.org/godotengine/godot/jobs/286360431

@Geequlim
Copy link
Contributor

I don't think it is a good idea to show 4 rows in the inspector for each element of a dictionary.

@djrm
Copy link
Contributor

djrm commented Oct 12, 2017

that interface seems very complicated for a simple task, maybe it would be better to wait until the inspector gets rewritten.

@jagt
Copy link
Contributor Author

jagt commented Oct 13, 2017

IMO I can't find a way to implement a usable Dictionary inspector atm. If there's plan to rewrite inspector then maybe this should be postponed after that is done.
Is there any discussion about the rewrite? Thanks!

@djrm
Copy link
Contributor

djrm commented Oct 14, 2017

@jagt the rewrite is planned for 3.1, so it might be better to wait after the release of 3 to discuss this

@Geequlim Geequlim mentioned this pull request Oct 14, 2017
6 tasks
@jagt
Copy link
Contributor Author

jagt commented Oct 15, 2017

@djrm ok. I'm closing this for now.

@jagt jagt closed this Oct 15, 2017
@bojidar-bg
Copy link
Contributor

Reopening as per discussion with reduz on IRC (click to expand)
‎‎<‎bojidar_bg‎>‎ … -- the PR got closed as the interface was "too complex"
‎‎<‎bojidar_bg‎>‎ and set to wait for inspector redesigning in 3.1
‎‎<‎reduz‎>‎ it should still work
‎‎<‎reduz‎>‎ actually
‎‎<‎reduz‎>‎ but most of the points raised are valid
‎‎<‎bojidar_bg‎>‎ I guess we might get it reopened and merged as a workaround
‎‎<‎bojidar_bg‎>‎ and fix it up later on, maybe even for a 3.0.1, in case there is a way
‎‎<‎reduz‎>‎ the order issue is solved by #12656 karroffel just PRd
‎‎<‎reduz‎>‎ i would also not expose size, simply in the type enum, an extra option "Delete" could be added
‎‎<‎reduz‎>‎ for duplicate keys, that's just life
‎‎<‎bojidar_bg‎>‎ sounds good
‎‎<‎reduz‎>‎ the only use case i'm not completely sure is adding new keys
‎‎<‎bojidar_bg‎>‎ we might rename the duplicate key, similar to how duplicate node names are renamed
‎‎<‎bojidar_bg‎>‎ reduz: there might be an empty entry at the end of the dictionary
‎‎<‎reduz‎>‎ bojidar_bg: oh that makes sense

The main points from the discussion are:

  • Order is fixed by make Dictionary use OrderedHashMap #12656 (which is likely to merged soon-ish).
  • Size should be replaced by a "Delete" type of keys and/or value (when selected, removes the key/value from the dictionary).
  • To add new key/value pairs, there should be a key with "Delete" type below all the others.
  • Duplicate keys might (or might not) be renamed similar to how nodes are currently renamed (if strings)

@Geequlim
Copy link
Contributor

I did the same thing in #11940 it is designed to display remote dictionaries so it doesn't implement too much features as @bojidar-bg said.

@akien-mga
Copy link
Member

Superseded by #11940 which we are soon going to merge, but thanks a lot for the proposal, it helped with the design of how we want it to be.

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.

6 participants