Add PageRouter component
ClosedPublic

Authored by cblack on Mar 28 2020, 5:20 PM.

Details

Summary

PageRouter is a component that manages named routes of pages in a declarative rather than imperative manner. A PageRow is managed imperatively (i.e. rather than declaring that a button takes you to the login page, it imperatively pushes a new instance of a login page component on click) and the PageRouter abstracts the imperative behavior of a PageRow into a more declarative API. A PageRouter is designed for the usecase of complex and randomly accessible page hierarchies.

For example, Discover, an appstore application navigates linearly for the most part, using a sidebar for non-linear navigation purposes: A listing of categories (/home) brings you to a listing of items (/apps(science)) brings you to a details page (/app(org.kde.ScienceApp)). Directly accessing the PageRow here works fine, as navigation needs aren't very complex.

In comparison, a theoretical appstore application could have much more complex navigation. On startup, it would check if you're logged in or not, and set the initial route to /login or /home accordingly. A user could either click through the interface, browsing available applications in a relatively linear manner /apps(games), then /app(org.kde.TetrisClone), or they could use a search field located on the home page, bringing them directly to /home/apps(games)/app(org.kde.TetrisClone). Or, an app store could be running a promotion, and the route would look like /home/promotions(BestGames2020)/app(org.kde.TetrisClone), bringing the user to the same page in a different hierarchy. A PageRow doesn't scale as nicely in this usecase, as there are a wide variety of pages in a hierarchy that can be navigated in a random order.

Handling of communicating between the parent PageRouter and the child Page is also abstracted in a nicer API—the Kirigami.PageRouter attached object is used for navigation instead of passing an id around, and its data property is used for data passing instead of pushing a page and setting a property on it.

Diff Detail

Repository
R169 Kirigami
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
cblack updated this revision to Diff 78948.Mar 31 2020, 2:26 AM

Add more documentation comments

mart requested changes to this revision.Apr 1 2020, 9:33 AM
mart added inline comments.
src/pagerouter.cpp
155

should add parsing and validation here

200

pagerow.pop will pop the *current* page and everything on top of it, so you can't rely on its behavior, it should really use its internal columnview directly (of which you have c++ access there)

src/pagerouter.h
56

add an example route of a/more/complex/path

if i have /home/login, will it always be the same component as just /login? is not clear here

85

how many routes an app would normally be going to have?

