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

New properties framework #4045

Draft
wants to merge 33 commits into
base: master
Choose a base branch
from

Conversation

bjorn
Copy link
Member

@bjorn bjorn commented Aug 30, 2024

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.

@bjorn
Copy link
Member Author

bjorn commented Aug 30, 2024

Here's a preview of what it looked like at some point, when it wasn't actually functional but had all Map properties represented:

image

And here's a video of the current state, with only three properties, but they are fully implemented:

resize-map-button.mp4

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.).

@dogboydog
Copy link
Contributor

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

@bjorn
Copy link
Member Author

bjorn commented Sep 2, 2024

@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 ValueProperty keeps 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 QObjectProperty could be even more automatic, since it wraps Qt's meta object system to directly work on a declared Q_PROPERTY. However, currently none of the relevant properties are exposed as a Q_PROPERTY. The closest are those on the EditableMap class, but they're lacking a NOTIFY signal which means the widget's value can't update when needed. I'm also not entirely sure about using EditableMap since 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.

src/tiled/propertieswidget.cpp Outdated Show resolved Hide resolved
* DoubleSpinBox precision logic ported from existing property editor
* Allow spin boxes and combo boxes to shrink horizontally
* Elide property names when there isn't enough space
* Added QPointF and QColor editor factories
* Added test with all built-in map properties
* Added headers (not collapsible for now).
* Added separators.
* Added support for non-consecutive enums in EnumEditorFactory.
* Stretch name and editor widgets by a fixed 2:3 ratio and ignore their
  size hint.
* Some start on possible API for setting/getting the value.
It adjusts the layout based on its width.

Also made a few other visual tweaks.
Introduced Property class and changed from using editor factories to
having the property itself create its widget.
* Map size is read-only but features a "Resize Map" button that triggers
  the resize dialog.

* Both map size and tile size are implemented based on the new
  "ValueTypeEditorFactory" which selects an editor based on the value
  type. This functionality is moved out of the VariantEditor.

* Introduced AbstractProperty, ValueProperty and QObjectProperty in an
  attempt to provide some convenience on top of the Property interface.

I'm not entirely convinced it was a good idea to put the "createEditor"
function on the Property. It makes it easy to do custom widgets for a
property, but annoying to have it use a type-based editor.
* Introduced MapProperties class to make it easier to emit valueChanged
  for all the map's properties, as well to move their creation out of
  PropertiesWidget::currentObjectChanged.

* Added GetSetProperty that makes it easier to define a property in
  terms of a given getter and setter function, since it avoids the need
  for a specific Property subclass.

* Finished implementation of the BoolEditorFactory (signals connected).

* Moved property edit widgets to their own file.
* Introduced a few helper functions to reduce code duplication, like
  MapProperties::push.

* Disabled properties when they are irrelevant.

* Finished connecting the signals for the remaining editor factories:
  StringEditorFactory, IntEditorFactory, FloatEditorFactory,
  PointEditorFactory, PointFEditorFactory, RectFEditorFactory and
  ColorEditorFactory.
Not sure why Qt 6 didn't need these missing metatype declarations.
This property uses an editable combo box with the valid classes set up for
the given object type.
Still needs special handling for:

* Color (requires handling invalid/unset values)
* Widget for editing allowed transformations
* Widget for showing tileset parameters (and trigger edit dialog)
* Specifying 1 as minimum value for column count
Some details remain to be done:

* Changes made to layer properties should apply to all selected layers
* Opacity should probably be a slider now and have appropriate limits
* Step size for opacity and parallax factor is too big
Added support for editing QUrl values using UrlEditorFactory that
creates a FileEdit.
Also in this change:

* Made SpinBox and DoubleSpinBox don't respond to changing from
  keyboard typing immediately.

* Share a single implementation of wrapping label/widget pairs for
  SizeEdit, SizeFEdit, PointEdit, PointFEdit and RectFEdit.

* Added widget factories for QFont, QSizeF and Qt::Alignment.

* Allow setting suffix on custom FloatEditorFactory, used for adding
  degree symbol to rotation values.

A few things remain to be done:

* Custom widget for editing tile object flipping flags
* Try reducing size of font editor with style toggle buttons
Now all built-in properties are present, apart from some loose ends.

* Added RectEdit / RectEditorFactory (QRect values)
* Added Property::toolTip (set on label and editor, but currently not
  functional on label due to the eliding tool tip logic)
* Added change events to fix updating of WangSet and WangColor properties
  (was already broken with old framework)
Instead there are now typed Property subclasses similar to the generic
GetSetProperty.
This name is more appropriate for what it actually does. It also no
longer derives from EditorFactory.

PropertyFactory::createQObjectProperty should be functional again. It
now returns an appropriate property, and the QObjectProperty class is
gone. This approach is still not used, however.
* Removed EditorFactory, EnumEditorFactory, AbstractProperty,
  ValueProperty and GetSetProperty.

* EnumProperty now derives from IntProperty and retrieves the enum
  meta-data based on its template argument.

* Use the typed properties to avoid QVariant when synchronizing between
  the created editor.

For the built-in properties, this definitely simplifies things. But it
remains to be seen how custom properties are best handled.

Also due to the lack of registering editor factories, QObjectProperty
is now broken for enums (but it's unused).
* Apply various minimum and maximum values
* Set custom step size where needed
* Set file filters where appropriate
* Support constraint on RectProperty
Makes font property widget take up less vertical space.
@bjorn
Copy link
Member Author

bjorn commented Sep 11, 2024

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:

image

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:

image

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.

* Added support for editing top-level custom properties (excluding support
  for classes and enums for now).

* Added support for collapsing the property groups, based on a new
  GroupProperty which creates nested VariantEditor instances.

* Removed QVariant based getter/setter from Property, since it doesn't
  appear to be useful.

* Added Property::DisplayMode, which is used to tell apart group
  properties and separators from regular properties.

* VariantEditor is no longer a scroll area itself, to enable nesting.
Also fixed indentatation of property labels to match the header widget.

And fixed the margins on the side for groups of properties without
separator (affected Custom Properties).

Not entirely sure moving the label of boolean properties is the right
thing to do.
Of course, this means for changing opacity the Properties view should
now be open. There is also no longer a way to see the actual value
anymore, but that could be added to the widget in the Properties view
(maybe as percentage).
Also, enabling a slider does not replace the spinbox, but puts a slider
next to it. Because I don't think a bare slider is ever what we'd want
to use.
Not everything was deleted from the layout because the deletion was
lacking support for handling nested layouts.

Also removed the only remaining case where a nested layout was used at
the top-level.
* Added "px" suffixes to various pixel values.

* Added support for custom enum properties (top-level only so far,
  because there is no support for class properties yet). It's amazing
  how trivial this was with the new approach (though, it does not
  support enums with values as flags yet).

* Enabled editing of tileset image parameters through a button and made
  some progress towards allowing a GroupProperty to appear like a normal
  property (currently it can, but then it no longer shows its children).
It creates a checkbox for each flag. They will need to be able to
collapse in case there are lots of flags, but this works for now.
Also when it isn't displayed as a header. Also introduced a "level"
member of VariantEditor which is passed on to the PropertyLabel, which
is used to indent expanded child properties.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants