<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/124666/">https://git.reviewboard.kde.org/r/124666/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I agree that we need this kind of stacked user interaction and react in a sensible way to the back button. We need to be careful though which things are a "stack element" and which not. Generally it's better to avoid full-screen elements and deep stacking. In particular I perceive dialog-like elements not as stack elements. Therefore I wouldn't put the search field into the stack. Instead I'd really remove its toggle possibility at all and just show it always on top of the map. The search result list in turn (which is shown full-screen) might be a stack element though that gets popped upon pressing the back button, returning you to the map.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">For the route related elements I agree with the stacking behavior when using https://git.reviewboard.kde.org/r/124627/ as a base. I'm also fine with getting this ready for pushing to master this way. Allow me to bring up a comment from RR 124627 again though: "I wonder if we can reduce things also by merging the route editor and the profile selection buttons into one dialog that is not shown full-screen, but only on the lower ~half of the screen. It could be triggered by a button similar to the current profile selection button. This dialog could also hold the "delete route" button and similar actions." Using the main application menu for routing related actions is not very convenient in my opinion. From a user's point of view I'd prefer to have elements/actions from the same context close together. I also wonder whether having the list of directions as a fullscreen-element is a useful thing at all. It might be better to reduce them to a small element that is shown in turn-by-turn navigation mode only and allows to jump forward and backward as some kind of route-preview (e.g. the current instruction is shown, and some forward and backward button allow jumping to the next and previous instruction on the way).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">For the implementation I'd prefer using the QML StackView http://doc.qt.io/qt-5/qml-qtquick-controls-stackview.html instead of providing our own implementation. I'll attach a patch below. The interesting parts of the patch are:
- removes search field toggle possibility as discussed above
- replaces custom ItemStack with QtQuick Controls StackView
- changes its transition to keep items below visible. This is needed for the dialog-like navigationSettings (buttons to set search results as departure, waypoint or destination)
- removes anchors from stack elements as StackView wants to maintain them on its own</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%"><span style="color: #000080; font-weight: bold">diff --git a/src/apps/marble-maps/MainScreen.qml b/src/apps/marble-maps/MainScreen.qml</span>
<span style="color: #000080; font-weight: bold">index d54b94e..c06ba33 100644</span>
<span style="color: #A00000">--- a/src/apps/marble-maps/MainScreen.qml</span>
<span style="color: #00A000">+++ b/src/apps/marble-maps/MainScreen.qml</span>
<span style="color: #800080; font-weight: bold">@@ -26,26 +26,12 @@ ApplicationWindow {</span>
id: menuBar
Menu {
title: qsTr("Marble Maps")
<span style="color: #A00000">- MenuItem{</span>
<span style="color: #A00000">- text: qsTr("Search")</span>
<span style="color: #A00000">- onTriggered: {</span>
<span style="color: #A00000">- if (itemStack.isOnTop(search)) {</span>
<span style="color: #A00000">- itemStack.removeObject(search);</span>
<span style="color: #A00000">- }</span>
<span style="color: #A00000">- else {</span>
<span style="color: #A00000">- itemStack.addObject(search);</span>
<span style="color: #A00000">- itemStack.removeObject(instructions);</span>
<span style="color: #A00000">- itemStack.removeObject(waypointOrderEditor)</span>
<span style="color: #A00000">- }</span>
<span style="color: #A00000">- }</span>
<span style="color: #A00000">- }</span>
MenuItem {
text: qsTr("Delete Route")
onTriggered: {
routing.clearRoute();
<span style="color: #A00000">- itemStack.removeObject(instructions);</span>
<span style="color: #A00000">- itemStack.removeObject(waypointOrderEditor);</span>
<span style="color: #00A000">+ itemStack.pop(mapItem)</span>
startRoutingButton.show = false;
}
visible: routing.hasRoute
<span style="color: #800080; font-weight: bold">@@ -54,8 +40,8 @@ ApplicationWindow {</span>
MenuItem {
text: qsTr("Modify Route")
onTriggered: {
<span style="color: #A00000">- if (!itemStack.isOnTop(waypointOrderEditor)) {</span>
<span style="color: #A00000">- itemStack.addObject(waypointOrderEditor);</span>
<span style="color: #00A000">+ if (itemStack.currentItem !== waypointOrderEditor) {</span>
<span style="color: #00A000">+ itemStack.push(waypointOrderEditor);</span>
}
}
visible: routing.hasRoute
<span style="color: #800080; font-weight: bold">@@ -64,8 +50,8 @@ ApplicationWindow {</span>
MenuItem {
text: qsTr("Navigation Instructions")
onTriggered: {
<span style="color: #A00000">- if (!itemStack.isOnTop(instructions)) {</span>
<span style="color: #A00000">- itemStack.addObject(instructions)</span>
<span style="color: #00A000">+ if (itemStack.currentItem !== instructions) {</span>
<span style="color: #00A000">+ itemStack.push(instructions)</span>
}
}
visible: routing.hasRoute && !instructions.visible
<span style="color: #800080; font-weight: bold">@@ -74,8 +60,7 @@ ApplicationWindow {</span>
MenuItem {
text: qsTr("Show the Map")
onTriggered: {
<span style="color: #A00000">- itemStack.removeObject(instructions);</span>
<span style="color: #A00000">- itemStack.removeObject(waypointOrderEditor);</span>
<span style="color: #00A000">+ itemStack.pop(mapItem)</span>
}
visible: instructions.visible || waypointOrderEditor.visible
}
<span style="color: #800080; font-weight: bold">@@ -89,22 +74,35 @@ ApplicationWindow {</span>
width: 600
height: 400
<span style="color: #A00000">- ItemStack {</span>
<span style="color: #00A000">+ SystemPalette{</span>
<span style="color: #00A000">+ id: palette</span>
<span style="color: #00A000">+ colorGroup: SystemPalette.Active</span>
<span style="color: #00A000">+ }</span>
<span style="color: #00A000">+</span>
<span style="color: #00A000">+ Rectangle {</span>
<span style="color: #00A000">+ id: background</span>
<span style="color: #00A000">+ anchors.fill: parent</span>
<span style="color: #00A000">+ color: palette.window</span>
<span style="color: #00A000">+ }</span>
<span style="color: #00A000">+</span>
<span style="color: #00A000">+ StackView {</span>
id: itemStack
anchors.fill: parent
focus: true
<span style="color: #A00000">- SystemPalette{</span>
<span style="color: #A00000">- id: palette</span>
<span style="color: #A00000">- colorGroup: SystemPalette.Active</span>
<span style="color: #A00000">- }</span>
<span style="color: #00A000">+ initialItem: mapItem</span>
<span style="color: #A00000">- Rectangle {</span>
<span style="color: #A00000">- id: background</span>
<span style="color: #A00000">- anchors.fill: parent</span>
<span style="color: #A00000">- color: palette.window</span>
<span style="color: #00A000">+ delegate: StackViewDelegate {</span>
<span style="color: #00A000">+ function transitionFinished(properties)</span>
<span style="color: #00A000">+ {</span>
<span style="color: #00A000">+ properties.exitItem.visible = true</span>
<span style="color: #00A000">+ }</span>
}
<span style="color: #00A000">+ }</span>
<span style="color: #00A000">+</span>
<span style="color: #00A000">+ Item {</span>
<span style="color: #00A000">+ id: mapItem</span>
PinchArea {
anchors.fill: parent
<span style="color: #800080; font-weight: bold">@@ -224,7 +222,7 @@ ApplicationWindow {</span>
routing.addPositionAsDeparture();
}
else {
<span style="color: #A00000">- itemStack.addObject(navigationSettings);</span>
<span style="color: #00A000">+ itemStack.push(waypointActionDialog);</span>
navigation = true;
}
}
<span style="color: #800080; font-weight: bold">@@ -254,17 +252,22 @@ ApplicationWindow {</span>
}
}
<span style="color: #A00000">- NavigationSetup {</span>
<span style="color: #A00000">- id: navigationSettings</span>
<span style="color: #A00000">- visible: false</span>
<span style="color: #A00000">- property bool departureIsSet: false</span>
<span style="color: #A00000">- property bool destinationIsSet: false</span>
<span style="color: #00A000">+ Item {</span>
<span style="color: #00A000">+ id: waypointActionDialog</span>
<span style="color: #A00000">- height: Screen.pixelDensity * 9</span>
<span style="color: #A00000">- anchors {</span>
<span style="color: #A00000">- bottom: parent.bottom</span>
<span style="color: #A00000">- left: parent.left</span>
<span style="color: #A00000">- right: parent.right</span>
<span style="color: #00A000">+ NavigationSetup {</span>
<span style="color: #00A000">+ id: navigationSettings</span>
<span style="color: #00A000">+ property bool departureIsSet: false</span>
<span style="color: #00A000">+ property bool destinationIsSet: false</span>
<span style="color: #00A000">+</span>
<span style="color: #00A000">+ onAccepted: itemStack.pop()</span>
<span style="color: #00A000">+</span>
<span style="color: #00A000">+ height: Screen.pixelDensity * 9</span>
<span style="color: #00A000">+ anchors {</span>
<span style="color: #00A000">+ bottom: parent.bottom</span>
<span style="color: #00A000">+ left: parent.left</span>
<span style="color: #00A000">+ right: parent.right</span>
<span style="color: #00A000">+ }</span>
}
}
<span style="color: #800080; font-weight: bold">@@ -280,7 +283,6 @@ ApplicationWindow {</span>
WaypointOrderManager {
id: waypointOrderEditor
<span style="color: #A00000">- anchors.fill: parent</span>
visible: false
routingManager:routing
}
<span style="color: #800080; font-weight: bold">@@ -288,13 +290,11 @@ ApplicationWindow {</span>
RoutePlanViewer{
id: instructions
visible: false
<span style="color: #A00000">- anchors.fill: parent</span>
model: routing.routingModel
}
Keys.onBackPressed: {
event.accepted = pop();
}
<span style="color: #A00000">-</span>
}
}
<span style="color: #000080; font-weight: bold">diff --git a/src/apps/marble-maps/NavigationSetup.qml b/src/apps/marble-maps/NavigationSetup.qml</span>
<span style="color: #000080; font-weight: bold">index e552efe..eb68490 100644</span>
<span style="color: #A00000">--- a/src/apps/marble-maps/NavigationSetup.qml</span>
<span style="color: #00A000">+++ b/src/apps/marble-maps/NavigationSetup.qml</span>
<span style="color: #800080; font-weight: bold">@@ -20,7 +20,8 @@ Item {</span>
signal routeToDestinationRequested()
signal routeFromDepartureRequested()
signal routeThroughWaypointRequested()
<span style="color: #A00000">- signal profileSelected(string profile)</span>
<span style="color: #00A000">+</span>
<span style="color: #00A000">+ signal accepted()</span>
SystemPalette{
id: palette
<span style="color: #800080; font-weight: bold">@@ -45,19 +46,19 @@ Item {</span>
NavigationSetupButton {
imageSource: "qrc:///navigation.png"
text: qsTr("As destination")
<span style="color: #A00000">- onClicked: { routeToDestinationRequested(); }</span>
<span style="color: #00A000">+ onClicked: { routeToDestinationRequested(); accepted(); }</span>
}
NavigationSetupButton {
imageSource: "qrc:///waypoint.png"
text: qsTr("As waypoint")
<span style="color: #A00000">- onClicked: { routeThroughWaypointRequested(); }</span>
<span style="color: #00A000">+ onClicked: { routeThroughWaypointRequested(); accepted(); }</span>
}
NavigationSetupButton {
imageSource: "qrc:///map.png"
text: qsTr("As departure")
<span style="color: #A00000">- onClicked: { routeFromDepartureRequested(); }</span>
<span style="color: #00A000">+ onClicked: { routeFromDepartureRequested(); accepted(); }</span>
}
}
}
<span style="color: #000080; font-weight: bold">diff --git a/src/apps/marble-maps/RoutePlanViewer.qml b/src/apps/marble-maps/RoutePlanViewer.qml</span>
<span style="color: #000080; font-weight: bold">index 4ca794e..32a8ad6 100644</span>
<span style="color: #A00000">--- a/src/apps/marble-maps/RoutePlanViewer.qml</span>
<span style="color: #00A000">+++ b/src/apps/marble-maps/RoutePlanViewer.qml</span>
<span style="color: #800080; font-weight: bold">@@ -17,7 +17,6 @@ import org.kde.edu.marble 0.20</span>
Item {
id: root
<span style="color: #A00000">- anchors.fill: parent</span>
property var model: []
<span style="color: #000080; font-weight: bold">diff --git a/src/apps/marble-maps/Search.qml b/src/apps/marble-maps/Search.qml</span>
<span style="color: #000080; font-weight: bold">index 69ecc0a..b777e4d 100644</span>
<span style="color: #A00000">--- a/src/apps/marble-maps/Search.qml</span>
<span style="color: #00A000">+++ b/src/apps/marble-maps/Search.qml</span>
<span style="color: #800080; font-weight: bold">@@ -17,8 +17,6 @@ import org.kde.edu.marble 0.20</span>
Item {
id: root
<span style="color: #A00000">- visible: false</span>
<span style="color: #A00000">-</span>
property var marbleQuickItem: null
signal itemSelected()
readonly property alias searchResultPlacemark: backend.selectedPlacemark
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Long story short: I'd suggest using plain Qt StackView and not making the search field an element in the stack. For further development after finishing the current review requests I'd like to discuss bringing the routing elements closer together in the user interface.</p></pre>
<br />
<p>- Dennis Nienhüser</p>
<br />
<p>On August 8th, 2015, 8:24 p.m. UTC, Gábor Péterffy wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for Marble, Mihail Ivchenko and Dennis Nienhüser.</div>
<div>By Gábor Péterffy.</div>
<p style="color: grey;"><i>Updated Aug. 8, 2015, 8:24 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
marble
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><ul style="padding: 0;text-rendering: inherit;margin: 0 0 0 1em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">A stack for QQuickItems</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Working back button</li>
</ul></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>src/apps/marble-maps/MainScreen.qml <span style="color: grey">(fafd183)</span></li>
<li>src/lib/marble/declarative/CMakeLists.txt <span style="color: grey">(6623503)</span></li>
<li>src/lib/marble/declarative/ItemStack.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/lib/marble/declarative/ItemStack.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/lib/marble/declarative/MarbleDeclarativePlugin.cpp <span style="color: grey">(72bd7f9)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/124666/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>