Adding support for list property types

[bjorn]

Implementation so far:

• Added 'list' as option to the Add Property dialog, for adding a QVariantList.
• Adjusted the CustomPropertiesHelper to create sub-properties that display the values in the list (maybe should rather be done by the VariantPropertyManager).
  • It is not done by VariantPropertyManager because it couldn't delegate class member creation back to the CustomPropertiesHelper. If VariantPropertyManager should handle lists, then it would need to handle classes as well.
• Added support for saving and loading list properties to TMX and JSON formats.

• Added a tool button with menu that allows to add new list items.

ToDo:

• Ability to add items to a list
• Ability to remove items from a list
• Editing list items
• Reordering list items

Resolves #1493

[bjorn]

Urgh, turned out the "Ability to add items to a list" is only mostly working. When just pressing the "+ Add" button instead of using the menu, it is not possible to add items of the same custom enum or class type. This is because the values are at that point converted to their "display value", which means a class is just a map and an enum is just a number, and only the CustomPropertiesHelper knows the associated type.

[bjorn]

Resuming work on this based on the new Properties framework introduced in #4045, I've just rebased the changes. The code for actually displaying the list will have to be re-written, so for now it just displays the amount of items:

[bjorn]

I believe lists are working quite fine now. There's at least the following remaining issues:

• Lists nested in lists don't load correctly yet for the JSON format.
• Object reference properties no longer have the necessary context (the relevant MapDocument) to display object name or trigger the object picker.

[bjorn]

Above issues now resolved, but I've discovered a few more problems:

• Storing "object reference" properties when stored in lists that are members of classes in the JSON format doesn't work correctly, since in this case lists are not going through the necessary conversion in MapToVariantConverter::toVariantMap.
• When saving/loading of custom classes, values in lists are not "resolved" yet and are not taken into account when checking for circular references between types.
• Displaying object reference arrows doesn't work when the references are stored within lists.

[dogboydog]

Had a chance to try this out. Overall, this is really cool. I had a few thoughts.

