Bug 73560 – [Qt] [WK2] Support customizing popup menus with QML

• Component: New Bugs
• Status: RESOLVED FIXED

[Caio Marcelo de Oliveira Filho]

[Qt] [WK2] Support customizing popup menus with QML

[Caio Marcelo de Oliveira Filho]

Created attachment 117415 [details]
Patch

[Caio Marcelo de Oliveira Filho]

MiniBrowser doesn't show anything, but will work once patch for bug 73338 lands.

[Caio Marcelo de Oliveira Filho]

(In reply to comment #2)
> MiniBrowser doesn't show anything, but will work once patch for bug 73338 lands.

anything = any popup menu yet :-)

[Kenneth Rohde Christiansen]

Comment on attachment 117415 [details]
Patch

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

> Source/WebKit2/ChangeLog:10
> +        component used when a popup menu is shown. When loaded, this component will
> +        have available in its context the popupMenuModel object, that can be used

not very English :/ 

When loaded the component will have the popupMenuModel available in its context.

> Source/WebKit2/ChangeLog:17
> +        The existing Desktop version is removed since after the Qt5 refactoring isn't
> +        working correctly. Once Qt have its own QML components for popup, we hope to use
> +        it as a default if no other popupMenu is specified.

Sounds good

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:465
> +QDeclarativeComponent* QQuickWebViewExperimental::popupMenu() const

I dont like the name popup... It is not really a popup, that highly depends on the implementation. What about selectorMenu ? or similar. It comes from the select tag, which has options.

Maybe optionMenu or just OptionSelector? like FileSelector.

> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:53
> +    enum Roles {
> +        EnabledRole = Qt::UserRole,
> +        SelectedRole = Qt::UserRole + 1,
> +        IsSeparatorRole = Qt::UserRole + 2

How do we handle indentation? (grouped options)? Also what about multi selection? pre selections ? Did you check the grob webkit patches?

> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:117
> +    case IsSeparatorRole:

Why not just SeparatorRole?

[Caio Marcelo de Oliveira Filho]

Comment on attachment 117415 [details]
Patch

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

Thanks for the comments, Kenne.

>> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:465
>> +QDeclarativeComponent* QQuickWebViewExperimental::popupMenu() const
> 
> I dont like the name popup... It is not really a popup, that highly depends on the implementation. What about selectorMenu ? or similar. It comes from the select tag, which has options.
> 
> Maybe optionMenu or just OptionSelector? like FileSelector.

Maybe ItemSelector is better then? (like Grob branch)

>> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:53
>> +        IsSeparatorRole = Qt::UserRole + 2
> 
> How do we handle indentation? (grouped options)? Also what about multi selection? pre selections ? Did you check the grob webkit patches?

I mentioned grouped options in a FIXME, maybe it's better to just go forward and implement in this patch (since it would only involve Qt code). :-)

I think we could first move to the QML supporting what we have now, then start upstreaming multiple selection support (that Grob branch have) in a separate patch.

>> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:117
>> +    case IsSeparatorRole:
> 
> Why not just SeparatorRole?

In QML usage, I though model.isSeparator was better than model.separator.

[Kenneth Rohde Christiansen]

(In reply to comment #5)
> (From update of attachment 117415 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117415&action=review
> 
> Thanks for the comments, Kenne.
> 
> >> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:465
> >> +QDeclarativeComponent* QQuickWebViewExperimental::popupMenu() const
> > 
> > I dont like the name popup... It is not really a popup, that highly depends on the implementation. What about selectorMenu ? or similar. It comes from the select tag, which has options.
> > 
> > Maybe optionMenu or just OptionSelector? like FileSelector.
> 
> Maybe ItemSelector is better then? (like Grob branch)

Yeah, I kind of like that
 
> >> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:53
> >> +        IsSeparatorRole = Qt::UserRole + 2
> > 
> > How do we handle indentation? (grouped options)? Also what about multi selection? pre selections ? Did you check the grob webkit patches?
> 
> I mentioned grouped options in a FIXME, maybe it's better to just go forward and implement in this patch (since it would only involve Qt code). :-)
> 
> I think we could first move to the QML supporting what we have now, then start upstreaming multiple selection support (that Grob branch have) in a separate patch.

That is fine :-) I just wanted to make sure you considered it. Also preselections (already selected items).

> >> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:117
> >> +    case IsSeparatorRole:
> > 
> > Why not just SeparatorRole?
> 
> In QML usage, I though model.isSeparator was better than model.separator.

I agree :-)

[Tor Arne Vestbø]

Comment on attachment 117415 [details]
Patch

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

> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:221
> +#include "WebPopupMenuProxyQt.moc"
> +#include "moc_WebPopupMenuProxyQt.cpp"

You should only need one of these

[Caio Marcelo de Oliveira Filho]

Comment on attachment 117415 [details]
Patch

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

>> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:221
>> +#include "moc_WebPopupMenuProxyQt.cpp"
> 
> You should only need one of these

I have a Q_OBJECT in my .cpp file and another in my .h file. I thought the first moc inclusion would trigger the moc to run on cpp file, and the second would include the moc for the .h in our existing .cpp. Am I missing something?

[Caio Marcelo de Oliveira Filho]

Created attachment 117460 [details]
Patch

[Noam Rosenthal]

(In reply to comment #8)
> (From update of attachment 117415 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117415&action=review
> 
> >> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:221
> >> +#include "moc_WebPopupMenuProxyQt.cpp"
> > 
> > You should only need one of these
> 
> I have a Q_OBJECT in my .cpp file and another in my .h file. I thought the first moc inclusion would trigger the moc to run on cpp file, and the second would include the moc for the .h in our existing .cpp. Am I missing something?

Yes, the "moc_***" file is generated when you add your h file to HEADERS in the .pro file.

[Caio Marcelo de Oliveira Filho]

Comment on attachment 117415 [details]
Patch

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

>>>> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:221
>>>> +#include "moc_WebPopupMenuProxyQt.cpp"
>>> 
>>> You should only need one of these
>> 
>> I have a Q_OBJECT in my .cpp file and another in my .h file. I thought the first moc inclusion would trigger the moc to run on cpp file, and the second would include the moc for the .h in our existing .cpp. Am I missing something?
> 
> Yes, the "moc_***" file is generated when you add your h file to HEADERS in the .pro file.

I think these two includes are correct and necessary.

Including "WebPopupMenuProxyQt.moc" triggers the moc utility run on WebPopupMenuProxy.cpp -- we want that because we declare a QObject subclass in this source.

The moc utility will run in .h file in our case (as Noam pointed out, because of HEADERS), generating "moc_WebPopupMenuProxyQt.cpp".

If I stopped here, qmake would try to compile "moc_WebPopupMenuProxyQt.cpp" by itself, and it would fail, because it includes "WebPopupMenuProxyQt.h", that includes WebKit headers, but doesn't include "config.h" from WebKit (so no ENABLE() and USE() and PLATFORM() macros).

Another tricky (that qmake understands) is that if we do include "moc_XXX.cpp" on our source, it won't compile "moc_XXX.cpp" by itself. This is perfect, because our source file includes "config.h".

In summary: removing the first and we don't get extra stuff from QObject declared in the cpp file, removing the second and we end up compiling moc_XXX.cpp standalone, which doesn't work. I tested removing each one before uploading the last patch.

[Noam Rosenthal]

> If I stopped here, qmake would try to compile "moc_WebPopupMenuProxyQt.cpp" by itself, and it would fail, because it includes "WebPopupMenuProxyQt.h", that includes WebKit headers, but doesn't include "config.h" from WebKit (so no ENABLE() and USE() and PLATFORM() macros).
> 

Fair enough, though deserves a comment in code!

[Caio Marcelo de Oliveira Filho]

(In reply to comment #12)
> > If I stopped here, qmake would try to compile "moc_WebPopupMenuProxyQt.cpp" by itself, and it would fail, because it includes "WebPopupMenuProxyQt.h", that includes WebKit headers, but doesn't include "config.h" from WebKit (so no ENABLE() and USE() and PLATFORM() macros).
> > 
> 
> Fair enough, though deserves a comment in code!

Need some help writing this in a concise manner :-)

"The first include triggers moc to run on this cpp file -- and includes it (XXX.moc). The second indicates qmake
that the generated file for our header (moc_XXX) isn't compiled standalone. This is important because the generated
file doesn't include "config.h" but include WebKit headers."

Or maybe something like:

#include "XXX.moc" // Because we define QObjects in XXX.cpp
#include "moc_XXX.cpp" // Because generated file for header doesn't include "config.h"

Note that the second include is standard practice in our sources already.

[Noam Rosenthal]

> #include "XXX.moc" // Because we define QObjects in XXX.cpp
This one doesn't need a comment.

> #include "moc_XXX.cpp" // Because generated file for header doesn't include "config.h"
// To make sure moc uses the right defines that originate in config.h, we include the moc file here instead of in the project file HEADERS directive.

[Simon Hausmann]

Comment on attachment 117460 [details]
Patch

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

I love the API :)

> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:93
> +    Vector<Item> m_items;

Super nitpick that you're completely free to ignore and I'm just saying because I'm bored ;-)  :

You could declare vector typetraits for the item type, so that the vector can move the items efficiently. Another approach would be to use QVector and Q_DECLARE_TYPEINFO(Item, Q_MOVEABLE_TYPE) (IIRC)

[Tor Arne Vestbø]

(In reply to comment #12)
> > If I stopped here, qmake would try to compile "moc_WebPopupMenuProxyQt.cpp" by itself, and it would fail, because it includes "WebPopupMenuProxyQt.h", that includes WebKit headers, but doesn't include "config.h" from WebKit (so no ENABLE() and USE() and PLATFORM() macros).
> > 
> 
> Fair enough, though deserves a comment in code!

Ah, yepp, in that case you need both. Comment makes sense, thanks for adding.

[Tor Arne Vestbø]

Comment on attachment 117460 [details]
Patch

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

> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:47
> +

Add a property for the list of items here, as discussed.

I think we also want a int property initalSelection, that you then could map to ListView.selectedIndex if you visualize the items as a ListView.

> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:64
> +    void dismiss() { emit dismissed(); }

These can also probably be normal functions with Q_INVOCABLE?

> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:67
> +    void selectedIndex(int);

I don't think we need to keep track of this, as long as we have the initialSelection.

> Tools/MiniBrowser/qt/qml/ItemSelector.qml:63
> +                    onClicked: selectorModel.selectIndex(model.index)

The item should change the current listView selection. The list view should then hook into selectionChanged, and then submit the dialog. Some dialogs might implement it by requiring the user to press ok for example.

[Caio Marcelo de Oliveira Filho]

Created attachment 117887 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 117887 [details]
Patch

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

Looks good to me

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_itemSelector.qml:24
> +                    selectorModel.items.reject()

I wonder if rejectChanged/acceptChanges would be more clear

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/common/select.html:16
> +<option value="__open__" >Open</option>
> +<option value="__closed__" >Closed</option>
> +<option value="__all__" >All</option>

you python you :-)

> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:78
> +        { }
> +        QString text;

newline

> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:103
> +    ItemsModel* items() { return &m_items; }

ItemModel no? why the s?

[Caio Marcelo de Oliveira Filho]

Created attachment 118561 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Comment on attachment 117887 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_itemSelector.qml:24
>> +                    selectorModel.items.reject()
> 
> I wonder if rejectChanged/acceptChanges would be more clear

I rather stay with accept/reject since was the one discussed with Tor-Arne for this patch.

>> Source/WebKit2/UIProcess/API/qt/tests/qmltests/common/select.html:16
>> +<option value="__all__" >All</option>
> 
> you python you :-)

Hehe, just copied from webkit.org bugzilla search page :-)

>> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:78
>> +        QString text;
> 
> newline

Fixed.

>> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:103
>> +    ItemsModel* items() { return &m_items; }
> 
> ItemModel no? why the s?

Renamed to PopupMenuItemModel.

[Tor Arne Vestbø]

Comment on attachment 118561 [details]
Patch

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

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_itemSelector.qml:9
> +    property int shouldSelectIndexFirst

This sounds like a bool. can you find a better name?

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_itemSelector.qml:10
> +    property int shouldSelectIndex

same with this

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_itemSelector.qml:21
> +                    selectorModel.items.select(WebView.view.shouldSelectIndexFirst)

Now that we've split up the selector model and the items model, you can name the selector model "model"

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_itemSelector.qml:24
> +                    selectorModel.items.reject()

reject() and accept() should be on the popup model, not on the individual items

[Caio Marcelo de Oliveira Filho]

Created attachment 118591 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Comment on attachment 118561 [details]
Patch

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

The latest patch also fixes some problems I had in the testing. Now should be OK.

>> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_itemSelector.qml:9
>> +    property int shouldSelectIndexFirst
> 
> This sounds like a bool. can you find a better name?

Fixed.

>> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_itemSelector.qml:10
>> +    property int shouldSelectIndex
> 
> same with this

Fixed.

>> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_itemSelector.qml:21
>> +                    selectorModel.items.select(WebView.view.shouldSelectIndexFirst)
> 
> Now that we've split up the selector model and the items model, you can name the selector model "model"

Fixed.

>> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_itemSelector.qml:24
>> +                    selectorModel.items.reject()
> 
> reject() and accept() should be on the popup model, not on the individual items

Fixed.

[Caio Marcelo de Oliveira Filho]

Comment on attachment 118561 [details]
Patch

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

>>> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_itemSelector.qml:24
>>> +                    selectorModel.items.reject()
>> 
>> reject() and accept() should be on the popup model, not on the individual items
> 
> Fixed.

For the record, they weren't in individual items. They were "invokables" in our model that contained items. But per IRC discussion, I moved them to the "main model" of our item selector.

[Caio Marcelo de Oliveira Filho]

Committed r102573: <http://trac.webkit.org/changeset/102573>