Editing string property values creates too many Undo events #3103
Comments
bjorn
added
the
usability
Previously, when editing any built-in string property (like name and type) or a custom string or file property, an undo command would be created for each typed character. While ideally this could be resolved by merging the undo commands, there are so many of them that I felt this would not be practical. Here, I have instead opted to apply the change only when editing is "finished", which means either the user pressed Enter or the edit field lost focus. One thing I'm not happy with, is that in this mode you'd probably expect the text to not change when pressing Escape, but then it applies as well. Closes #3103
|
@eishiya I've tried to implement a possible solution, but I'm not entirely sure about it. Maybe you have some thoughts about it as well. There will be builds available soon at https://github.com/mapeditor/tiled/actions/runs/1578850353. |
|
I gave it a look and played around with some custom string properties and with layer and object names (via the Properties panel), and this definitely cleans up the Undo stack nicely, undoing changes is nice and snappy now. I was expecting to miss the live preview when changing Object names and layer names, but I don't, having it commit at the end felt perfectly fine :D As you mentioned in the commit message, it's weird that Escape commits the new name, I think it should cancel the edit operation and bring back the old value. This behaviour is very unusual. Escape works as expected (cancels edit) when editing layer names via the Layers panel and those have clean Undos and commit upon Enter or losing focus, is there some way to get the same behaviour for text edited via the Properties panel? |
Probably there is, but I haven't looked into it yet. I've made an alternative change (which could also work in addition, of course), which instead merges changes to the same object and property, as well as removing undo entries when they become obsolete due to the merging (so, when changing the same property back and forth). A build with this change will be available at https://github.com/mapeditor/tiled/actions/runs/1582618374, just note that it only affects built-in layer properties for now. I'd be interested to hear your opinion about this as well. If it works well, I will put in some additional effort to make it work for custom properties and the properties of other types. |
|
I tried renaming a layer via the Properties panel. Undo works as expected, but I see there's the added benefit of the changes being "live". Unfortunately, this has the same problem with Escape as the previous change did - Escape commits the changes in the properties view, instead of cancelling them like it does in the Layers/Objects panels. |
When the change applies "live", then Escape is just a way to close the editor and it can't "cancel" the change because the old value is long gone. I can only change the Escape behavior, when the value has still only changed in the editor widget and hasn't been applied yet. |
|
I think I'd rather see consistent Esc cancel behaviour over live updates, especially since for just about everything but names, the live string updates don't actually have a visible effect. |
I would like to eventually transition to a layout where all the properties have "live" widgets though, which works a lot better especially for combo boxes and checkboxes. This would get rid of Esc as a meaningful shortcut. In the meantime though, I guess I could try to get the canceling to work. |
That's a shame, but it's something one can get used to, especially with just one Undo required to bring back the old value, and with Undo working even when the field is focused :] |
* Merge undo entries when they affect the same property on the same layer. * Remove the last undo entry when it becomes obsolete. This happens when another change is made directly afterwards that brings the property back to its original value (for example, when toggling a layer's visibility on and off). Unfortunately since undo commands can currently affect only a single layer, neither of the above works when multiple layers are changed at the same time. Issue #3103
The undo commands affecting ImageLayer and ObjectGroup properties are now based on the "ChangeValue" command as well. The "ChangeValue" command was improved to be able to change the value of multiple objects at once, which also resolves the problem where toggling for example the visibility of a set of layers on and off would fail to mark the undo command as obsolete. Part of issue #3103
* MapObject::name (and other MapObject properties) * WangColor::name * WangSet::name * Tileset::name Part of issue #3103
|
I looked into the possibility to make the Escape key work like a cancel, but it seems like this is difficult or would be rather hacky, due to this actually default behavior being bypassed by the "QtPropertyBrowser" solution. So, I've just continued to make more undo commands merge together where applicable. This of course turns out to be a major effort as well... But, a new build will be available soon that merges most string properties, except for custom string properties, that change is still work in progress. |
* Merge undo entries when they affect the same property on the same layer. * Remove the last undo entry when it becomes obsolete. This happens when another change is made directly afterwards that brings the property back to its original value (for example, when toggling a layer's visibility on and off). Unfortunately since undo commands can currently affect only a single layer, neither of the above works when multiple layers are changed at the same time. Issue #3103
The undo commands affecting ImageLayer and ObjectGroup properties are now based on the "ChangeValue" command as well. The "ChangeValue" command was improved to be able to change the value of multiple objects at once, which also resolves the problem where toggling for example the visibility of a set of layers on and off would fail to mark the undo command as obsolete. Part of issue #3103
* MapObject::name (and other MapObject properties) * WangColor::name * WangSet::name * Tileset::name Part of issue #3103
At some point after 1.4.0, Undo events for editing property, object, layer, etc names were modified so that changing the name would create just one Undo event, instead of creating one for each typed character. However, it seems like properties of type
stringwere left out of this, and still generate an Undo event for every individual change, such as every character typed and every individual deletion.While editing the string, Undo works as one would expect in a text editor, grouping together additions and deletions. However, after committing the change, Undo causes each individual typed character and deletion to be undone one by one.
Expected behaviour: Undo should either revert the value to what it was before the edit began, or it should have the same style of Undo events as during editing, grouping together additions and deletions.
I think the first option (revert the whole value) would be better, as it would be consistent with editing names and with editing other property values. Piecemeal undo/redo is already available while editing the string, which is when it's usually needed.
The text was updated successfully, but these errors were encountered: