Bug 35255 – [Qt] DRT: Send double click event from EventSender

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

[Diego Gonzalez]

Make Qt DumpRenderTree EventSender able to send double click events
                                                                       
LayoutTests:                                                       
    fast/events/dblclick-addEventListener.html

[Diego Gonzalez]

Created attachment 49233 [details]
Proposed patch

[WebKit Review Bot]

Attachment 49233 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/DumpRenderTree/qt/EventSenderQt.cpp:103:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Diego Gonzalez]

Created attachment 49319 [details]
Proposed patch

[Kenneth Rohde Christiansen]

Comment on attachment 49319 [details]
Proposed patch


> +    // only consider a click to count, a event which was sent from the
> +    // same previous button and from the same position
> +    if (m_currentButton == button && m_mousePos == m_clickPos)
> +        m_clickCount++;
> +    else
> +        m_clickCount = 1;

Is time difference not supposed to be considered as well? Doesn't Qt already handle this?

> +    QMouseEvent* event;
> +    if (m_clickCount == 2) {
> +        event = new QMouseEvent(QEvent::MouseButtonDblClick, m_mousePos,
> +                        m_mousePos, mouseButton, m_mouseButtons,
> +                        Qt::NoModifier);
> +    } else {
> +        event = new QMouseEvent(QEvent::MouseButtonPress, m_mousePos,
> +                        m_mousePos, mouseButton, m_mouseButtons,
> +                        Qt::NoModifier);

event = new QMouseEvent((m_clickCount == 2) ? QEvent::MouseButtonDblClick : QEvent::MouseButtonPress,
                        m_mousePos, m_mousePos, mouseButton, m_mouseButtons, Qt::NoModifier); 

seems simpler

[Diego Gonzalez]

(In reply to comment #4)
> (From update of attachment 49319 [details])
> 
> > +    // only consider a click to count, a event which was sent from the
> > +    // same previous button and from the same position
> > +    if (m_currentButton == button && m_mousePos == m_clickPos)
> > +        m_clickCount++;
> > +    else
> > +        m_clickCount = 1;
> 
> Is time difference not supposed to be considered as well? Doesn't Qt already
> handle this?
> 
> > +    QMouseEvent* event;
> > +    if (m_clickCount == 2) {
> > +        event = new QMouseEvent(QEvent::MouseButtonDblClick, m_mousePos,
> > +                        m_mousePos, mouseButton, m_mouseButtons,
> > +                        Qt::NoModifier);
> > +    } else {
> > +        event = new QMouseEvent(QEvent::MouseButtonPress, m_mousePos,
> > +                        m_mousePos, mouseButton, m_mouseButtons,
> > +                        Qt::NoModifier);
> 
> event = new QMouseEvent((m_clickCount == 2) ? QEvent::MouseButtonDblClick :
> QEvent::MouseButtonPress,
>                         m_mousePos, m_mousePos, mouseButton, m_mouseButtons,
> Qt::NoModifier); 
> 
> seems simpler

checking now about time considerations

[Antonio Gomes]

imho it is fragile aproach we to be considering all this variants ourselves. it would be much simpler if  EventSender classes had a dlKeyDown method, instead of blowing our fragile EventSender::keyDown w/ counts and time checks.

of course it'd be something to be done to other ports' drt , and be discussed in the webkit-dev mailing list first.

[Diego Gonzalez]

Created attachment 49384 [details]
Proposed patch

[WebKit Review Bot]

Attachment 49384 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/304469

[Diego Gonzalez]

Created attachment 49389 [details]
Proposed patch

[Antonio Gomes]

diego:

+    // only consider a click to count, a event which was sent from the
+    // same previous button and from the same position. Using 400ms as
+    // the double-click interval like in KDE default settings.

* aN event

* sent => originated by (?)

* AT the same position ...

[Diego Gonzalez]

Created attachment 49391 [details]
Proposed patch

Correcting comments. Thanks tonikitoo

[Antonio Gomes]

just guessing:

@diego: 

1) it is possible to have 3 click situations, like to selection a whole paragraph. I do not know if we have layout tests for that, but would the patch work ?

2) if in the same test we have a single click event followed by a dl click event (or in the reverse order). would the patch work ?

if answer is good for both question, i think it is robust enough :)

