Bug 61425 – [Qt] Implement find feature for QtTestBrowser

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Kristóf Kosztyó]

[Qt] Implement find feature for QtTestBrowser

[Kristóf Kosztyó]

Created attachment 94963 [details]
proposed fix

[Kristóf Kosztyó]

Created attachment 94964 [details]
proposed fix

[Chang Shu]

Comment on attachment 94964 [details]
proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=94964&action=review

Disclaim: not a reviewer. :)

> Tools/QtTestBrowser/launcherwindow.cpp:1032
> +        page()->findText("", QFlag(8));

Is there an enum for 8?

> Tools/QtTestBrowser/launcherwindow.cpp:1045
> +    if (mode == BackWard)

if -> else if

> Tools/QtTestBrowser/launcherwindow.cpp:1047
> +    if (mode == CaseSensitively)

anyway, they should not be mutually exclusive. see further comments below.

> Tools/QtTestBrowser/launcherwindow.cpp:1051
> +    if (mode == HighLightAll)

HighLightAll should not be an exclusive value. It can be "OR"-ed to other values.

> Tools/QtTestBrowser/launcherwindow.h:252
> +                    WarpsAround,

typo

> Tools/QtTestBrowser/launcherwindow.h:254
> +    };

It doesn't seem to be right to use enums for these options (casesensitive, wrap, highlight) since they are not mutually exclusive. You should use separate flags or bit-masks.

[Andras Becsi]

Comment on attachment 94964 [details]
proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=94964&action=review

LGTM, just few comments.

> Tools/QtTestBrowser/launcherwindow.h:195
> +    void find(int mode);

This should be FindMode instead of int and you could set the default value here to Normal.

> Tools/QtTestBrowser/launcherwindow.h:248
> +    int m_findFlag;

This could be QWebPage::FindFlag instead of int.

> Tools/QtTestBrowser/launcherwindow.h:254
> +    enum FindMode { Normal,
> +                    BackWard,
> +                    CaseSensitively,
> +                    WarpsAround,
> +                    HighLightAll
> +    };

You could use QWebPage::FindFlag, and have a constant for Normal instead of an extra FindMode enum here.

[Csaba Osztrogonác]

Comment on attachment 94964 [details]
proposed fix

r- because of the comments and it needs to be updated to ToT to be appliable.

[Andras Becsi]

Comment on attachment 94964 [details]
proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=94964&action=review

> Tools/QtTestBrowser/launcherwindow.cpp:413
> +    m_findBar = new QToolBar("Find");

Also, this should be QToolBar("Find", this) since otherwise you might leak m_findBar.

[Kristóf Kosztyó]

Created attachment 95135 [details]
proposed fix

[Kristóf Kosztyó]

Thank you, that you viewed my patch 

With the enum you were right, I fixed it.

Let me explain how the method works:
When you type, or push the next or prev button it will find the text.
If you change the checkbox's setting the right flag will be set up in the m_findFlag with a bitwise XOR.
The HighLightAllOccurences is a special case, because when it set on, the find doesn't highlight the next founded text again so it has to make a new find without the HighLightAllOccurences flag to get the right result.

[Alexis Menard (darktears)]

Comment on attachment 95135 [details]
proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=95135&action=review

> Tools/QtTestBrowser/launcherwindow.cpp:4
> + * Copyright (C) 2011 Kristof Kosztyo <[e-mail]>

Perhaps you can squeeze in one line but no big deal.

> Tools/QtTestBrowser/launcherwindow.cpp:201
> +    editMenu->addAction("&Find", this, SLOT(showFindBar()), QKeySequence(Qt::CTRL | Qt::Key_F));

On Mac this is not the normal shortcuts. Command is.

> Tools/QtTestBrowser/launcherwindow.cpp:427
> +    findClose->setText("X");

Nitpick : Can you find a nice close icon on google (Creative Commons) or something? QtTestBrowser has to be awesome looking :D.

