Bug 36292 - [Qt] The numbers are not displayed properly when entering through VKB in number mode
Summary: [Qt] The numbers are not displayed properly when entering through VKB in numb...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: QtWebKit Unassigned
URL:
Keywords: Qt
: 33836 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-03-18 06:04 PDT by Kristian Amlie
Modified: 2010-03-24 04:15 PDT (History)
6 users (show)

See Also:


Attachments
Patch (2.07 KB, patch)
2010-03-18 06:04 PDT, Kristian Amlie
hausmann: review-
hausmann: commit-queue-
Details | Formatted Diff | Diff
Patch v2 (3.08 KB, patch)
2010-03-19 04:06 PDT, Kristian Amlie
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kristian Amlie 2010-03-18 06:04:59 PDT
Created attachment 51020 [details]
Patch

Steps to reproduce: 

1. Compile this program:
int main(int argc, char **argv)
{
    QApplication app(argc, argv);
    QWidget w;

    QLayout *layout = new QVBoxLayout;

    QGraphicsScene scene;
    QGraphicsView view(&scene);
    QGraphicsWebView *wv = new QGraphicsWebView;
    wv->load(QUrl("http://www.google.com/"));
    scene.addItem(wv);
    layout->addWidget(&view);

    w.setLayout(layout);
    w.showMaximized();
    return app.exec();
}

2. Tap the lineedit so the input panel opens.
3. Enter number mode.
4. Try to enter numbers.

The result is that the numbers lag behind, that is, when entering "23", the first character will produce nothing, and the 3 will produce "2". Must be a QInputContext::update issue, since the content is correct in the widget itself.
Comment 1 Kristian Amlie 2010-03-18 07:17:47 PDT
Comment on attachment 51020 [details]
Patch

