Bug 32482 - [Qt] Add support for keyboard modifiers to TouchEvent
Summary: [Qt] Add support for keyboard modifiers to TouchEvent
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P3 Normal
Assignee: Nobody
URL:
Keywords: Qt
Depends on:
Blocks: 32484 32485
  Show dependency treegraph
 
Reported: 2009-12-13 02:34 PST by Simon Hausmann
Modified: 2009-12-14 12:57 PST (History)
3 users (show)

See Also:


Attachments
Patch (14.88 KB, patch)
2009-12-13 14:52 PST, Simon Hausmann
no flags Details | Formatted Diff | Diff
Patch with corrected coding style (14.86 KB, patch)
2009-12-13 14:55 PST, Simon Hausmann
no flags Details | Formatted Diff | Diff
Updated patch with better tests (15.26 KB, patch)
2009-12-13 23:51 PST, Simon Hausmann
no flags Details | Formatted Diff | Diff
Updated patch with indenting glitch in TouchEvent.idl fixed (15.22 KB, patch)
2009-12-13 23:53 PST, Simon Hausmann
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Hausmann 2009-12-13 02:34:56 PST
The iPhone's TouchEvent interface provides access to the keyboard modifiers as properties.

QTouchEvent has the same properties and it should be straight-forward to map those and test the mapping.

(This issue is part of http://bugreports.qt.nokia.com/browse/QTWEBKIT-20 )
Comment 1 Simon Hausmann 2009-12-13 14:52:53 PST
Created attachment 44766 [details]
Patch
Comment 2 Simon Hausmann 2009-12-13 14:55:38 PST
Created attachment 44767 [details]
Patch with corrected coding style
Comment 3 WebKit Review Bot 2009-12-13 14:56:54 PST
style-queue ran check-webkit-style on attachment 44767 [details] without any errors.
Comment 4 Holger Freyther 2009-12-13 19:51:07 PST
Comment on attachment 44767 [details]
Patch with corrected coding style

Do you plan to do dump the other modifiers in the test case too? It would be interesting to set two modifiers at the same time... mostly to test the bit setting, clearling logic in the Event Sender :)


>      : MouseRelatedEvent(type, true, true, view, 0, screenX, screenY, pageX, pageY,
> -                        false, false, false, false)
> +                        ctrlKey, altKey, shiftKey, metaKey)

maybe add isSimulated as well... that is the only missing one from MouseRelatedEvent...



> +            bool ctrlKey, bool altKey, bool shiftKey, bool metaKey);
>  
>      virtual bool isTo
> +        : m_type(TouchStart)
> +        , m_shiftKey(false)
> +        , m_ctrlKey(false)
> +        , m_altKey(false)
> +        , m_metaKey(false)
> +    {}


and in Qt you change the order... but I don't see an issue with it.

> +void EventSender::setTouchModifier(const QString &modifier, bool enable)
> +{
> +#if QT_VERSION >= QT_VERSION_CHECK(4, 6, 0)
> +    Qt::KeyboardModifier mod = Qt::NoModifier;
> +    if (!modifier.compare(QLatin1String("shift"), Qt::CaseInsensitive))
> +        mod = Qt::ShiftModifier;
> +    else if (!modifier.compare(QLatin1String("alt"), Qt::CaseInsensitive))
> +        mod = Qt::AltModifier;
> +    else if (!modifier.compare(QLatin1String("meta"), Qt::CaseInsensitive))
> +        mod = Qt::MetaModifier;
> +    else if (!modifier.compare(QLatin1String("ctrl"), Qt::CaseInsensitive))
> +        mod = Qt::ControlModifier;

I'm confused about the "!". Can you explain?
Comment 5 Simon Hausmann 2009-12-13 23:42:24 PST
(In reply to comment #4)
> (From update of attachment 44767 [details])
> Do you plan to do dump the other modifiers in the test case too? It would be
> interesting to set two modifiers at the same time... mostly to test the bit
> setting, clearling logic in the Event Sender :)

Ok, will do :)

> >      : MouseRelatedEvent(type, true, true, view, 0, screenX, screenY, pageX, pageY,
> > -                        false, false, false, false)
> > +                        ctrlKey, altKey, shiftKey, metaKey)
> 
> maybe add isSimulated as well... that is the only missing one from
> MouseRelatedEvent...

Hm, I could, but it would be completely unused AFAICS.

> > +            bool ctrlKey, bool altKey, bool shiftKey, bool metaKey);
> >  
> >      virtual bool isTo
> > +        : m_type(TouchStart)
> > +        , m_shiftKey(false)
> > +        , m_ctrlKey(false)
> > +        , m_altKey(false)
> > +        , m_metaKey(false)
> > +    {}
> 
> 
> and in Qt you change the order... but I don't see an issue with it.

Hehe, copy & paste. Will fix

> > +void EventSender::setTouchModifier(const QString &modifier, bool enable)
> > +{
> > +#if QT_VERSION >= QT_VERSION_CHECK(4, 6, 0)
> > +    Qt::KeyboardModifier mod = Qt::NoModifier;
> > +    if (!modifier.compare(QLatin1String("shift"), Qt::CaseInsensitive))
> > +        mod = Qt::ShiftModifier;
> > +    else if (!modifier.compare(QLatin1String("alt"), Qt::CaseInsensitive))
> > +        mod = Qt::AltModifier;
> > +    else if (!modifier.compare(QLatin1String("meta"), Qt::CaseInsensitive))
> > +        mod = Qt::MetaModifier;
> > +    else if (!modifier.compare(QLatin1String("ctrl"), Qt::CaseInsensitive))
> > +        mod = Qt::ControlModifier;
> 
> I'm confused about the "!". Can you explain?

QString::compare returns an int, similar to strcmp, which is zero when the strings are equal ;(
Comment 6 Simon Hausmann 2009-12-13 23:51:05 PST
Created attachment 44776 [details]
Updated patch with better tests
Comment 7 Simon Hausmann 2009-12-13 23:53:11 PST
Created attachment 44778 [details]
Updated patch with indenting glitch in TouchEvent.idl fixed
Comment 8 WebKit Review Bot 2009-12-13 23:56:16 PST
style-queue ran check-webkit-style on attachment 44778 [details] without any errors.
Comment 9 WebKit Commit Bot 2009-12-14 12:57:39 PST
Comment on attachment 44778 [details]
Updated patch with indenting glitch in TouchEvent.idl fixed

Clearing flags on attachment: 44778

Committed r52113: <http://trac.webkit.org/changeset/52113>
Comment 10 WebKit Commit Bot 2009-12-14 12:57:44 PST
All reviewed patches have been landed.  Closing bug.