[Antonio Gomes]

Comment on attachment 95135 [details]
proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=95135&action=review

> Tools/QtTestBrowser/launcherwindow.cpp:1033
> +        page()->findText("", QWebPage::HighlightAllOccurrences);

some real browsers pre fill it with any selected content in the page. it could be a nice enhancement.

> Tools/QtTestBrowser/launcherwindow.cpp:1037
> +void LauncherWindow::find(int mode = m_findNormal)

looks ugly :)

> Tools/QtTestBrowser/launcherwindow.h:249
> +    static const int m_findNormal = 0;

could we have a better name?

[Chang Shu]

> > Tools/QtTestBrowser/launcherwindow.cpp:1037
> > +void LauncherWindow::find(int mode = m_findNormal)
> 
> looks ugly :)
> 
> > Tools/QtTestBrowser/launcherwindow.h:249
> > +    static const int m_findNormal = 0;
> 
> could we have a better name?

I think it's better to be "s_findNormal". m_ prefix indicates member variable.
Or maybe simply using 0.

Also, I still find the logic inside find() function a bit confusing. It seems page()->findText() maybe be triggered several times in certain cases. And the following line seems a typo (isn't ~m_findNormal == 1?):
found = page()->findText(m_lineEdit->text(), QFlag((m_findFlag | QWebPage::HighlightAllOccurrences) & ~m_findNormal));

Another thing I personally am not sure is the difference between LauncherWindow and MainWindow. It seems to be me the other toolbar is a member of MainWindow. Is there anything this implementation needs from LauncherWindow only?

[Csaba Osztrogonác]

(In reply to comment #10)
> (From update of attachment 95135 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95135&action=review
> 
> > Tools/QtTestBrowser/launcherwindow.cpp:1033
> > +        page()->findText("", QWebPage::HighlightAllOccurrences);
> 
> some real browsers pre fill it with any selected content in the page. it could be a nice enhancement.

:))

It is a good idea. But do we really want to make QtTestBrowser the best browser in this bug? :)

If it is a simple fix, let's do it now. But if 
it isn't so trivial, fix it in a new bug report.

[Kristóf Kosztyó]

> I think it's better to be "s_findNormal". m_ prefix indicates member variable.
> Or maybe simply using 0.

I will cahnge it. 


> Also, I still find the logic inside find() function a bit confusing. It seems page()->findText() maybe be triggered several times in certain cases. And the following line seems a typo (isn't ~m_findNormal == 1?):
> found = page()->findText(m_lineEdit->text(), QFlag((m_findFlag | QWebPage::HighlightAllOccurrences) & ~m_findNormal));

honestly I don't know why I did need to make like this, but it really doesn't needed I will delete that.


> Another thing I personally am not sure is the difference between LauncherWindow and MainWindow. It seems to be me the other toolbar is a member of MainWindow. Is there anything this implementation needs from LauncherWindow only?

The menu assembled here, it seems logical to place here.

[Chang Shu]

> > Another thing I personally am not sure is the difference between LauncherWindow and MainWindow. It seems to be me the other toolbar is a member of MainWindow. Is there anything this implementation needs from LauncherWindow only?
> 
> The menu assembled here, it seems logical to place here.

Thanks, good to know.:)

[Antonio Gomes]

(In reply to comment #12)
> (In reply to comment #10)
> > (From update of attachment 95135 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=95135&action=review
> > 
> > > Tools/QtTestBrowser/launcherwindow.cpp:1033
> > > +        page()->findText("", QWebPage::HighlightAllOccurrences);
> > 
> > some real browsers pre fill it with any selected content in the page. it could be a nice enhancement.
> 
> :))
> 
> It is a good idea. But do we really want to make QtTestBrowser the best browser in this bug? :)

No no, unless you want to do it yourself :)

> If it is a simple fix, let's do it now. But if 
> it isn't so trivial, fix it in a new bug report.

