Uploaded image for project: 'Qt'
  1. Qt
  2. QTBUG-58988

QGraphicsView: Wrong pos when moving item and using itemChange() to change parent's pos

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Reported
    • Priority: P3: Somewhat important
    • Resolution: Unresolved
    • Affects Version/s: 5.7.1, 5.8.0
    • Fix Version/s: None
    • Component/s: Widgets: GraphicsView
    • Labels:
      None
    • Environment:
      Tested on Linux with Qt-5.8 (git), Qt-5.7 and 5.6

      Description

      As initially reported here:
      http://lists.qt-project.org/pipermail/development/2017-February/028826.html

      When moving an item that uses itemChange() to update it's parent pos() and it's own pos(), the item pos is not updated correctly.

      See attached file to reproduce the problem.

      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?

        Attachments

        1. main.cpp
          1 kB
        2. qtbug58988.zip
          1 kB
        No reviews matched the request. Check your Options in the drop-down menu of this sections header.

          Activity

            People

            • Assignee:
              chgans Christian Gagneraud
              Reporter:
              chgans Christian Gagneraud
            • Votes:
              1 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:

                Gerrit Reviews

                There are no open Gerrit changes