[Development] Bug (and fix) in QGItem::itemChange() (?)

[Ch'Gans]

Please find attached a simple demo:
If you try to move the big green rect by dragging the small red rect, the
big rect will goes "wild", using my proposed fixed, the big rect follows
the mouse as you drag the small rect.

Chris

On 16 February 2017 at 17:52, Ch'Gans <chgans at gna.org> wrote:

> Hi there,
>
> Here is my use-case:
> An item has "handles" child items, when the user moves around an handles,
> the parent item update it's geometry and position accordingly. Simple
> concept to let the user change an item geometry by dragging handles.
> Imagine, you're using a drawing application, you select a circle and 5
> handles appear on the screen, one in each corner to resize the circle, and
> one in the center to move the circle around.
>
> The way i implement it is to let the circle item filter an handle item's
> 'itemChange()' notification. This work nicely for resizing handles, but
> fails for the 'move handle'. Here is the simplified pseudo code:
> QVariant HandleItem::itemChange(change, value)
> {
>  // Move the parent to where the handle wants to go...
>  parentItem()->setPos(parentItem()->mapToParent(value);
>  // ... and leave the handle at parent's origin
>  return QPointF(0, 0);
> }
>
> After banging my head for a while, i finally pin-pointed what I think is
> the problem:
>
> This is related to how items are moved in qt5/qtbase/src/widgets/
> graphicsview/qgraphicsitem.cpp. When a move begins, the 'initial
> position' of the item is stored in the scene. This initial position is
> simply the value of item->pos(), which is in parent's item CS:
>
> initialPositions = d_ptr->scene->d_func()->movingItemsInitialPositions;
> if (initialPositions.isEmpty()) {
>   foreach (QGraphicsItem *item, selectedItems)
>     initialPositions[item] = item->pos();
>   initialPositions[this] = pos();
> }
> d_ptr->scene->d_func()->movingItemsInitialPositions = initialPositions;
>
> As the move goes on, the item's pos is updated based on this 'initial
> position' (expressed in an outdated parent's CS) and the parent position
> relative to the item's position:
>
> currentParentPos = item->mapToParent(item->mapFromScene(event->scenePos()
> ));
> buttonDownParentPos = item->mapToParent(item->mapFromScene(event->
> buttonDownScenePos(Qt::LeftButton)));
> [...]
> item->setPos(initialPositions.value(item) + currentParentPos -
> buttonDownParentPos);
>
> Basically the above 3 lines would be correct only and only if the
> initialPositions were expressed in a dynamic way, that is, if they were
> independent of the parent's CS since it can change during the move.
>
> The following pseudo-code modifications fixes that:
>
> // 'Capture' time
> initialPosition = item->mapToScene(item->mapFromParent(item->pos())); //
> Independent of parent CS
> [...]
> // 'Update' time
> initialPosition = item->mapToParent(item->mapFromScene(initialPosition));
> // up-to-date w/ changed parent's CS
> item->setPos(initialPosition + currentParentPos - buttonDownParentPos);
>
>
> This modification fixes my use-cases, and doesn't seem to break the "40000
> chips" and other demo/example.
> The scene's private 'movingItemsInitialPositions' is only set/used by
> QGI::mouseMoveEvent() and cleared by QGI::mouseReleaseEvent(), so this is
> not an invasive change
>
>
> So my chain of question is:
> - Is changing the item's parent position in an item's itemChange() a
> supported feature?
> - if it is, then isn't there a bug in qgraphicsitem.ccp as i described
> above?
> - if positive, is the above fix correct and would you accept this sort of
> things if i push that to gerrit?
>
>
> Chris
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.qt-project.org/pipermail/development/attachments/20170216/31e75c2c/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: main.cpp
Type: text/x-c++src
Size: 1509 bytes
Desc: not available
URL: <http://lists.qt-project.org/pipermail/development/attachments/20170216/31e75c2c/attachment.cpp>