Bug 74074 - [Qt] Visualize mock points in the Qt MiniBrowser
Summary: [Qt] Visualize mock points in the Qt MiniBrowser
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 74098
  Show dependency treegraph
 
Reported: 2011-12-08 04:33 PST by Alexander Færøy
Modified: 2011-12-08 10:39 PST (History)
3 users (show)

See Also:


Attachments
Patch (9.36 KB, patch)
2011-12-08 04:46 PST, Alexander Færøy
no flags Details | Formatted Diff | Diff
Patch (9.20 KB, patch)
2011-12-08 06:39 PST, Alexander Færøy
no flags Details | Formatted Diff | Diff
Patch (9.12 KB, patch)
2011-12-08 06:58 PST, Alexander Færøy
no flags Details | Formatted Diff | Diff
Patch (10.97 KB, patch)
2011-12-08 08:13 PST, Alexander Færøy
no flags Details | Formatted Diff | Diff
Patch (10.94 KB, patch)
2011-12-08 09:17 PST, Alexander Færøy
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Færøy 2011-12-08 04:33:21 PST
SSIA.
Comment 1 Alexander Færøy 2011-12-08 04:46:42 PST
Created attachment 118363 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2011-12-08 05:02:39 PST
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?
Comment 3 Tor Arne Vestbø 2011-12-08 05:21:20 PST
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
Comment 4 Tor Arne Vestbø 2011-12-08 05:22:17 PST
(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
Comment 5 Alexander Færøy 2011-12-08 06:39:52 PST
Created attachment 118382 [details]
Patch
Comment 6 Alexander Færøy 2011-12-08 06:58:25 PST
Created attachment 118383 [details]
Patch
Comment 7 Tor Arne Vestbø 2011-12-08 07:19:16 PST
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());
Comment 8 Alexander Færøy 2011-12-08 08:13:15 PST
Created attachment 118393 [details]
Patch
Comment 9 Tor Arne Vestbø 2011-12-08 09:11:12 PST
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
Comment 10 Alexander Færøy 2011-12-08 09:17:28 PST
Created attachment 118403 [details]
Patch
Comment 11 Tor Arne Vestbø 2011-12-08 09:24:28 PST
Comment on attachment 118403 [details]
Patch

great!
Comment 12 WebKit Review Bot 2011-12-08 10:39:09 PST
Comment on attachment 118403 [details]
Patch

Clearing flags on attachment: 118403

Committed r102348: <http://trac.webkit.org/changeset/102348>
Comment 13 WebKit Review Bot 2011-12-08 10:39:14 PST
All reviewed patches have been landed.  Closing bug.