mapeditor
New properties framework
I've started implementing a new properties framework in an attempt to improve some existing usability issues as well as to make it easier to add support for list properties (#4002).
This is a work-in-progress! It its current state the Properties view can only edit a few built-in map properties.
I'm sharing this here for the curious and to gather feedback.
Here's a preview of what it looked like at some point, when it wasn't actually functional but had all Map properties represented:
And here's a video of the current state, with only three properties, but they are fully implemented:
https://github.com/user-attachments/assets/54b037b1-8fd5-4f82-aa2d-201d18da161b
Main advantages are currently:
- The widgets can be directly interacted with.
- They can respond to layout (W/H presented horizontally or vertically based on width of view).
- Easy to add buttons like "Resize Map" that are not hidden.
- Huge reduction in complexity / code size compared to existing properties framework (but still lacking features, like nested properties, selection, etc.).
I got a chance to try this out and look at the code. I like it. It's nice to have the resize map button more accessible on the map properties.
As far as the code, I'm definitely less equipped than you to judge it but I had a few thoughts.
At first I wondered if there might be a way to define these editable properties and the widgets that would be produced on the edited objects themselves (worlds, maps, etc.) but I guess that's really only used in this property editor component, so there's no need.
The properties have to take the editor factory in as a constructor argument, and currently you are creating a subclass for each type of property. I'm wondering about if there's a way to make it so that only the PropertiesWidget would even need to know about the factory. I feel like the factory could just be a way to generate editor widgets for well-known types of properties that don't need customization. Or maybe the factory just becomes a parameter to addProperty() instead and you only need to provide one if you need to customize the produced widget.
Also, it might be nice if there was a lighter syntax for adding a property that didn't entail creating a class for each property. Part of what makes me think this is the current need to store the editor factory in each property which creates a little more boilerplate for each property, while classes that customize the widget like MapSizeProperty don't actually need the factory at all from what I can see. Being able to just pass in lambdas or something for getting/setting the value to addProperty() could be nice. I would think there would be a lot of properties where you would just be calling existing getters and setters, but maybe I'm wrong.
Feel free to disregard any of this, though, just some initial thoughts that may be misinformed
@dogboydog Thanks for writing down your thoughts on the new properties implementation! They've been the exact things I'm also wondering about / struggling with. There may not be one simple solution for everything, but indeed this "factory" argument is quite annoying, especially when passing in that "default factory" most of the time anyway.
I've thought about creating the properties through the factory instead. So far there are ValueTypeEditorFactory::createProperty (to create a ValueProperty) and createQObjectProperty. But for now both have remained unused because they didn't quite fit either:
-
The
ValuePropertykeeps a local value, which I thought would be a good way to avoid subclassing. Of course, then signals need to be connected to keep that value updated and respond to changes. I still need to try this approach. -
The
QObjectPropertycould be even more automatic, since it wraps Qt's meta object system to directly work on a declaredQ_PROPERTY. However, currently none of the relevant properties are exposed as aQ_PROPERTY. The closest are those on theEditableMapclass, but they're lacking aNOTIFYsignal which means the widget's value can't update when needed. I'm also not entirely sure about usingEditableMapsince it's the scripting API, but it could work out and some people could use the change signals in the API too anyway.
When not using QObjectProperty, another thing I'm not happy about is that each property needs to do its own change signal handling. Hence I'm looking into introducing a kind of MapProperties class, which would own all the map's properties and connect to the relevant change signals instead.
Small update on the current state. Last week I've finished implementing the built-in properties for all data types, minus a few that will need a custom widget:
In the above we see a lacking widget for setting "Allowed Transformations" as well as for changing tileset parameters (which will use a button for now, similar to "Resize Map").
In the past days I've addressed many loose ends, like setting minimum/maximum values, step sizes and file dialog filters. I've also improved the widget for setting up the font of text objects, so it takes up less vertical space:
The biggest task remaining is to add support for editing, adding and removing Custom Properties, and also to make the headers collapsible. Other open tasks are restoring support for changing the properties of multiple objects at once and displaying "inherited" values like the class set on a tile when a tile object is selected.
Screenshot showing some of the recent changes:
- Collapsible headers.
- Support for editing custom properties, including support for expandable custom classes.
- Boolean properties showing their name on the right side of the checkbox rather than in the left column (test).
- Color properties displaying the "Unset" state again (as in old properties framework).
- "Remove" buttons to remove top-level properties.
- "Reset" buttons to reset class members to their default value, and indicating whether a class value is set with bold font.
After the above change, inherited properties are now shown in disabled state, with a '+' button that allows adding those properties:
One issue that remains to be addressed is when the removal of a property exposes an inherited property with the same name, but with a different type. The relevant property will need to be re-created in this case.
Latest changes address various features that already existed with the old properties solution:
- Handling changes in the custom property type definitions.
- Showing suggested properties from all selected objects.
- Applying changes to all selected objects or layers
Also there is now a custom widget for the tile object flipping states:
And there is a tool tip showing the type of each custom property, including the type for class members:
Today I've added a context menu with some useful actions:
Also, the expanded state is now remembered (by property name, so it applies to that property regardless of which object is selected).
Reported feedback so far:
- Icons for the add/remove/reset buttons are lacking a fallback so they only work with a system icon theme.
- Scroll wheel on some edit widgets triggers focus and value change (affects spin boxes and combo boxes at least).
- Undo shortcut is eaten by local widget's undo support, but expected is generally the global undo.
- Consider moving "Output Chunk Size" together with "Infinite" because the latter determines whether the former is enabled.
- Consider moving "Compression Level" together with "Layer Data Format", for same reason.
- It's hard to unfocus an editor (previously could be done by clicking anywhere outside of it).
- Repeated changes to Map properties are not grouped together.
- Collapsed state of headers isn't remembered.
- If Map/Tileset/Layer/etc Properties are collapsed and one tries to access them through the menu, they should expand (see also #3806).
- Consider adding Map/Tileset Properties buttons to the bottom of the properties panel (and to the document tab context menu) (see #3101)
- The Add/Remove/Reset buttons take up a lot of space in a narrow view. Would be good to add an option to hide them (the actions are also available from the context menu, anyway).
- Boolean labels True/False may be preferred to On/Off.
Many thanks to @eishiya for all their feedback!
Many of the issues mentioned by @eishiya have now been addressed. Except for these:
- It's hard to unfocus an editor (previously could be done by clicking anywhere outside of it).
When looking into this one, the thing is that you need to click on a widget that will take focus by mouse click. This does seem to include most of the UI since any scrollable view will take focus, but it does not include tool bars and their buttons, title bars or tabs. I wonder if this is really an issue?
- Consider moving "Output Chunk Size" together with "Infinite" because the latter determines whether the former is enabled.
While it is related, I'm leaving it where it is now, because it also belongs together with the layer data format settings. It's also a bit of an advanced setting, anyway.
- Boolean labels True/False may be preferred to On/Off.
I'd prefer to stick with On/Off for now. I noticed this is what they used in Godot and it seems less programmer oriented and easier to translate (in German the True/False is currently translated as Wahr/Falsch, which feels weird to me whereas An/Aus would be fine). Other alternatives might be Yes/No or Enabled/Disabled, but I also worry what translators might make of the latter.
Finally the below issues are still on the list of stuff to look into:
- Consider adding Map/Tileset Properties buttons to the bottom of the properties panel (and to the document tab context menu) (see https://github.com/mapeditor/tiled/issues/3101)
- The Add/Remove/Reset buttons take up a lot of space in a narrow view. Would be good to add an option to hide them (the actions are also available from the context menu, anyway).
I see for this you've gone for active widgets and bold labels for overridden values, rather than the approach in the Properties view with inactive widgets for non-overridden values. Is there a reason for this difference? I imagine some might prefer this style for the properties view as well. Active widgets are more convenient for when you expect to be overriding most properties, whereas inactive widgets are good if you expect to only override the occasional one. For me, the case is usually the former.
I see for this you've gone for active widgets and bold labels for overridden values, rather than the approach in the Properties view with inactive widgets for non-overridden values.
The difference you're seeing is between handling the top-level properties and handling class members. It exists mainly to reflect that a top-level property can be "removed", whereas a class member can only be "reset". A class member exists because it is defined in the class (so it's not going to go away), whereas a top-level property exists because it has been added to the object.
It might make sense to handle top-level properties that are shown based on the class of an object the same way as class members elsewhere. Or in general handle all "inherited" properties that way. Though I'm not entirely sure about this.
Had to experiment with it a bit myself in the properties view to understand, but I see what you mean now. Most of my custom type use is as a Class rather than as a Type, so I hadn't noticed the difference.
While I do like how clear it is that a property inherited from a class "doesn't exist" with the disabled widget, it's inconvenient in most cases because after setting a class, I have to "manually" add all those properties instead of just being able to start editing them like before. Makes using classes slow. Perhaps it might be enough to have the widgets always be active like for member properties, but with a remove "-" button instead of the reset broom button? Though really, in most (all?) cases, remove and reset do the same thing on an inherited property, since there's still some inherited value. So I think treating all inherited items the same, whether top-level or member, would be best.
hmm seems that the new system you cand copy-paste properties anymore :(
You can, though for now only with shortcuts Ctrl+X/C/V. I still need to add those actions back to the context menu.
Got a question - Will this PR be a breaking change for existing exporters for other engines?
Got a question - Will this PR be a breaking change for existing exporters for other engines?
No, this change is purely related to improving the UI of the Properties view the related view on class members in the Custom Types Editor.
When this change is done I will resume work on #4002, which will be a new feature to support in exporters and other engines. It's not expected to be a breaking change either though, unless array properties are actually used.