this seems really a thing for QQmlListProperty (which then each route must become a qobject tough)
so routes: [
Route {

name: "home"
component: homeComponent
cache: true
initialProperties: {"name": "value,...}

},
Route {}
]

which kindof becomes similar to pagepoolaction (which i still think they can be merged)

122

i'm not sure if i
it hadles directly the creation of the components.. so it really doesn't have a reason of being a pagerow over columnView which is the internal implementation of pagerow.
in this paramenter, it should search for a columnview property or something like that which it can statically check if it's actually a columnview which will be able to access directly hopefully without needing any invokemethod

169

this property would be better per page

309

quite ugly name..
maybe isRouteActive

This revision now requires changes to proceed.Apr 1 2020, 9:33 AM
ahiemstra added inline comments.
src/pagerouter.h
85

This is exactly what I was thinking. And if you mark it as default property you wouldn't even need to do the "routes: []" awkwardness.

309

If you have routes as objects you could have an "active" property on the route, removing the need for this invokable.

cblack updated this revision to Diff 79073.Apr 1 2020, 5:48 PM
cblack marked 2 inline comments as done.

Consume ColumnView instead of PageRow

cblack updated this revision to Diff 79080.Apr 1 2020, 7:27 PM
cblack marked 5 inline comments as done.

Address more feedback

cblack added inline comments.Apr 1 2020, 7:29 PM
src/pagerouter.h
309

Not really, the point of this property is to query whether a full URI is on the stack. The login route being on the stack somewhere is not the same information as whether /home/login or /apps/login are on the route.

cblack marked an inline comment as done.Apr 1 2020, 7:29 PM
cblack updated this revision to Diff 79082.Apr 1 2020, 7:50 PM
cblack marked an inline comment as done.

Address more feedback

mart added inline comments.Apr 3 2020, 8:17 AM
src/controls/PageRow.qml
60

if is a publicly accessible property, it should be apublic and documented

cblack updated this revision to Diff 79320.Apr 4 2020, 4:21 PM
cblack marked an inline comment as done.

Add documentation

mart requested changes to this revision.Apr 6 2020, 8:51 AM
mart added inline comments.
src/pagerouter.h
13

is this still needed for routes that are a composition of PageRoute objects like /path/to/some/thing?

67

since is per page this global property should go

92

any reason this should be a qquickitem?

this doesn't display things per se, neither should be a parent of items, but just to remote control a given pagerow, it should be a QObject

115

Q_PROPERTY(QString initialRoute (provided the route name is unique, which should be checked and some type of error thrown if not)

This revision now requires changes to proceed.Apr 6 2020, 8:51 AM
cblack updated this revision to Diff 79498.Apr 6 2020, 3:42 PM
cblack marked 4 inline comments as done.

Better documentation comment

cblack added inline comments.Apr 6 2020, 3:43 PM
src/pagerouter.h
13

This is an internal struct for keeping track of internal state and to have a C++ representation of a a QJSValue that may not correspond to a PageRoute object declared by the user. Compare by struct values instead of compare by pointer reference is used here as well.

67

The global property is already gone, this is the per-page property

92

componentComplete is used to ensure that everything is parsed only when QML is done, not when the object is created

115

The initial route can have data

cblack marked 4 inline comments as done.Apr 6 2020, 3:43 PM
cblack planned changes to this revision.Apr 9 2020, 2:38 PM

@include not resolving, will fix

cblack updated this revision to Diff 79745.Apr 10 2020, 1:37 AM

Fix Doxygen errors

cblack updated this revision to Diff 79746.Apr 10 2020, 2:34 AM

Address feedback (both human and compiler), improve examples, port to QQmlParserStatus

cblack updated this revision to Diff 79748.Apr 10 2020, 4:20 AM

Improve documentation

cblack marked an inline comment as done.Apr 10 2020, 3:56 PM
cblack updated this revision to Diff 79774.Apr 10 2020, 4:05 PM

Fix bad image text

cblack updated this revision to Diff 79785.Apr 10 2020, 6:39 PM

Add bringToView and isCurrent

cblack updated this revision to Diff 79870.Apr 11 2020, 3:32 PM

Add currentRoutes function

cblack updated this revision to Diff 79874.Apr 11 2020, 4:20 PM

Add tests

mart added inline comments.Apr 15 2020, 9:38 AM
autotests/tst_pagerouter.qml
57

I find all the "/" in the examples confusing, not sure is a good naming convention (and works prefectly without having that /, and i don't necessarly care what the convention in other frameworks is).
in the hend you can't push directly something like "/home/login" as a single string so doesn't really make sense as paths, i would prefer examples had simple strings as route names, so your route would be indicated as ["home", "login"]

src/pagerouter.cpp
215

I wouldn't expect a verb like navigate to be always destructive.
if the route happens to be the same, then the function should be a no-op
if the current route is
home/foo/bar and i want to navigate to home/foo/baz i would expect to remove and destroy only bar, keeping the first two pages

if the new route is foo/bar/baz, should just push the new one keeping all is there

src/pagerouter.h
181

I know we are fixed to columnview now and its fine, but i would prefer the property being called pageStack like in applicationwindow (in reality i would like both being called pageview but it's too late) which doesn't assume which it is, if sme day we would like to support stackview or whatever else we wouldn't have a sore point in the api

also, to the property assigning the pagerow (which will look for the columnview internally, but not exposed trough the api)

466

newlines!

cblack updated this revision to Diff 80223.Apr 15 2020, 6:33 PM
cblack marked 3 inline comments as done.

Address feedback

cblack marked an inline comment as done.Apr 15 2020, 6:33 PM
mart accepted this revision.Apr 16 2020, 8:27 AM
cblack updated this revision to Diff 80275.Apr 16 2020, 2:04 PM

Fix faulty navigateToRoute

mart accepted this revision.Apr 16 2020, 2:16 PM
mart added inline comments.
src/pagerouter.cpp
218

more descriptive name

cblack updated this revision to Diff 80282.Apr 16 2020, 2:45 PM
cblack marked an inline comment as done.

Use more descriptive name

This revision was not accepted when it landed; it landed in state Needs Review.Apr 16 2020, 2:48 PM
This revision was automatically updated to reflect the committed changes.
dfaure added a subscriber: dfaure.Apr 18 2020, 2:22 PM

The unittest in this commit appears to break in CI.

https://build.kde.org/job/Frameworks/view/Platform%20-%20SUSEQt5.12/job/kirigami/job/kf5-qt5%20SUSEQt5.12/417/testReport/junit/projectroot.home.jenkins.workspace.Frameworks.kirigami.kf5-qt5_SUSEQt512/autotests/tst_pagerouter_qml/

PASS   : Kirigami::PageRouterGeneralTests::test_50_push()
PASS   : Kirigami::PageRouterGeneralTests::test_60_pop()
QWARN  : Kirigami::PageRouterGeneralTests::test_70_bring_to_view() Route "login" with data QVariant(QString, "red") is not on the current stack of routes.
FAIL!  : Kirigami::PageRouterGeneralTests::test_70_bring_to_view() Compared values are not the same
   Actual   (): 0
   Expected (): 1
   Loc: [/home/jenkins/workspace/Frameworks/kirigami/kf5-qt5 SUSEQt5.12/autotests/tst_pagerouter.qml(38)]
FAIL!  : Kirigami::PageRouterGeneralTests::test_80_routeactive() Compared values are not the same
   Actual   (): false
   Expected (): true
   Loc: [/home/jenkins/workspace/Frameworks/kirigami/kf5-qt5 SUSEQt5.12/autotests/tst_pagerouter.qml(45)]
PASS   : Kirigami::PageRouterGeneralTests::test_90_initial_route()

Please investigate and fix.
Thanks!