Bug 35255 - [Qt] DRT: Send double click event from EventSender
Summary: [Qt] DRT: Send double click event from EventSender
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2010-02-22 12:16 PST by Diego Gonzalez
Modified: 2010-03-04 11:30 PST (History)
6 users (show)

See Also:


Attachments
Proposed patch (4.27 KB, patch)
2010-02-22 12:33 PST, Diego Gonzalez
no flags Details | Formatted Diff | Diff
Proposed patch (5.76 KB, patch)
2010-02-23 13:13 PST, Diego Gonzalez
no flags Details | Formatted Diff | Diff
Proposed patch (6.46 KB, patch)
2010-02-24 06:10 PST, Diego Gonzalez
no flags Details | Formatted Diff | Diff
Proposed patch (6.36 KB, patch)
2010-02-24 07:20 PST, Diego Gonzalez
no flags Details | Formatted Diff | Diff
Proposed patch (6.36 KB, patch)
2010-02-24 07:30 PST, Diego Gonzalez
no flags Details | Formatted Diff | Diff
Proposed patch (6.11 KB, patch)
2010-03-02 11:34 PST, Diego Gonzalez
hausmann: review-
Details | Formatted Diff | Diff
Proposed patch (6.45 KB, patch)
2010-03-03 09:35 PST, Diego Gonzalez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Diego Gonzalez 2010-02-22 12:16:30 PST
Make Qt DumpRenderTree EventSender able to send double click events
                                                                       
LayoutTests:                                                       
    fast/events/dblclick-addEventListener.html
Comment 1 Diego Gonzalez 2010-02-22 12:33:03 PST
Created attachment 49233 [details]
Proposed patch
Comment 2 WebKit Review Bot 2010-02-22 12:35:28 PST
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.
Comment 3 Diego Gonzalez 2010-02-23 13:13:34 PST
Created attachment 49319 [details]
Proposed patch
Comment 4 Kenneth Rohde Christiansen 2010-02-23 13:28:43 PST
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
Comment 5 Diego Gonzalez 2010-02-23 13:35:48 PST
(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
Comment 6 Antonio Gomes 2010-02-23 13:49:10 PST
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.
Comment 7 Diego Gonzalez 2010-02-24 06:10:26 PST
Created attachment 49384 [details]
Proposed patch
Comment 8 WebKit Review Bot 2010-02-24 06:19:23 PST
Attachment 49384 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/304469
Comment 9 Diego Gonzalez 2010-02-24 07:20:51 PST
Created attachment 49389 [details]
Proposed patch
Comment 10 Antonio Gomes 2010-02-24 07:25:16 PST
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 ...
Comment 11 Diego Gonzalez 2010-02-24 07:30:53 PST
Created attachment 49391 [details]
Proposed patch

Correcting comments. Thanks tonikitoo
Comment 12 Antonio Gomes 2010-02-24 07:48:23 PST
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 :)
Comment 13 Diego Gonzalez 2010-02-24 07:59:56 PST
(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 :)
Comment 14 Diego Gonzalez 2010-03-02 11:34:08 PST
Created attachment 49824 [details]
Proposed patch
Comment 15 Simon Hausmann 2010-03-03 06:32:37 PST
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 :)
Comment 16 Antonio Gomes 2010-03-03 08:56:16 PST
(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 ?
Comment 17 Diego Gonzalez 2010-03-03 09:35:57 PST
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.
Comment 18 Simon Hausmann 2010-03-03 12:33:15 PST
(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.
Comment 19 Simon Hausmann 2010-03-03 12:36:41 PST
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.
Comment 20 Diego Gonzalez 2010-03-03 12:50:45 PST
(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. :)
Comment 21 Antonio Gomes 2010-03-04 09:46:28 PST
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+
Comment 22 WebKit Commit Bot 2010-03-04 11:30:48 PST
Comment on attachment 49917 [details]
Proposed patch

Clearing flags on attachment: 49917

Committed r55541: <http://trac.webkit.org/changeset/55541>
Comment 23 WebKit Commit Bot 2010-03-04 11:30:54 PST
All reviewed patches have been landed.  Closing bug.