[PATCH] Implement drop down menu for comboboxes in Snowshoe desktop.

[Alexis Menard]

It has scrollbars and support sections.

Reviewed-by: NOBODY
---
src/core/PopupWindow.cpp | 4 --
src/desktop/BrowserWindow.cpp | 13 ++++
src/desktop/BrowserWindow.h | 1 +
src/desktop/qml/BookmarkBar.qml | 1 +
src/desktop/qml/DropDownMenuBookmarkDelegate.qml | 2 +-
src/desktop/qml/ItemSelector.qml | 87 ++++++++++++++++++++++++
src/desktop/qml/ItemSelectorDelegate.qml | 64 +++++++++++++++++
src/desktop/qml/PageWidget.qml | 6 ++
src/desktop/qml/ScrollIndicator.qml | 71 +++++++++++++++++++
src/desktop/snowshoe.qrc | 3 +
10 files changed, 247 insertions(+), 5 deletions(-)
create mode 100644 src/desktop/qml/ItemSelector.qml
create mode 100644 src/desktop/qml/ItemSelectorDelegate.qml
create mode 100644 src/desktop/qml/ScrollIndicator.qml

diff --git a/src/core/PopupWindow.cpp b/src/core/PopupWindow.cpp
index e440fe4..6978b56 100644
--- a/src/core/PopupWindow.cpp
+++ b/src/core/PopupWindow.cpp
@@ -37,14 +37,10 @@ void PopupWindow::showEvent(QShowEvent* ev)
QTimer::singleShot(0, this, SLOT(setMouseGrab()));
}

-// FIXME: I couldn't make the PopupWindow show its contents when hidden then shown again.
-// So we now discard during hideEvent. This is convenient if we want to dismiss the window.
-// See https://bugreports.qt.nokia.com/browse/QTBUG-23571.
void PopupWindow::hideEvent(QHideEvent* ev)
{
QQuickCanvas::hideEvent(ev);
setMouseGrabEnabled(false);
- deleteLater();
}

void PopupWindow::mousePressEvent(QMouseEvent* ev)
diff --git a/src/desktop/BrowserWindow.cpp b/src/desktop/BrowserWindow.cpp
index 710adfa..f9c5122 100644
--- a/src/desktop/BrowserWindow.cpp
+++ b/src/desktop/BrowserWindow.cpp
@@ -50,6 +50,19 @@ BrowserWindow::BrowserWindow(const QStringList& arguments)
Q_ASSERT(m_browserView);
}