Sure. Not even needed, just an idea if one likes to play with it...

[Kristóf Kosztyó]

Created attachment 95328 [details]
proposed fix

[Andras Becsi]

Comment on attachment 95328 [details]
proposed fix

I think the patch is OK now.
A reviewer will set the r flag for you.

[Csaba Osztrogonác]

Comment on attachment 95328 [details]
proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=95328&action=review

typo: findWarpAround -> findWrapAround

> Tools/QtTestBrowser/launcherwindow.cpp:427
> +    m_lineEdit = new QLineEdit(m_findBar);
> +    m_lineEdit->setMaximumWidth(200);
> +    QCheckBox* findCaseSensitive = new QCheckBox("CaseSensitive", m_findBar);
> +    QCheckBox* findWarpAround = new QCheckBox("WarpAround", m_findBar);
> +    QCheckBox* findHighLightAll = new QCheckBox("HighLightAll", m_findBar);
> +    QToolButton* findNext = new QToolButton(m_findBar);
> +    findNext->setArrowType(Qt::RightArrow);
> +    QToolButton* findPrevious = new QToolButton(m_findBar);
> +    findPrevious->setArrowType(Qt::LeftArrow);
> +    QToolButton* findClose = new QToolButton(m_findBar);
> +    findClose->setText("X");

It would be great to keep the natural order here too:
findClose, m_lineEdit, findPrevious, findNext, findCaseSensitive, findWrapAround, findHighLightAll

[Andras Becsi]

Comment on attachment 95328 [details]
proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=95328&action=review

Took another look. I think the order and typo could be fixed during landing, but I have a few questions:

> Tools/QtTestBrowser/launcherwindow.cpp:1040
> +void LauncherWindow::find(int mode = s_findNormalFlag)
> +{
> +    page()->findText("", QFlag(QWebPage::HighlightAllOccurrences));

Do we always need to call this? Is this for clearing the previous search?

> Tools/QtTestBrowser/launcherwindow.cpp:1053
> +    if (mode == s_findNormalFlag)
> +        page()->findText(m_lineEdit->text(), QFlag(m_findFlag & ~QWebPage::HighlightAllOccurrences));
> +    else if (mode == QWebPage::FindBackward)
> +        page()->findText(m_lineEdit->text(), QFlag((m_findFlag | QWebPage::FindBackward) & ~QWebPage::HighlightAllOccurrences));
> +    else
> +        m_findFlag = m_findFlag ^ mode;
> +
> +    if (!m_lineEdit->text().isEmpty())
> +        found = page()->findText(m_lineEdit->text(), QFlag(m_findFlag | QWebPage::HighlightAllOccurrences));

This seems to be needlessly complicated. Do we need all these findText calls for highlighting to work properly? Couldn't we just combine the proper find flags in the conditional cases and call findText once (or twice in case of HighlightAllOccurrences)?

[Kristóf Kosztyó]

Created attachment 95407 [details]
proposed fix

[Andras Becsi]

Comment on attachment 95407 [details]
proposed fix

Great work. LGTM, Ossy what do you think?

[Csaba Osztrogonác]

Comment on attachment 95407 [details]
proposed fix

Great patch, r=me.

[WebKit Commit Bot]

Comment on attachment 95407 [details]
proposed fix

Clearing flags on attachment: 95407

Committed r87720: <http://trac.webkit.org/changeset/87720>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ademar Reis]

Revision r87720 cherry-picked into qtwebkit-2.2 with commit af58e95 <http://gitorious.org/webkit/qtwebkit/commit/af58e95>

[Ademar Reis]

"Buildfix for --minimal and CONFIG+=qt_minimal build" commited in r87724

[Ademar Reis]

Revision r87724 cherry-picked into qtwebkit-2.2 with commit cbc9e96 <http://gitorious.org/webkit/qtwebkit/commit/cbc9e96>