Summary: | [Qt] The numbers are not displayed properly when entering through VKB in number mode | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kristian Amlie <kristian.amlie> | ||||||
Component: | WebKit Qt | Assignee: | QtWebKit Unassigned <webkit-qt-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | commit-queue, harry, hausmann, joseph.ligman, koshuin, sakari.peltomaki | ||||||
Priority: | P2 | Keywords: | Qt | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | PC | ||||||||
OS: | Linux | ||||||||
Attachments: |
|
Description
Kristian Amlie
2010-03-18 06:04:59 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 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? (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. (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! :) 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? *** Bug 33836 has been marked as a duplicate of this bug. *** (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. Created attachment 51142 [details]
Patch v2
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? (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 on attachment 51142 [details] Patch v2 Clearing flags on attachment: 51142 Committed r56322: <http://trac.webkit.org/changeset/56322> All reviewed patches have been landed. Closing bug. Cherry-picked into qtwebkit-4.6 with commit aa40cdb9595eb15a68e7be03322f973aa613a8f9 |