+QPoint BrowserWindow::ensureInsideScreen(int x, int y, int width, int height)
+{
+ QPoint newPos(x, y);
+ QScreen* currentScreen = screen();
+ int right = x + width;
+ int bottom = y + height;
+ if (right > currentScreen->availableGeometry().width())
+ newPos.setX(x - (right - currentScreen->availableGeometry().width()));
+ if (bottom > currentScreen->availableGeometry().height())
+ newPos.setY(y - (bottom - currentScreen->availableGeometry().height()));
+ return newPos;
+}
+
QPoint BrowserWindow::mapToGlobal(int x, int y)
{
return QWindow::mapToGlobal(QPoint(x, y));
diff --git a/src/desktop/BrowserWindow.h b/src/desktop/BrowserWindow.h
index ec87432..3a68118 100644
--- a/src/desktop/BrowserWindow.h
+++ b/src/desktop/BrowserWindow.h
@@ -30,6 +30,7 @@ public:

QStringList urlsFromCommandLine() const { return m_urlsFromCommandLine; }

+ Q_INVOKABLE QPoint ensureInsideScreen(int x, int y, int width, int height);
Q_INVOKABLE QPoint mapToGlobal(int x, int y);
// FIXME: Move to appropriate object exposed to handle download.
Q_INVOKABLE QString decideDownloadPath(const QString& suggestedPath);
diff --git a/src/desktop/qml/BookmarkBar.qml b/src/desktop/qml/BookmarkBar.qml
index 00144d4..b2e8737 100644
--- a/src/desktop/qml/BookmarkBar.qml
+++ b/src/desktop/qml/BookmarkBar.qml
@@ -80,6 +80,7 @@ Item {
var point = mapToItem(tabWidget, x + width, height);
var globalPos = BrowserWindow.mapToGlobal(point.x - 200 + width, point.y);
var menu = dropDownMenuComponent.createObject();
+ globalPos = BrowserWindow.ensureInsideScreen(globalPos.x, globalPos.y, menu.width, menu.height);
menu.x = globalPos.x;
menu.y = globalPos.y;
menu.show();
diff --git a/src/desktop/qml/DropDownMenuBookmarkDelegate.qml b/src/desktop/qml/DropDownMenuBookmarkDelegate.qml
index 89bcf18..73875a2 100644
--- a/src/desktop/qml/DropDownMenuBookmarkDelegate.qml
+++ b/src/desktop/qml/DropDownMenuBookmarkDelegate.qml
@@ -33,7 +33,7 @@ Item {
Text {
id: text
font.pixelSize: 11
- text: name
+ text: model.name
elide: Text.ElideRight
anchors {
verticalCenter: parent.verticalCenter
diff --git a/src/desktop/qml/ItemSelector.qml b/src/desktop/qml/ItemSelector.qml
new file mode 100644
index 0000000..c153f20
--- /dev/null
+++ b/src/desktop/qml/ItemSelector.qml
@@ -0,0 +1,87 @@
+/****************************************************************************
+ * Copyright (C) 2011 Instituto Nokia de Tecnologia (INdT) *
+ * *
+ * This file may be used under the terms of the GNU Lesser *
+ * General Public License version 2.1 as published by the Free Software *
+ * Foundation and appearing in the file LICENSE.LGPL included in the *
+ * packaging of this file. Please review the following information to *
+ * ensure the GNU Lesser General Public License version 2.1 requirements *
+ * will be met: http://www.gnu.org/licenses/old-licenses/lgpl-2.1.html. *
+ * *
+ * This program is distributed in the hope that it will be useful, *
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of *
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the *
+ * GNU Lesser General Public License for more details. *
+ ****************************************************************************/
+
+import QtQuick 2.0
+import Snowshoe 1.0
+Item {
+ id : root
+ property var selectorModel: model
+ property var contentArea
+ property int maxHeight
+ property int margins: 8
+
+ PopupWindow {
+ id : popup
+
+ width: listView.width + bg.border.right + bg.border.left
+ height: listView.height + bg.border.top + bg.border.bottom + (root.margins * 2)
+
+ Component.onCompleted: {
+ var globalPos = BrowserWindow.mapToGlobal(selectorModel.elementRect.x - bg.border.left, contentArea.y + selectorModel.elementRect.y + selectorModel.elementRect.height);
+ globalPos = BrowserWindow.ensureInsideScreen(globalPos.x, globalPos.y, popup.width, popup.height);
+ popup.x = globalPos.x;
+ popup.y = globalPos.y;
+ popup.show();
+ }
+
+ onVisibleChanged : { if (!visible) selectorModel.reject(); }
+
+ BorderImage {
+ id: bg
+ anchors.fill: parent
+ source: "qrc:///combobox/base_bg"
+ border { left: 12; top: 6; right: 12; bottom: 16 }
+ }
+
+ ListView {
+ id: listView
+ property int elementHeight: 24
+
+ x: bg.x + bg.border.left
+ y: bg.y + bg.border.top + root.margins
+ width: selectorModel.elementRect.width
+ clip: true
+ orientation: ListView.Vertical
+ model: selectorModel.items
+ spacing: 2
+
+ interactive: Boolean(height == root.maxHeight)
+ height: Math.min(contentItem.height, root.maxHeight);
+
+ delegate: ItemSelectorDelegate {
+ onClicked: selectorModel.accept(index)
+ }
+
+ section.property: "group"
+ section.delegate: Rectangle {
+ width: parent.width
+ height: listView.elementHeight
+ color: "silver"
+ Text {
+ anchors.centerIn: parent
+ text: section
+ font.pixelSize: 11
+ elide: Text.ElideRight
+ font.bold: true
+ }
+ }
+ }
+
+ ScrollIndicator {
+ flickableItem : listView
+ }
+ }
+}
diff --git a/src/desktop/qml/ItemSelectorDelegate.qml b/src/desktop/qml/ItemSelectorDelegate.qml
new file mode 100644
index 0000000..7bdc0c7
--- /dev/null
+++ b/src/desktop/qml/ItemSelectorDelegate.qml
@@ -0,0 +1,64 @@
+/****************************************************************************
+ * Copyright (C) 2011 Instituto Nokia de Tecnologia (INdT) *
+ * *
+ * This file may be used under the terms of the GNU Lesser *
+ * General Public License version 2.1 as published by the Free Software *
+ * Foundation and appearing in the file LICENSE.LGPL included in the *
+ * packaging of this file. Please review the following information to *
+ * ensure the GNU Lesser General Public License version 2.1 requirements *
+ * will be met: http://www.gnu.org/licenses/old-licenses/lgpl-2.1.html. *
+ * *
+ * This program is distributed in the hope that it will be useful, *
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of *
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the *
+ * GNU Lesser General Public License for more details. *
+ ****************************************************************************/
+
+import QtQuick 2.0
+
+Item {
+ signal clicked(var index)
+
+ width: listView.width
+ height: listView.elementHeight
+
+ BorderImage {
+ anchors.fill: parent
+ id: overlay
+ source: "qrc:///combobox/item_over_bg"
+ border { left: 2; top: 2; right: 2; bottom: 2 }
+ visible: false
+ }
+
+ Text {
+ id: text
+ font.pixelSize: 11
+ text: model.text
+ elide: Text.ElideRight
+ anchors {
+ verticalCenter: parent.verticalCenter
+ left: parent.left
+ leftMargin: 8
+ rightMargin: 8
+ right: parent.right
+ }
+ onWidthChanged: {
+ if (width > listView.width)
+ listView.width = width
+ }
+
+ }
+
+ MouseArea {
+ anchors.fill: parent
+ hoverEnabled: true
+ onEntered: {
+ overlay.visible = true;
+ }
+ onExited: {
+ overlay.visible = false;
+ }
+ enabled: model.enabled
+ onClicked: parent.clicked(model.index)
+ }
+}
diff --git a/src/desktop/qml/PageWidget.qml b/src/desktop/qml/PageWidget.qml
index df2e148..6d4a81e 100644
--- a/src/desktop/qml/PageWidget.qml
+++ b/src/desktop/qml/PageWidget.qml
@@ -81,6 +81,12 @@ Item {

experimental.preferences.fullScreenEnabled: true
experimental.preferences.developerExtrasEnabled: true
+ experimental.itemSelector: ItemSelector {
+ contentArea : root.parent
+ // FIXME: We should be able to use Screen.height from QtQuick.Window to
+ // calculate this but it isn't working yet.
+ maxHeight: 600
+ }

experimental.onDownloadRequested: {
downloadItem.destinationPath = BrowserWindow.decideDownloadPath(downloadItem.suggestedFilename)
diff --git a/src/desktop/qml/ScrollIndicator.qml b/src/desktop/qml/ScrollIndicator.qml
new file mode 100644
index 0000000..79df35a
--- /dev/null
+++ b/src/desktop/qml/ScrollIndicator.qml
@@ -0,0 +1,71 @@
+/****************************************************************************
+ * Copyright (C) 2011 Instituto Nokia de Tecnologia (INdT) *
+ * *
+ * This file may be used under the terms of the GNU Lesser *
+ * General Public License version 2.1 as published by the Free Software *
+ * Foundation and appearing in the file LICENSE.LGPL included in the *
+ * packaging of this file. Please review the following information to *
+ * ensure the GNU Lesser General Public License version 2.1 requirements *
+ * will be met: http://www.gnu.org/licenses/old-licenses/lgpl-2.1.html. *
+ * *
+ * This program is distributed in the hope that it will be useful, *
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of *
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the *
+ * GNU Lesser General Public License for more details. *
+ ****************************************************************************/
+
+import QtQuick 2.0
+Item {
+ id : root
+ property Flickable flickableItem
+ property int hideTimeout: 600
+ property real size: 4
+
+ anchors.fill: flickableItem
+
+ Item {
+ id: verticalIndicator
+ opacity: 0
+ width: root.size
+ height: flickableItem.height - size
+
+ anchors.right: root.right
+
+ Rectangle {
+ x: 0
+ y: Math.min(parent.height - height, Math.max(0, flickableItem.visibleArea.yPosition * verticalIndicator.height))
+
+ radius: 10
+ color: "black"
+ border.color: "gray"
+ border.width: 1
+ opacity: 0.5
+ smooth: true;
+
+ width: root.size
+ height: flickableItem.visibleArea.heightRatio * verticalIndicator.height;
+ }
+
+ states: [
+ State {
+ name: "active"
+ when: flickableItem.movingVertically
+ PropertyChanges {
+ target: verticalIndicator
+ opacity: 1
+ }
+ }
+ ]
+
+ transitions: [
+ Transition {
+ NumberAnimation {
+ target: verticalIndicator
+ properties: "opacity"
+ duration: hideTimeout
+ }
+ }
+ ]
+ }
+
+}
diff --git a/src/desktop/snowshoe.qrc b/src/desktop/snowshoe.qrc
index 5209f57..114117f 100644
--- a/src/desktop/snowshoe.qrc
+++ b/src/desktop/snowshoe.qrc
@@ -68,5 +68,8 @@
<file alias="urlbar/btn_nav_refresh_disable">images/urlbar/component_btn_nav_refresh_disable.png</file>
<file alias="pagewidget/link_status_bg_left">images/pagewidget/link_status_bg_left.png</file>
<file alias="pagewidget/link_status_bg_right">images/pagewidget/link_status_bg_right.png</file>
+ <file>qml/ItemSelector.qml</file>
+ <file>qml/ItemSelectorDelegate.qml</file>
+ <file>qml/ScrollIndicator.qml</file>
</qresource>
</RCC>
--
1.7.11

[Rafael Brandao]

Nice, just check the comments before landing.

I understand what this is doing, but are you really ensuring it is completely inside the screen? What if the screen is smaller than the dropdown menu? The initial position will be in negative positions, right? The question is, should we care about this? Just pointing out a possible issue. ;-)

 

+{
+    QPoint newPos(x, y);
+    QScreen* currentScreen = screen();

Keep currentScreen->availableGeometry() instead, you use it many times in the rest of this function.

If height and maxHeight have the same type, you should use "===" instead.

--
Rafael Brandao @ INdT

[Alexis Menard]

Let's be realistic. An item so long that's it's larger than the
screen. Regardless any algorithm it would be impossible to show it in
a way user will be able to use it correctly, it is a broken website.

On a side note : Qt4 has been around for 7 years, the popup placing
algorithm never took care of such issue, nobody ever complained about
it.

> The question is, should we care about this? Just pointing out a possible
> issue. ;-)

Theory Vs Reality :)

>
>>
>> +{
>> +    QPoint newPos(x, y);
>> +    QScreen* currentScreen = screen();
>
>
> Keep currentScreen->availableGeometry() instead, you use it many times in
> the rest of this function.

Good point already modified.

Thanks.

--
Alexis Menard (darktears)
Software Engineer
openBossa @ INdT - Instituto Nokia de Tecnologia

[Rafael Brandao]

Not theory only, google chrome does handle this. But on their case the popup is not inside the screen, but on top of it, so I think we should not worry about this on qt.

[Caio Marcelo de Oliveira Filho]

r=me with comments, pick the ones you like and pick the style ones pleeease.

> -// FIXME: I couldn't make the PopupWindow show its contents when hidden then shown again.
> -// So we now discard during hideEvent. This is convenient if we want to dismiss the window.
> -// See https://bugreports.qt.nokia.com/browse/QTBUG-23571.
> void PopupWindow::hideEvent(QHideEvent* ev)
> {
> QQuickCanvas::hideEvent(ev);
> setMouseGrabEnabled(false);
> - deleteLater();
> }

By reading the code again it seems this doesn't affect us anymore
because the itemSelector is destroyed by the WebView everytime is
hidden already. Is that correct? Maybe a note on the commit message
could be useful...

> +QPoint BrowserWindow::ensureInsideScreen(int x, int y, int width, int height)
> +{
> + QPoint newPos(x, y);
> + QScreen* currentScreen = screen();
> + int right = x + width;
> + int bottom = y + height;
> + if (right > currentScreen->availableGeometry().width())
> + newPos.setX(x - (right - currentScreen->availableGeometry().width()));
> + if (bottom > currentScreen->availableGeometry().height())
> + newPos.setY(y - (bottom - currentScreen->availableGeometry().height()));

I think this would be clearer with two local variables screenWidth and screenHeight.
For fun, you could use newPos.rx() and newPos.ry() as well :)

> + * Copyright (C) 2011 Instituto Nokia de Tecnologia (INdT) *

2012, same thing for the others.

> + id : root

id: root

> + property var selectorModel: model
> + property var contentArea
> + property int maxHeight
> + property int margins: 8
> +
> + PopupWindow {
> + id : popup
> +
> + width: listView.width + bg.border.right + bg.border.left
> + height: listView.height + bg.border.top + bg.border.bottom + (root.margins * 2)
> +
> + Component.onCompleted: {
> + var globalPos = BrowserWindow.mapToGlobal(selectorModel.elementRect.x - bg.border.left, contentArea.y + selectorModel.elementRect.y + selectorModel.elementRect.height);
> + globalPos = BrowserWindow.ensureInsideScreen(globalPos.x, globalPos.y, popup.width, popup.height);

By your comment later, Screen is broken. But if it was working, could
we get rid of ensureInsideScreen in C++ and bring this code here? If
so, one approach would be expose screenHeight and screenWidth with a
note to use Screen object in the future...

> +Item {
> + signal clicked(var index)

I think we can use int here instead of var.

> + width: listView.width
> + height: listView.elementHeight

These references to listView here assume that there'll be a listView
set somewhere in the creation context of this component.

The "right way" to get the corresponding ListView for a delegate is to
use the ListView.view attached property. Note that it is an attached
property of the topmost element of the delegate component...

> + Text {
> + id: text
> + font.pixelSize: 11
> + text: model.text
> + elide: Text.ElideRight
> + anchors {
> + verticalCenter: parent.verticalCenter
> + left: parent.left
> + leftMargin: 8
> + rightMargin: 8
> + right: parent.right
> + }
> + onWidthChanged: {
> + if (width > listView.width)

...So here you would probably write parent.ListView.view if I recall
correctly.

> + MouseArea {
> + anchors.fill: parent
> + hoverEnabled: true
> + onEntered: {
> + overlay.visible = true;
> + }
> + onExited: {
> + overlay.visible = false;
> + }

Couldn't we bound overlay's visible property to the mouse area contaisMouse property?

> + experimental.itemSelector: ItemSelector {
> + contentArea : root.parent

contentArea: root.parent

> + // FIXME: We should be able to use Screen.height from QtQuick.Window to
> + // calculate this but it isn't working yet.
> + maxHeight: 600

There was a comment above to expose screenHeight and screenWidth
instead of having the ensure* function. It could be useful here too.


> +import QtQuick 2.0
> +Item {

Missing blank line.

> + id : root

id: root

> + states: [
> + State {
> + name: "active"
> + when: flickableItem.movingVertically
> + PropertyChanges {
> + target: verticalIndicator
> + opacity: 1
> + }
> + }
> + ]
> +
> + transitions: [
> + Transition {
> + NumberAnimation {
> + target: verticalIndicator
> + properties: "opacity"
> + duration: hideTimeout
> + }
> + }
> + ]

One alternative here is to use a Behavior for verticalIndicator
opacity with this animation and use property binding for
opacity. States are more clear and explicit, but in this case, since
it's only one property in a small component a behavior could be fine. Your call. :-)


Cheers,

--
Caio Marcelo de Oliveira Filho

[Alexis Menard]

rx() is ry() is very confusing syntax, so I let it like this :).

>
>> + *   Copyright (C) 2011  Instituto Nokia de Tecnologia (INdT)               *
>
> 2012, same thing for the others.
>
>> +    id : root
>
> id: root
>
>> +    property var selectorModel: model
>> +    property var contentArea
>> +    property int maxHeight
>> +    property int margins: 8
>> +
>> +    PopupWindow {
>> +        id : popup
>> +
>> +        width: listView.width + bg.border.right + bg.border.left
>> +        height: listView.height + bg.border.top + bg.border.bottom + (root.margins * 2)
>> +
>> +        Component.onCompleted: {
>> +            var globalPos = BrowserWindow.mapToGlobal(selectorModel.elementRect.x - bg.border.left, contentArea.y + selectorModel.elementRect.y + selectorModel.elementRect.height);
>> +            globalPos = BrowserWindow.ensureInsideScreen(globalPos.x, globalPos.y, popup.width, popup.height);
>
> By your comment later, Screen is broken. But if it was working, could
> we get rid of ensureInsideScreen in C++ and bring this code here? If
> so, one approach would be expose screenHeight and screenWidth with a
> note to use Screen object in the future...

Yes I can add a comment to replace this later with Screen

>
>
>> +Item {
>> +    signal clicked(var index)
>
> I think we can use int here instead of var.
>
>> +    width: listView.width
>> +    height: listView.elementHeight
>
> These references to listView here assume that there'll be a listView
> set somewhere in the creation context of this component.

Yes it's the delegate tight to it.

>
> The "right way" to get the corresponding ListView for a delegate is to
> use the ListView.view attached property. Note that it is an attached
> property of the topmost element of the delegate component...

Ok modified

Done

>
>
>> +        experimental.itemSelector: ItemSelector {
>> +            contentArea : root.parent
>
> contentArea: root.parent
>
>> +            // FIXME: We should be able to use Screen.height from QtQuick.Window to
>> +            // calculate this but it isn't working yet.
>> +            maxHeight: 600
>
> There was a comment above to expose screenHeight and screenWidth
> instead of having the ensure* function. It could be useful here too.

Yep.

I will fix all of that in a separate patch when the Screen stuff work.

--
Alexis Menard (darktears)
Software Engineer