It could be good to render a preview of some kind instead of just the ID when the type in a list is object
(the name at least and maybe a thumbnail of the tile if any / the shape type if it's a basic object ? )

This i guess is just on all properties from the new properties framework, but the tooltip for the type of the list items is useful here

"Property value" classes used in a list don't re-render when custom types are edited. For example I added a bool to the MyListEntry class, and it wouldn't render on the list until I navigated away from map properties and back to it. Here's a video demonstrating, where the non-list properties on MapClass automatically re-render, but the class instance in the list doesn't update its properties

list.properties.feedback.mov

[bjorn]

It could be good to render a preview of some kind instead of just the ID when the type in a list is object
(the name at least and maybe a thumbnail of the tile if any / the shape type if it's a basic object ? )

Yeah, it used to show the object name and an icon depending on the type of the object, but we lost that functionality because it relied on the previous Properties view having two states: display and edit, whereas we now only have the edit state. Maybe we can somehow display that name and icon when the field is not focused.

Of course that's unrelated to list properties but applies to object reference properties in general, so it can be a separate PR to improve the ObjectRefEdit.

"Property value" classes used in a list don't re-render when custom types are edited.

Thanks for noticing that! Indeed I haven't finished this part of the updating logic, where VariantListProperty::createOrUpdateProperty currently can't check the value of mPropertyTypesChanged because it is stored only in the VariantMapProperty instance. Maybe I can just move such a boolean to the Preferences from which the propertyTypesChanged signal originates, though it feels somewhat hacky.

[bjorn]

Now I think there's one remaining major issue:

• When adding a value to a list in the Custom Types Editor, it doesn't prevent the user from adding a class value that will cause a circular reference.

[dogboydog]

Alright, I gave it another test drive... here are my thoughts

Interacting with the list, it reminds me a lot of the experience of editing an Array in the Godot inspector. I like the UX, particularly adding to the list feels smooth and I like being able to collapse the list items. Since I personally prefer C# in Godot and strong typing, that made me think that there's currently no way to constrain a list to only be a certain type here (e.g. list of strings only). This probably isn't a big deal and shouldn't prevent this from merging but just a thought I have.

Tested opening a map with list properties in a version of Tiled which doesn't support them yet. The list was resaved as type "QVariantList". Then when I opened the map with the list properties version again, it loaded as expected. I wanted to see if the list property would be dropped by the older version due to being an unrecognized type so that we would know whether to warn people not to open their maps with an older Tiled version, but I think this is a pretty good amount of "forward compatibility".

I wonder, would there be a use case for class usage flags for restricting a custom type to be only usable as a list item? The way you can currently say a class can be used for maps but not objects and so forth. Maybe for something like drop table entries that only appear in a list. Probably not a strong need though and not a blocker.

The issue I identified earlier where a class instance used as an item in a list wouldn't re-render when the default properties of the class were edited seems to be fixed. : )

Not strictly related to list properties, but I think I would still recommend creating custom properties when a name has been entered instead of discarding them when focus is lost. Currently it seems like they're even lost when you enter name and change the type dropdown, but just don't hit the return key. It feels really jarring, almost like a bug even though I know it's intended right now to require you to submit.

A class with no properties displays slightly awkwardly. It makes sense, but I wonder if something like placeholder text here like No properties would look a little more polished?

Overall, really nice feature. Trying it got me thinking of how I might be able to make use of this for my games.

[bjorn]

Since I personally prefer C# in Godot and strong typing, that made me think that there's currently no way to constrain a list to only be a certain type here (e.g. list of strings only). This probably isn't a big deal and shouldn't prevent this from merging but just a thought I have.

That's right, this is a desirable feature. However, the type is currently embedded in the value so a list with a type per value was the more natural fit to start with. I was thinking it might be alright if we add an option to class members to set them as arrays, though such an approach isn't without issues either especially for top-level values.

It is definitely worth considering an approach towards this before the 1.12 release, though its implementation may be out of scope for now.

Tested opening a map with list properties in a version of Tiled which doesn't support them yet. The list was resaved as type "QVariantList". Then when I opened the map with the list properties version again, it loaded as expected.

Wow, I didn't expect that to actually work, but it works because for the JSON format the value is converted directly from JSON to a QVariant and when saving from QVariant back into JSON. So as long as you don't touch it, the original JSON structure remains intact.

It's a bit of a happy coincidence which unfortunately doesn't work for the XML format, which rather completely destroys the list values, replacing it with an empty string value. I'll definitely need add a warning in the news post and the docs regarding this.

I wonder, would there be a use case for class usage flags for restricting a custom type to be only usable as a list item?

I would personally think the "Property value" checkbox should suffice for now, but more fine-grained settings could be implemented later if that turns out to be sufficiently useful.

Not strictly related to list properties, but I think I would still recommend creating custom properties when a name has been entered instead of discarding them when focus is lost.

Thanks for bringing this up again! I've implemented that in #4265.

A class with no properties displays slightly awkwardly. It makes sense, but I wonder if something like placeholder text here like No properties would look a little more polished?

Maybe it would suffice to remove the expand interaction for "empty" classes.

In addition I think it might be good to display the class name. I had implemented this as part of the name column and it seemed kind of busy, which is why it's only in the tool tip now, but especially for class values in lists there is such an odd waste of space. We could try showing the class name to the value column, but this should also be a new PR.

Thanks for the thorough feedback!

[eishiya]

Maybe it would suffice to remove the expand interaction for "empty" classes.

I don't think it would. I've seen that in other software and it always feels janky when there isn't an explicit indicator that a list is empty. A row saying "(empty)" when expanded, for example, or "(empty") in the list header.

[bjorn]

I don't think it would. I've seen that in other software and it always feels janky when there isn't an explicit indicator that a list is empty.

Ah, that's a good point regarding lists, but @dogboydog was talking about class values. Since you can't add anything to an empty class (except in the Custom Types Editor) I think hiding the expand arrow there is fine.

For lists I wonder if it would be nice to show the "Add Value" thing into the list as the last element, since that's where it will actually add the value. That does mean you'd need to scroll to the bottom of the list to click it, but that might be better than the current behavior, where you need to scroll back to the top to add another element.

[eishiya]

Ah, whoops.

+1 for having the add value widget at the bottom. Either way it's going to be an issue with long lists though, and a nice bit of polish would be to make it easy to add a new value even when a list is unwieldy. Perhaps a context menu "Add value" option that scrolls you to that widget if it's not visible and focuses it?