[Diego Gonzalez]

(In reply to comment #12)
> just guessing:
> 
> @diego: 
> 
> 1) it is possible to have 3 click situations, like to selection a whole
> paragraph. I do not know if we have layout tests for that, but would the patch
> work ?
> 

Yes it's possible, just use the click counter to get the triple click and send
the event. I did not find any layout test treating that.

> 2) if in the same test we have a single click event followed by a dl click
> event (or in the reverse order). would the patch work ?
> 

I'm pretty sure it will work if a really single click be sent

> if answer is good for both question, i think it is robust enough :)

[Diego Gonzalez]

Created attachment 49824 [details]
Proposed patch

[Simon Hausmann]

Comment on attachment 49824 [details]
Proposed patch

r- because the m_timer should be stopped and resetClickCount() should be const, as discussed on IRC. the rest looks good to me :)

[Antonio Gomes]

(In reply to comment #15)
> (From update of attachment 49824 [details])
> r- because the m_timer should be stopped and resetClickCount() should be const,
> as discussed on IRC. the rest looks good to me :)

simon, I do not think resetClickCount should be 'const'. it is doing a assignmentt:

+    void resetClickCount() { m_clickCount = 0; }


did i miss something ?

[Diego Gonzalez]

Created attachment 49917 [details]
Proposed patch

Make the m_clickTimer stops as Simon requests. I agree with tonikitoo, in this case the const in void resetClickCount() { m_clickCount = 0; } maybe is not applicable.

[Simon Hausmann]

(In reply to comment #16)
> (In reply to comment #15)
> > (From update of attachment 49824 [details] [details])
> > r- because the m_timer should be stopped and resetClickCount() should be const,
> > as discussed on IRC. the rest looks good to me :)
> 
> simon, I do not think resetClickCount should be 'const'. it is doing a
> assignmentt:
> 
> +    void resetClickCount() { m_clickCount = 0; }
> 
> 
> did i miss something ?

You're right, I'm on crack :), there's no point in using const here. For a split second I thought it was an accessor.

[Simon Hausmann]

Comment on attachment 49917 [details]
Proposed patch

r+, but I realize an even simpler way of doing the timer is by not using a QObject timer at all but instead to simply use a cheap lightweight QTime object and instead of checking if the timer is active check if the elapsed() time since the start is less than the double click interval.

[Diego Gonzalez]

(In reply to comment #19)
> (From update of attachment 49917 [details])
> r+, but I realize an even simpler way of doing the timer is by not using a
> QObject timer at all but instead to simply use a cheap lightweight QTime object
> and instead of checking if the timer is active check if the elapsed() time
> since the start is less than the double click interval.

Thanks, Simon. I used QTime in previous patches but the patch only will work for evironments with Qt > 4.6, for this I opted to use QBasicTimer. :)

[Antonio Gomes]

Comment on attachment 49917 [details]
Proposed patch

(In reply to comment #20)
> (In reply to comment #19)
> > (From update of attachment 49917 [details] [details])
> > r+, but I realize an even simpler way of doing the timer is by not using a
> > QObject timer at all but instead to simply use a cheap lightweight QTime object
> > and instead of checking if the timer is active check if the elapsed() time
> > since the start is less than the double click interval.
> 
> Thanks, Simon. I used QTime in previous patches but the patch only will work
> for evironments with Qt > 4.6, for this I opted to use QBasicTimer. :)

that is actually a problem , because EWS-Qt build bot still lives w/ qt4.5 .

well, i'd prefer simon's approach, but ... cq+

[WebKit Commit Bot]

Comment on attachment 49917 [details]
Proposed patch

Clearing flags on attachment: 49917

Committed r55541: <http://trac.webkit.org/changeset/55541>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.