> diff --git a/src/3rdparty/webkit/WebKit/qt/Api/qgraphicswebview.cpp b/src/3rdparty/webkit/WebKit/qt/Api/qgraphicswebview.cpp
> index ceb5ee1..5786965 100644
> --- a/src/3rdparty/webkit/WebKit/qt/Api/qgraphicswebview.cpp
> +++ b/src/3rdparty/webkit/WebKit/qt/Api/qgraphicswebview.cpp
> @@ -30,6 +30,7 @@
>  #include <QtGui/qapplication.h>
>  #include <QtGui/qgraphicssceneevent.h>
>  #include <QtGui/qstyleoption.h>
> +#include <QtGui/qinputcontext.h>
>  #if defined(Q_WS_X11)
>  #include <QX11Info>
>  #endif
> @@ -63,6 +64,8 @@ public:
>  
>      void _q_doLoadFinished(bool success);
>  
> +    void _q_updateMicroFocus();
> +
>      QGraphicsWebView* q;
>      QWebPage* page;
>  };
> @@ -80,6 +83,18 @@ void QGraphicsWebViewPrivate::_q_doLoadFinished(bool success)
>      emit q->loadFinished(success);
>  }
>  
> +void QGraphicsWebViewPrivate::_q_updateMicroFocus()
> +{
> +#if !defined(QT_NO_IM) && (defined(Q_WS_X11) || defined(Q_WS_QWS) || defined(Q_OS_SYMBIAN))
> +    QList<QGraphicsView *> views = q->scene()->views();
> +    for (int c = 0; c < views.size(); ++c) {
> +        QInputContext *ic = views[c]->inputContext();
> +        if (ic)
> +            ic->update();
> +    }
> +#endif
> +}
> +
>  void QGraphicsWebViewPrivate::scroll(int dx, int dy, const QRect& rectToScroll)
>  {
>      q->scroll(qreal(dx), qreal(dy), QRectF(rectToScroll));
> @@ -435,6 +450,8 @@ void QGraphicsWebView::setPage(QWebPage* page)
>              this, SIGNAL(statusBarMessage(QString)));
>      connect(d->page, SIGNAL(linkClicked(QUrl)),
>              this, SIGNAL(linkClicked(QUrl)));
> +    connect(d->page, SIGNAL(microFocusChanged()),
> +            this, SLOT(_q_updateMicroFocus()));
>  }
>  
>  /*!
> diff --git a/src/3rdparty/webkit/WebKit/qt/Api/qgraphicswebview.h b/src/3rdparty/webkit/WebKit/qt/Api/qgraphicswebview.h
> index f3afb4c..f983ae4 100644
> --- a/src/3rdparty/webkit/WebKit/qt/Api/qgraphicswebview.h
> +++ b/src/3rdparty/webkit/WebKit/qt/Api/qgraphicswebview.h
> @@ -134,6 +134,7 @@ protected:
>  
>  private:
>      Q_PRIVATE_SLOT(d, void _q_doLoadFinished(bool success))
> +    Q_PRIVATE_SLOT(d, void _q_updateMicroFocus())
>  
>      QGraphicsWebViewPrivate* const d;
>      friend class QGraphicsWebViewPrivate;
Comment 2 Simon Hausmann 2010-03-18 07:21:53 PDT
Comment on attachment 51020 [details]
Patch

I think in principle this looks good, but I can see two things missing:

* A ChangeLog entry
* A comment indicating that this is a missing feature in QGraphicsItem, ideally with a link to an entry in the Qt JIRA tracking this missing feature. Then we can remove this code again once it's fixed in Qt.

> +#if !defined(QT_NO_IM) && (defined(Q_WS_X11) || defined(Q_WS_QWS) || defined(Q_OS_SYMBIAN))
> +    QList<QGraphicsView *> views = q->scene()->views();

Coding style, no space before the '*'

> +    for (int c = 0; c < views.size(); ++c) {
> +        QInputContext *ic = views[c]->inputContext();

I think you may want to use views.at(c) instead of [], to avoid detaching the QList that the scene returns.

> +        if (ic)
> +            ic->update();

Shouldn't this be a call to views.at(c)->updateMicroFocus() instead?
Comment 3 Kristian Amlie 2010-03-18 08:14:38 PDT
(In reply to comment #2)
> * A comment indicating that this is a missing feature in QGraphicsItem, ideally
> with a link to an entry in the Qt JIRA tracking this missing feature. Then we
> can remove this code again once it's fixed in Qt.

So what you are talking about is basically to have QGraphicsItem::updateMicroFocus()? I guess that makes sense, although it can't be a slot, because QGraphicsItem is not a QObject, so we would still need the extra slot in QGraphicsWebView.

QGraphicsTextItem currently also solves this by calling ic->update(), although it looks at qApp->focusWidget() only, which might be smarter.

> > +    for (int c = 0; c < views.size(); ++c) {
> > +        QInputContext *ic = views[c]->inputContext();
> 
> I think you may want to use views.at(c) instead of [], to avoid detaching the
> QList that the scene returns.

I don't think it detaches as long as the called function is also const, but in the sake of clarity, you're right. :-)

> > +        if (ic)
> > +            ic->update();
> 
> Shouldn't this be a call to views.at(c)->updateMicroFocus() instead?

Would be nice, but updateMicroFocus() is protected.
Comment 4 Simon Hausmann 2010-03-18 08:46:14 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > * A comment indicating that this is a missing feature in QGraphicsItem, ideally
> > with a link to an entry in the Qt JIRA tracking this missing feature. Then we
> > can remove this code again once it's fixed in Qt.
> 
> So what you are talking about is basically to have
> QGraphicsItem::updateMicroFocus()? I guess that makes sense, although it can't
> be a slot, because QGraphicsItem is not a QObject, so we would still need the
> extra slot in QGraphicsWebView.

Right. It could be turned into a slot in QGraphicsObject, btw. Then we could also use it. But either way I think this should be filed in the Qt bug tracker and we should reference it here.

> QGraphicsTextItem currently also solves this by calling ic->update(), although
> it looks at qApp->focusWidget() only, which might be smarter.

That's a good argument in favour of fixing it really in QGraphicsItem :-)
 
> > > +    for (int c = 0; c < views.size(); ++c) {
> > > +        QInputContext *ic = views[c]->inputContext();
> > 
> > I think you may want to use views.at(c) instead of [], to avoid detaching the
> > QList that the scene returns.
> 
> I don't think it detaches as long as the called function is also const, but in
> the sake of clarity, you're right. :-)

The variable is local, so why would the compiler choose the non-const [] overload? The constness of the currect function doesn't change that.
 
> > > +        if (ic)
> > > +            ic->update();
> > 
> > Shouldn't this be a call to views.at(c)->updateMicroFocus() instead?
> 
> Would be nice, but updateMicroFocus() is protected.

Dang! :)
Comment 5 Simon Hausmann 2010-03-18 15:15:49 PDT
This is tracked upstream at http://bugreports.qt.nokia.com/browse/QTBUG-7578 . Kristian, can you add a link to that as a comment in the code?
Comment 6 Simon Hausmann 2010-03-18 15:16:44 PDT
*** Bug 33836 has been marked as a duplicate of this bug. ***
Comment 7 Kristian Amlie 2010-03-19 03:19:52 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > QGraphicsTextItem currently also solves this by calling ic->update(), although
> > it looks at qApp->focusWidget() only, which might be smarter.
> 
> That's a good argument in favour of fixing it really in QGraphicsItem :-)

Indeed. I'll leave that in the task you mentioned for now, since this needs to go into 4.6 as well.

> > > > +    for (int c = 0; c < views.size(); ++c) {
> > > > +        QInputContext *ic = views[c]->inputContext();
> > > 
> > > I think you may want to use views.at(c) instead of [], to avoid detaching the
> > > QList that the scene returns.
> > 
> > I don't think it detaches as long as the called function is also const, but in
> > the sake of clarity, you're right. :-)
> 
> The variable is local, so why would the compiler choose the non-const []
> overload? The constness of the currect function doesn't change that.

You're right, I was assuming that inputContext() was a const function, but it isn't, so the compiler would indeed choose the non-const operator[].

I'll get a modified patch up shortly.
Comment 8 Kristian Amlie 2010-03-19 04:06:06 PDT
Created attachment 51142 [details]
Patch v2
Comment 9 Simon Hausmann 2010-03-19 06:35:27 PDT
Comment on attachment 51142 [details]
Patch v2


> +void QGraphicsWebViewPrivate::_q_updateMicroFocus()
> +{
> +#if !defined(QT_NO_IM) && (defined(Q_WS_X11) || defined(Q_WS_QWS) || defined(Q_OS_SYMBIAN))
> +    // Ideally, this should be handled by a common call to an updateMicroFocus function
> +    // in QGraphicsItem. See http://bugreports.qt.nokia.com/browse/QTBUG-7578.

Why is this only enabled for X11/QWS/Symbian?
Comment 10 Kristian Amlie 2010-03-19 08:21:31 PDT
(In reply to comment #9)
> (From update of attachment 51142 [details])
> 
> > +void QGraphicsWebViewPrivate::_q_updateMicroFocus()
> > +{
> > +#if !defined(QT_NO_IM) && (defined(Q_WS_X11) || defined(Q_WS_QWS) || defined(Q_OS_SYMBIAN))
> > +    // Ideally, this should be handled by a common call to an updateMicroFocus function
> > +    // in QGraphicsItem. See http://bugreports.qt.nokia.com/browse/QTBUG-7578.
> 
> Why is this only enabled for X11/QWS/Symbian?

I copied it from QWidget::updateMicroFocus().
Comment 11 WebKit Commit Bot 2010-03-21 16:21:12 PDT
Comment on attachment 51142 [details]
Patch v2

Clearing flags on attachment: 51142

Committed r56322: <http://trac.webkit.org/changeset/56322>
Comment 12 WebKit Commit Bot 2010-03-21 16:21:17 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Simon Hausmann 2010-03-24 04:15:47 PDT
Cherry-picked into qtwebkit-4.6 with commit aa40cdb9595eb15a68e7be03322f973aa613a8f9