letsfindaway / OpenBoard

I'm using this fork to contribute features and fixes to the upstream project. In order to create good pull requests, I'm rebasing my feature branches, squashing and reordering commits, etc. If you fork this repository be aware that my development branches may rewrite history without prior notice.
http://openboard.ch/
GNU General Public License v3.0
9 stars 0 forks source link

Serialization of a group #137

Closed letsfindaway closed 1 year ago

letsfindaway commented 1 year ago

When you persist a scene with a group, the group elements are persisted twice: Once as members of the group and identically again as individual strokes. When re-opening the document you can easily observe this by selecting the group or one of the group elements and moving it away.

letsfindaway commented 1 year ago

The problem here seems to be UBGraphicsScene::sceneDeepCopy which creates the additional polygons by not correctly handling a group.

Note that QGraphicsScene::items() not only returns top level items, but also recursively all childItems(), because QGraphicsScene::addItem() recursively adds all child items. See https://github.com/qt/qtbase/blob/29400a683f96867133b28299c0d0bd6bcf40df35/src/widgets/graphicsview/qgraphicsscene.cpp#L2630. This is not clear from the documentation, but obvious from the implementation.

Before copying, a simple scene with a group containing two strokes looks as follows. Note that only the five elements marked with an asterisk are items of the scene, the indented information is just to document the item hierarchy:

*  Polygon parent=0x423b350 stroke=0x4239a90 strokesGroup=0x423b340
*  StrokesGroup parent=0x438db00 children=1
     Polygon parent=0x423b350 stroke=0x4239a90 strokesGroup=0x423b340
*  Polygon parent=0x4175da0 stroke=0x437f4f0 strokesGroup=0x4175d90
*  StrokesGroup parent=0x438db00 children=1
     Polygon parent=0x4175da0 stroke=0x437f4f0 strokesGroup=0x4175d90
*  Group parent=0x0 children=2
     StrokesGroup parent=0x438db00 children=1
       Polygon parent=0x4175da0 stroke=0x437f4f0 strokesGroup=0x4175d90
     StrokesGroup parent=0x438db00 children=1
       Polygon parent=0x423b350 stroke=0x4239a90 strokesGroup=0x423b340

After copying the scene looks as follows:

*  Polygon parent=0x0 stroke=0x0 strokesGroup=0x0
*  Polygon parent=0x0 stroke=0x0 strokesGroup=0x0
*  Polygon parent=0x423ea40 stroke=0x439a000 strokesGroup=0x423ea30
*  StrokesGroup parent=0x4240480 children=1
     Polygon parent=0x423ea40 stroke=0x439a000 strokesGroup=0x423ea30
*  Polygon parent=0x423fc60 stroke=0x42405e0 strokesGroup=0x423fc50
*  StrokesGroup parent=0x4240480 children=1
     Polygon parent=0x423fc60 stroke=0x42405e0 strokesGroup=0x423fc50
*  Group parent=0x0 children=2
     StrokesGroup parent=0x4240480 children=1
       Polygon parent=0x423fc60 stroke=0x42405e0 strokesGroup=0x423fc50
     StrokesGroup parent=0x4240480 children=1
       Polygon parent=0x423ea40 stroke=0x439a000 strokesGroup=0x423ea30
*  Polygon parent=0x423dcb0 stroke=0x4240110 strokesGroup=0x423dca0
*  StrokesGroup parent=0x0 children=1
     Polygon parent=0x423dcb0 stroke=0x4240110 strokesGroup=0x423dca0
*  Polygon parent=0x43afc90 stroke=0x423d800 strokesGroup=0x43afc80
*  StrokesGroup parent=0x0 children=1
     Polygon parent=0x43afc90 stroke=0x423d800 strokesGroup=0x43afc80

The strokes groups are duplicated and additionally two polygons outside of a stroke group are added, so we have two unneeded copies.

letsfindaway commented 1 year ago

UBGraphicsScene::sceneDeepCopy() is severely broken:

letsfindaway commented 1 year ago

Closing as PR was merged