----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124666/#review83596 -----------------------------------------------------------
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. 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 nav igation 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). 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 ``` diff --git a/src/apps/marble-maps/MainScreen.qml b/src/apps/marble-maps/MainScreen.qml index d54b94e..c06ba33 100644 --- a/src/apps/marble-maps/MainScreen.qml +++ b/src/apps/marble-maps/MainScreen.qml @@ -26,26 +26,12 @@ ApplicationWindow { id: menuBar Menu { title: qsTr("Marble Maps") - MenuItem{ - text: qsTr("Search") - onTriggered: { - if (itemStack.isOnTop(search)) { - itemStack.removeObject(search); - } - else { - itemStack.addObject(search); - itemStack.removeObject(instructions); - itemStack.removeObject(waypointOrderEditor) - } - } - } MenuItem { text: qsTr("Delete Route") onTriggered: { routing.clearRoute(); - itemStack.removeObject(instructions); - itemStack.removeObject(waypointOrderEditor); + itemStack.pop(mapItem) startRoutingButton.show = false; } visible: routing.hasRoute @@ -54,8 +40,8 @@ ApplicationWindow { MenuItem { text: qsTr("Modify Route") onTriggered: { - if (!itemStack.isOnTop(waypointOrderEditor)) { - itemStack.addObject(waypointOrderEditor); + if (itemStack.currentItem !== waypointOrderEditor) { + itemStack.push(waypointOrderEditor); } } visible: routing.hasRoute @@ -64,8 +50,8 @@ ApplicationWindow { MenuItem { text: qsTr("Navigation Instructions") onTriggered: { - if (!itemStack.isOnTop(instructions)) { - itemStack.addObject(instructions) + if (itemStack.currentItem !== instructions) { + itemStack.push(instructions) } } visible: routing.hasRoute && !instructions.visible @@ -74,8 +60,7 @@ ApplicationWindow { MenuItem { text: qsTr("Show the Map") onTriggered: { - itemStack.removeObject(instructions); - itemStack.removeObject(waypointOrderEditor); + itemStack.pop(mapItem) } visible: instructions.visible || waypointOrderEditor.visible } @@ -89,22 +74,35 @@ ApplicationWindow { width: 600 height: 400 - ItemStack { + SystemPalette{ + id: palette + colorGroup: SystemPalette.Active + } + + Rectangle { + id: background + anchors.fill: parent + color: palette.window + } + + StackView { id: itemStack anchors.fill: parent focus: true - SystemPalette{ - id: palette - colorGroup: SystemPalette.Active - } + initialItem: mapItem - Rectangle { - id: background - anchors.fill: parent - color: palette.window + delegate: StackViewDelegate { + function transitionFinished(properties) + { + properties.exitItem.visible = true + } } + } + + Item { + id: mapItem PinchArea { anchors.fill: parent @@ -224,7 +222,7 @@ ApplicationWindow { routing.addPositionAsDeparture(); } else { - itemStack.addObject(navigationSettings); + itemStack.push(waypointActionDialog); navigation = true; } } @@ -254,17 +252,22 @@ ApplicationWindow { } } - NavigationSetup { - id: navigationSettings - visible: false - property bool departureIsSet: false - property bool destinationIsSet: false + Item { + id: waypointActionDialog - height: Screen.pixelDensity * 9 - anchors { - bottom: parent.bottom - left: parent.left - right: parent.right + NavigationSetup { + id: navigationSettings + property bool departureIsSet: false + property bool destinationIsSet: false + + onAccepted: itemStack.pop() + + height: Screen.pixelDensity * 9 + anchors { + bottom: parent.bottom + left: parent.left + right: parent.right + } } } @@ -280,7 +283,6 @@ ApplicationWindow { WaypointOrderManager { id: waypointOrderEditor - anchors.fill: parent visible: false routingManager:routing } @@ -288,13 +290,11 @@ ApplicationWindow { RoutePlanViewer{ id: instructions visible: false - anchors.fill: parent model: routing.routingModel } Keys.onBackPressed: { event.accepted = pop(); } - } } diff --git a/src/apps/marble-maps/NavigationSetup.qml b/src/apps/marble-maps/NavigationSetup.qml index e552efe..eb68490 100644 --- a/src/apps/marble-maps/NavigationSetup.qml +++ b/src/apps/marble-maps/NavigationSetup.qml @@ -20,7 +20,8 @@ Item { signal routeToDestinationRequested() signal routeFromDepartureRequested() signal routeThroughWaypointRequested() - signal profileSelected(string profile) + + signal accepted() SystemPalette{ id: palette @@ -45,19 +46,19 @@ Item { NavigationSetupButton { imageSource: "qrc:///navigation.png" text: qsTr("As destination") - onClicked: { routeToDestinationRequested(); } + onClicked: { routeToDestinationRequested(); accepted(); } } NavigationSetupButton { imageSource: "qrc:///waypoint.png" text: qsTr("As waypoint") - onClicked: { routeThroughWaypointRequested(); } + onClicked: { routeThroughWaypointRequested(); accepted(); } } NavigationSetupButton { imageSource: "qrc:///map.png" text: qsTr("As departure") - onClicked: { routeFromDepartureRequested(); } + onClicked: { routeFromDepartureRequested(); accepted(); } } } } diff --git a/src/apps/marble-maps/RoutePlanViewer.qml b/src/apps/marble-maps/RoutePlanViewer.qml index 4ca794e..32a8ad6 100644 --- a/src/apps/marble-maps/RoutePlanViewer.qml +++ b/src/apps/marble-maps/RoutePlanViewer.qml @@ -17,7 +17,6 @@ import org.kde.edu.marble 0.20 Item { id: root - anchors.fill: parent property var model: [] diff --git a/src/apps/marble-maps/Search.qml b/src/apps/marble-maps/Search.qml index 69ecc0a..b777e4d 100644 --- a/src/apps/marble-maps/Search.qml +++ b/src/apps/marble-maps/Search.qml @@ -17,8 +17,6 @@ import org.kde.edu.marble 0.20 Item { id: root - visible: false - property var marbleQuickItem: null signal itemSelected() readonly property alias searchResultPlacemark: backend.selectedPlacemark ``` 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. - Dennis Nienhüser On Aug. 8, 2015, 8:24 p.m., Gábor Péterffy wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/124666/ > ----------------------------------------------------------- > > (Updated Aug. 8, 2015, 8:24 p.m.) > > > Review request for Marble, Mihail Ivchenko and Dennis Nienhüser. > > > Repository: marble > > > Description > ------- > > - A stack for QQuickItems > - Working back button > > > Diffs > ----- > > src/apps/marble-maps/MainScreen.qml fafd183 > src/lib/marble/declarative/CMakeLists.txt 6623503 > src/lib/marble/declarative/ItemStack.h PRE-CREATION > src/lib/marble/declarative/ItemStack.cpp PRE-CREATION > src/lib/marble/declarative/MarbleDeclarativePlugin.cpp 72bd7f9 > > Diff: https://git.reviewboard.kde.org/r/124666/diff/ > > > Testing > ------- > > > Thanks, > > Gábor Péterffy > >
_______________________________________________ Marble-devel mailing list Marble-devel@kde.org https://mail.kde.org/mailman/listinfo/marble-devel
