Bug 74074 – [Qt] Visualize mock points in the Qt MiniBrowser

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Alexander Færøy]

SSIA.

[Alexander Færøy]

Created attachment 118363 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 118363 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118363&action=review

> Tools/ChangeLog:16
> +        * MiniBrowser/qt/qml/TouchMockPoint.qml: Copied from Tools/MiniBrowser/qt/BrowserWindow.h.

Pretty stupid comment :-) pls remove and add Added

> Tools/MiniBrowser/qt/BrowserWindow.cpp:88
> +    QHash<int, QQuickItem*>::iterator iter = m_touchMockPoints.find(id);

we normally use the name "it" in webkit

> Tools/MiniBrowser/qt/BrowserWindow.cpp:99
> +
> +    iter.value()->setX(position.x() / 2);
> +    iter.value()->setY(position.y() / 2);
> +    iter.value()->setOpacity(opacity);

why not (*it)->set...

> Tools/MiniBrowser/qt/MiniBrowserApplication.cpp:174
> +            // Get rid of touch-points that are no longer valid

+ . at the end :-)

> Tools/MiniBrowser/qt/qml/TouchMockPoint.qml:31
> +    visible: true

isn't that default?

[Tor Arne Vestbø]

Comment on attachment 118363 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118363&action=review

> Tools/MiniBrowser/qt/BrowserWindow.cpp:86
> +void BrowserWindow::updateTouchMockPoint(int id, const QPointF& position, qreal opacity)

updateMockTouchPoints

>> Tools/MiniBrowser/qt/BrowserWindow.cpp:88
>> +    QHash<int, QQuickItem*>::iterator iter = m_touchMockPoints.find(id);
> 
> we normally use the name "it" in webkit

Look up old touch points that you can re-use from the engine based on the id, instead of keeping a list.

> Tools/MiniBrowser/qt/BrowserWindow.cpp:97
> +    iter.value()->setX(position.x() / 2);

set the x and y, and do the offsetting to center it in the qml

>> Tools/MiniBrowser/qt/BrowserWindow.cpp:99
>> +    iter.value()->setOpacity(opacity);
> 
> why not (*it)->set...

i'd rather see a "pressed" property that you set and unset based on the point being released or not, and you can then deal with how you hide it in the qml

> Tools/MiniBrowser/qt/MiniBrowserApplication.cpp:163
> +

do we ever have a situation without a browser window?

> Tools/MiniBrowser/qt/MiniBrowserApplication.cpp:168
> +        qreal opacity = 1.0;

see comment about "pressed". we should not deal with opacity here, that's an implementation detail of the visual component

[Tor Arne Vestbø]

(In reply to comment #3)
> (From update of attachment 118363 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118363&action=review
> 
> > Tools/MiniBrowser/qt/BrowserWindow.cpp:86
> > +void BrowserWindow::updateTouchMockPoint(int id, const QPointF& position, qreal opacity)
> 
> updateMockTouchPoints

Or, updateVisualMockTouchPoints

[Alexander Færøy]

Created attachment 118382 [details]
Patch

[Alexander Færøy]

Created attachment 118383 [details]
Patch

[Tor Arne Vestbø]

Comment on attachment 118383 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118383&action=review

> Tools/MiniBrowser/qt/BrowserWindow.cpp:107
> +        foreach (QQuickItem* item, rootObject()->childItems()) {
> +            QVariant itemId = item->property("id");
> +
> +            if (itemId.toString() == mockTouchPointIdentifier) {
> +                mockTouchPointItem = item;
> +                break;
> +            }
> +        }

Set the objectName instead of the id, and use rootObject()->findChild<QQuickItem*>(mockTouchPointIdentifier);

> Tools/MiniBrowser/qt/BrowserWindow.cpp:118
> +        mockTouchPointItem->setProperty("pressed", QVariant(pressed));

replace the pressed variable with touchPoint.state == Qt::TouchPointReleased

> Tools/MiniBrowser/qt/MiniBrowserApplication.cpp:166
> +    BrowserWindow* browserWindow = dynamic_cast<BrowserWindow*>(targetWindow);
> +
> +    if (browserWindow)
> +        browserWindow->updateVisualMockTouchPoints(m_touchPoints.values());
> +

if (BrowserWindow* browserWindow = qobject_cast<BrowserWindow*>(targetWindow))
    browserWindow->updateVisualMockTouchPoints(m_touchPoints.values());

[Alexander Færøy]

Created attachment 118393 [details]
Patch

[Tor Arne Vestbø]

Comment on attachment 118393 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118393&action=review

> Tools/MiniBrowser/qt/BrowserWindow.cpp:92
> +        bool pressed = touchPoint.state != Qt::TouchPointReleased;
> +        QPointF position = touchPoint.area.topLeft();
> +        position.rx() -= geometry().x();
> +        position.ry() -= geometry().y();

Move this down to where you set it, and remove the pressed-variable, you can set the property directly based on touchPoint.state != Qt::TouchPointReleased

[Alexander Færøy]

Created attachment 118403 [details]
Patch

[Tor Arne Vestbø]

Comment on attachment 118403 [details]
Patch

great!

[WebKit Review Bot]

Comment on attachment 118403 [details]
Patch

Clearing flags on attachment: 118403

Committed r102348: <http://trac.webkit.org/changeset/102348>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.