Skip to content

Use new undo/redo history and item traits system with smart pointers

Noah Davis requested to merge work/ndavis/new-undo-redo-history into master

TL;DR: This MR adds a new undo/redo item system based on smart pointers instead of raw pointers and items that have drawable traits rather than types that match a specific AnnotationTool type.

Pointer Safety

Previously, we used lists of raw pointers to annotation objects for undo/redo history. Each annotation object also had a replaces and replacedBy raw pointer that were used to indicate that a later item replaced an earlier item and prevent the earlier item from being rendered. The selected action wrapper also had a raw pointer to know which annotation object was selected. As long as all memory was properly freed, there wasn't much of an issue with the previous undo/redo lists, but the selected action pointer and replaces/replacedBy pointers could easily become dangling pointers and cause segfaults if a developer wasn't careful enough. We also used polymorphic classes for the annotation objects with an EditAction virtual base class. In order to use all the attributes of an annotation object, you'd need to cast them to the correct type. The type of polymorphism used made it awkward to share features between classes.

The new system uses lists of shared_ptrs for undo/redo history. The previous selected pointer and replaces/replacedBy pointers were turned into weak_ptrs. replaces/replacedBy were also renamed to parent/child since I found it took a bit too much effort to avoid confusing replaces and replacedBy. We still have to check if the pointer isn't empty before trying to use it, but the pointers are no longer at risk of becoming dangling pointers and shared_ptr ensures that the object it points to is kept as long as you're still using the shared_ptr.

Just as an example, the changes fix https://bugs.kde.org/show_bug.cgi?id=469919.

New/Different Behaviors

When creating a new item, the AnnotationTool type decides which traits are enabled and how they should be initialized. We do not need to keep track of a tool type once a drawable object is created, we just need to know what traits it has when deciding how to modify it later and what options to show in the UI for selected items. For instance, now any item with the Geometry, Stroke and Fill traits can show the same options as items created by the RectangleTool. This means at a later time we can add optional backgrounds for items created by the TextTool without needing to create any new classes or write any new rendering code.

History is now kept in its own class separate from AnnotationDocument. It helps to encapsulate the management of history items, but it also makes it so that later we can go back to previous screenshots with their own sets of history. An example of when we might want that is when a user starts a rectangle screenshot, but wants to cancel that to go back to the previous screenshot. Currently, the whole image and all its history are permanently replaced. Besides that, this class makes the management of history less manual within the context of AnnotationDocument where the most complex code is. The behavior and API of the History class is minimal, but gives feedback in the form of return values that can be used for more complex behaviors in AnnotationDocument.

We have talked about making Spectacle's annotations into a reusable library one day and my hope is that this takes things a step closer. We'll probably need to add clear ways for app devs to extend the functionality if we ever do make it into a library.

The previous type of annotation object geometry was mainly based on QRectFs and used those to represent the annotation click/hover/selection areas in the UI, but the new items rely on QPainterPath. We can still use rectangles just by making a rectangle path, but using QPainterPath for geometry enables things we previously didn't have, such as form fitting click/hover/selection areas. To make sure clicking on an item isn't difficult when it's a 1px thick stroke, there is now a 4px margin around the mouse pointer for hovering or selecting items.

Form Fitting Outline
Screenshot_20240124_010847

The new history items use up more memory than the EditAction based items, but the size isn't far off (144 bytes for EditAction and 200 bytes for TextAction vs 256 bytes for HistoryItem. Since std::optional is used for setting traits, the memory for the underlying data is allocated, but no processing is done beyond that. Not even constructors of the members of each trait. The optional traits are all stored in a tuple, mainly because actual container types like vector, set or map only accept one value type, so I'd have to use std::variants.

Testing Checklist

  • With every tool except for Freehand, Highlight, Text and Number, clicking and releasing in the annotation viewport without moving the mouse should not allow an undo on mouse release.
  • An item created by the Text tool should disappear automatically if there is no text and the text item loses selection or the tool changes.
  • Selecting or unselecting an annotation should turn any uncommitted changes to a selected annotation into an undoable item.
  • Clicking and dragging an unselected annotation should select and move it 1:1 with mouse movement at zoom levels above and below 100%.
    • High speed movements made within 250ms of selecting and dragging should not create 2 undoable items.
    • upon mouse release, an undoable item for the translation should be created.
  • Dragging a resize handle on a selected annotation should resize it in a direction appropriate for the handle.
  • Resize handles should move 1:1 with mouse movement at all zoom levels.
  • resize handles should be able to invert the selection's geometry when dragged enough to create a negative scale.
  • The text tool's handle should move the same way as when dragging a selected item and only translate the text item.
  • Resizing blur or pixelate items outside the bounds of the base image should not create stretched effect images.
  • Freehand drawing should appear fairly smooth without steppy or laggy movement. There are limitations to how smooth it can be, but it should be decent.
  • Resizing an annotation item should always rerender every visible part of the item without leaving traces of the old version of the item behind in the viewport.
  • Resize handles should not appear for number or text items.
  • It should be possible to delete text items by deleting all of their text and pressing delete.
Edited by Noah Davis

Merge request reports