D28383: Add PageRouter component
dfaure added a comment. 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! REPOSITORY R169 Kirigami REVISION DETAIL https://phabricator.kde.org/D28383 To: cblack, #kirigami, mart, davidedmundson Cc: dfaure, ahiemstra, davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, mart
D28383: Add PageRouter component
This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit R169:1d5398acecaa: Add PageRouter component (authored by cblack). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D28383?vs=80282=80283#toc REPOSITORY R169 Kirigami CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28383?vs=80282=80283 REVISION DETAIL https://phabricator.kde.org/D28383 AFFECTED FILES Mainpage.dox autotests/CMakeLists.txt autotests/tst_pagerouter.qml docs/pics/PageRouterModel.svg docs/pics/PageRouterNavigate.svg docs/pics/PageRouterPop.svg docs/pics/PageRouterPush.svg examples/PageRoute.qml examples/PageRouter.qml examples/PageRouterCachePagesDo.qml examples/PageRouterCachePagesDont.qml examples/PageRouterColumnView.qml examples/PageRouterInitialRoute.qml examples/PageRouterRoutes.qml src/CMakeLists.txt src/controls/PageRow.qml src/kirigamiplugin.cpp src/pagerouter.cpp src/pagerouter.h To: cblack, #kirigami, mart, davidedmundson Cc: ahiemstra, davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, mart
D28383: Add PageRouter component
cblack updated this revision to Diff 80282. cblack marked an inline comment as done. cblack added a comment. Use more descriptive name REPOSITORY R169 Kirigami CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28383?vs=80275=80282 BRANCH arcpatch-D28383 REVISION DETAIL https://phabricator.kde.org/D28383 AFFECTED FILES Mainpage.dox autotests/CMakeLists.txt autotests/tst_pagerouter.qml docs/pics/PageRouterModel.svg docs/pics/PageRouterNavigate.svg docs/pics/PageRouterPop.svg docs/pics/PageRouterPush.svg examples/PageRoute.qml examples/PageRouter.qml examples/PageRouterCachePagesDo.qml examples/PageRouterCachePagesDont.qml examples/PageRouterColumnView.qml examples/PageRouterInitialRoute.qml examples/PageRouterRoutes.qml src/CMakeLists.txt src/controls/PageRow.qml src/kirigamiplugin.cpp src/pagerouter.cpp src/pagerouter.h To: cblack, #kirigami, mart, davidedmundson Cc: ahiemstra, davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, mart
D28383: Add PageRouter component
mart accepted this revision. mart added inline comments. INLINE COMMENTS > pagerouter.cpp:217 > +auto incomingRoutes = parseRoutes(route); > +QList mut; > + more descriptive name REPOSITORY R169 Kirigami REVISION DETAIL https://phabricator.kde.org/D28383 To: cblack, #kirigami, mart, davidedmundson Cc: ahiemstra, davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, mart
D28383: Add PageRouter component
cblack updated this revision to Diff 80275. cblack added a comment. Fix faulty navigateToRoute REPOSITORY R169 Kirigami CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28383?vs=80223=80275 BRANCH arcpatch-D28383 REVISION DETAIL https://phabricator.kde.org/D28383 AFFECTED FILES Mainpage.dox autotests/CMakeLists.txt autotests/tst_pagerouter.qml docs/pics/PageRouterModel.svg docs/pics/PageRouterNavigate.svg docs/pics/PageRouterPop.svg docs/pics/PageRouterPush.svg examples/PageRoute.qml examples/PageRouter.qml examples/PageRouterCachePagesDo.qml examples/PageRouterCachePagesDont.qml examples/PageRouterColumnView.qml examples/PageRouterInitialRoute.qml examples/PageRouterRoutes.qml src/CMakeLists.txt src/controls/PageRow.qml src/kirigamiplugin.cpp src/pagerouter.cpp src/pagerouter.h To: cblack, #kirigami, mart, davidedmundson Cc: ahiemstra, davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, mart
D28383: Add PageRouter component
cblack updated this revision to Diff 80223. cblack marked 3 inline comments as done. cblack added a comment. Address feedback REPOSITORY R169 Kirigami CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28383?vs=79874=80223 BRANCH arcpatch-D28383 REVISION DETAIL https://phabricator.kde.org/D28383 AFFECTED FILES Mainpage.dox autotests/CMakeLists.txt autotests/tst_pagerouter.qml docs/pics/PageRouterModel.svg docs/pics/PageRouterNavigate.svg docs/pics/PageRouterPop.svg docs/pics/PageRouterPush.svg examples/PageRoute.qml examples/PageRouter.qml examples/PageRouterCachePagesDo.qml examples/PageRouterCachePagesDont.qml examples/PageRouterColumnView.qml examples/PageRouterInitialRoute.qml examples/PageRouterRoutes.qml src/CMakeLists.txt src/controls/PageRow.qml src/kirigamiplugin.cpp src/pagerouter.cpp src/pagerouter.h To: cblack, #kirigami, mart, davidedmundson Cc: ahiemstra, davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, mart
D28383: Add PageRouter component
mart added inline comments.
INLINE COMMENTS
> tst_pagerouter.qml:56
> +id: router
> +initialRoute: "/home"
> +columnView: root.columnView
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"]
> pagerouter.cpp:214
> +
> +void PageRouter::navigateToRoute(QJSValue route)
> +{
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
> pagerouter.h:180
> + */
> +Q_PROPERTY(ColumnView* columnView MEMBER m_columnView NOTIFY
> columnViewChanged)
> +
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)
> pagerouter.h:357
> +QML_DECLARE_TYPEINFO(PageRouter, QML_HAS_ATTACHED_PROPERTIES)
> \ No newline at end of file
newlines!
REPOSITORY
R169 Kirigami
REVISION DETAIL
https://phabricator.kde.org/D28383
To: cblack, #kirigami, mart, davidedmundson
Cc: ahiemstra, davidedmundson, plasma-devel, fbampaloukas, GB_2, domson,
dkardarakos, ngraham, apol, mart
D28383: Add PageRouter component
cblack updated this revision to Diff 79874. cblack added a comment. Add tests REPOSITORY R169 Kirigami CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28383?vs=79870=79874 BRANCH cblack/pagerouter REVISION DETAIL https://phabricator.kde.org/D28383 AFFECTED FILES Mainpage.dox autotests/CMakeLists.txt autotests/tst_pagerouter.qml docs/pics/PageRouterModel.svg docs/pics/PageRouterNavigate.svg docs/pics/PageRouterPop.svg docs/pics/PageRouterPush.svg examples/PageRoute.qml examples/PageRouter.qml examples/PageRouterCachePagesDo.qml examples/PageRouterCachePagesDont.qml examples/PageRouterColumnView.qml examples/PageRouterInitialRoute.qml examples/PageRouterRoutes.qml src/CMakeLists.txt src/controls/PageRow.qml src/kirigamiplugin.cpp src/pagerouter.cpp src/pagerouter.h To: cblack, #kirigami, mart, davidedmundson Cc: ahiemstra, davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, mart
D28383: Add PageRouter component
cblack updated this revision to Diff 79870. cblack added a comment. Add currentRoutes function REPOSITORY R169 Kirigami CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28383?vs=79785=79870 BRANCH cblack/pagerouter REVISION DETAIL https://phabricator.kde.org/D28383 AFFECTED FILES Mainpage.dox docs/pics/PageRouterModel.svg docs/pics/PageRouterNavigate.svg docs/pics/PageRouterPop.svg docs/pics/PageRouterPush.svg examples/PageRoute.qml examples/PageRouter.qml examples/PageRouterCachePagesDo.qml examples/PageRouterCachePagesDont.qml examples/PageRouterColumnView.qml examples/PageRouterInitialRoute.qml examples/PageRouterRoutes.qml src/CMakeLists.txt src/controls/PageRow.qml src/kirigamiplugin.cpp src/pagerouter.cpp src/pagerouter.h To: cblack, #kirigami, mart, davidedmundson Cc: ahiemstra, davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, mart
D28383: Add PageRouter component
cblack updated this revision to Diff 79785. cblack added a comment. Add bringToView and isCurrent REPOSITORY R169 Kirigami CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28383?vs=79774=79785 BRANCH cblack/pagerouter REVISION DETAIL https://phabricator.kde.org/D28383 AFFECTED FILES Mainpage.dox docs/pics/PageRouterModel.svg docs/pics/PageRouterNavigate.svg docs/pics/PageRouterPop.svg docs/pics/PageRouterPush.svg examples/PageRoute.qml examples/PageRouter.qml examples/PageRouterCachePagesDo.qml examples/PageRouterCachePagesDont.qml examples/PageRouterColumnView.qml examples/PageRouterInitialRoute.qml examples/PageRouterRoutes.qml src/CMakeLists.txt src/controls/PageRow.qml src/kirigamiplugin.cpp src/pagerouter.cpp src/pagerouter.h To: cblack, #kirigami, mart, davidedmundson Cc: ahiemstra, davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, mart
D28383: Add PageRouter component
cblack updated this revision to Diff 79774. cblack added a comment. Fix bad image text REPOSITORY R169 Kirigami CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28383?vs=79748=79774 BRANCH cblack/pagerouter REVISION DETAIL https://phabricator.kde.org/D28383 AFFECTED FILES Mainpage.dox docs/pics/PageRouterModel.svg docs/pics/PageRouterNavigate.svg docs/pics/PageRouterPop.svg docs/pics/PageRouterPush.svg examples/PageRoute.qml examples/PageRouter.qml examples/PageRouterCachePagesDo.qml examples/PageRouterCachePagesDont.qml examples/PageRouterColumnView.qml examples/PageRouterInitialRoute.qml examples/PageRouterRoutes.qml src/CMakeLists.txt src/controls/PageRow.qml src/kirigamiplugin.cpp src/pagerouter.cpp src/pagerouter.h To: cblack, #kirigami, mart, davidedmundson Cc: ahiemstra, davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, mart
D28383: Add PageRouter component
cblack updated this revision to Diff 79748. cblack added a comment. Improve documentation REPOSITORY R169 Kirigami CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28383?vs=79746=79748 BRANCH cblack/pagerouter REVISION DETAIL https://phabricator.kde.org/D28383 AFFECTED FILES Mainpage.dox docs/pics/PageRouterModel.svg docs/pics/PageRouterNavigate.svg docs/pics/PageRouterPop.svg docs/pics/PageRouterPush.svg examples/PageRoute.qml examples/PageRouter.qml examples/PageRouterCachePagesDo.qml examples/PageRouterCachePagesDont.qml examples/PageRouterColumnView.qml examples/PageRouterInitialRoute.qml examples/PageRouterRoutes.qml src/CMakeLists.txt src/controls/PageRow.qml src/kirigamiplugin.cpp src/pagerouter.cpp src/pagerouter.h To: cblack, #kirigami, mart, davidedmundson Cc: ahiemstra, davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, mart
D28383: Add PageRouter component
cblack updated this revision to Diff 79746. cblack added a comment. Address feedback (both human and compiler), improve examples, port to QQmlParserStatus REPOSITORY R169 Kirigami CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28383?vs=79745=79746 BRANCH cblack/pagerouter REVISION DETAIL https://phabricator.kde.org/D28383 AFFECTED FILES Mainpage.dox examples/PageRoute.qml examples/PageRouter.qml examples/PageRouterCachePagesDo.qml examples/PageRouterCachePagesDont.qml examples/PageRouterColumnView.qml examples/PageRouterInitialRoute.qml examples/PageRouterRoutes.qml src/CMakeLists.txt src/controls/PageRow.qml src/kirigamiplugin.cpp src/pagerouter.cpp src/pagerouter.h To: cblack, #kirigami, mart, davidedmundson Cc: ahiemstra, davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, mart
D28383: Add PageRouter component
cblack updated this revision to Diff 79745. cblack added a comment. Fix Doxygen errors REPOSITORY R169 Kirigami CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28383?vs=79498=79745 BRANCH cblack/pagerouter REVISION DETAIL https://phabricator.kde.org/D28383 AFFECTED FILES Mainpage.dox src/CMakeLists.txt src/controls/PageRow.qml src/kirigamiplugin.cpp src/pagerouter.cpp src/pagerouter.h To: cblack, #kirigami, mart, davidedmundson Cc: ahiemstra, davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, mart
D28383: Add PageRouter component
cblack planned changes to this revision. cblack added a comment. `@include` not resolving, will fix REPOSITORY R169 Kirigami REVISION DETAIL https://phabricator.kde.org/D28383 To: cblack, #kirigami, mart, davidedmundson Cc: ahiemstra, davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, mart
D28383: Add PageRouter component
cblack added inline comments. INLINE COMMENTS > mart wrote in pagerouter.h:12 > is this still needed for routes that are a composition of PageRoute objects > like /path/to/some/thing? 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. > mart wrote in pagerouter.h:66 > since is per page this global property should go The global property is already gone, this is the per-page property > mart wrote in pagerouter.h:91 > 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 componentComplete is used to ensure that everything is parsed only when QML is done, not when the object is created > mart wrote in pagerouter.h:114 > Q_PROPERTY(QString initialRoute (provided the route name is unique, which > should be checked and some type of error thrown if not) The initial route can have data REPOSITORY R169 Kirigami REVISION DETAIL https://phabricator.kde.org/D28383 To: cblack, #kirigami, mart, davidedmundson Cc: ahiemstra, davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, mart
D28383: Add PageRouter component
cblack updated this revision to Diff 79498. cblack marked 4 inline comments as done. cblack added a comment. Better documentation comment REPOSITORY R169 Kirigami CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28383?vs=79320=79498 BRANCH cblack/pagerouter REVISION DETAIL https://phabricator.kde.org/D28383 AFFECTED FILES Mainpage.dox src/CMakeLists.txt src/controls/PageRow.qml src/doc/PageRoute.qml src/doc/PageRouter.qml src/doc/PageRouterCachePagesDo.qml src/doc/PageRouterCachePagesDont.qml src/doc/PageRouterColumnView.qml src/doc/PageRouterInitialRoute.qml src/doc/PageRouterRoutes.qml src/kirigamiplugin.cpp src/pagerouter.cpp src/pagerouter.h To: cblack, #kirigami, mart, davidedmundson Cc: ahiemstra, davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, mart
D28383: Add PageRouter component
mart requested changes to this revision.
mart added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> pagerouter.h:12
> +
> +struct ParsedRoute {
> +QString name;
is this still needed for routes that are a composition of PageRoute objects
like /path/to/some/thing?
> pagerouter.h:66
> + */
> +Q_PROPERTY(bool cache MEMBER m_cache READ cache)
> +
since is per page this global property should go
> pagerouter.h:91
> + */
> +class PageRouter : public QQuickItem
> +{
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
> pagerouter.h:114
> + */
> +Q_PROPERTY(QJSValue initialRoute READ initialRoute WRITE setInitialRoute
> NOTIFY initialRouteChanged)
> +
Q_PROPERTY(QString initialRoute (provided the route name is unique, which
should be checked and some type of error thrown if not)
REPOSITORY
R169 Kirigami
REVISION DETAIL
https://phabricator.kde.org/D28383
To: cblack, #kirigami, mart, davidedmundson
Cc: ahiemstra, davidedmundson, plasma-devel, fbampaloukas, GB_2, domson,
dkardarakos, ngraham, apol, mart
D28383: Add PageRouter component
cblack updated this revision to Diff 79320. cblack marked an inline comment as done. cblack added a comment. Add documentation REPOSITORY R169 Kirigami CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28383?vs=79082=79320 BRANCH cblack/pagerouter REVISION DETAIL https://phabricator.kde.org/D28383 AFFECTED FILES Mainpage.dox src/CMakeLists.txt src/controls/PageRow.qml src/doc/PageRoute.qml src/doc/PageRouter.qml src/doc/PageRouterCachePagesDo.qml src/doc/PageRouterCachePagesDont.qml src/doc/PageRouterColumnView.qml src/doc/PageRouterInitialRoute.qml src/doc/PageRouterRoutes.qml src/kirigamiplugin.cpp src/pagerouter.cpp src/pagerouter.h To: cblack, #kirigami, mart, davidedmundson Cc: ahiemstra, davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, mart
D28383: Add PageRouter component
mart added inline comments. INLINE COMMENTS > PageRow.qml:60 > +// Private handle to columnView. > +property alias _columnView: columnView > + if is a publicly accessible property, it should be apublic and documented REPOSITORY R169 Kirigami REVISION DETAIL https://phabricator.kde.org/D28383 To: cblack, #kirigami, mart, davidedmundson Cc: ahiemstra, davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, mart
D28383: Add PageRouter component
cblack updated this revision to Diff 79082. cblack marked an inline comment as done. cblack added a comment. Address more feedback REPOSITORY R169 Kirigami CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28383?vs=79080=79082 BRANCH cblack/pagerouter REVISION DETAIL https://phabricator.kde.org/D28383 AFFECTED FILES Mainpage.dox src/CMakeLists.txt src/controls/PageRow.qml src/doc/PageRoute.qml src/doc/PageRouter.qml src/doc/PageRouterCachePagesDo.qml src/doc/PageRouterCachePagesDont.qml src/doc/PageRouterColumnView.qml src/doc/PageRouterInitialRoute.qml src/doc/PageRouterRoutes.qml src/kirigamiplugin.cpp src/pagerouter.cpp src/pagerouter.h To: cblack, #kirigami, mart, davidedmundson Cc: ahiemstra, davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, mart
D28383: Add PageRouter component
cblack added inline comments. INLINE COMMENTS > ahiemstra wrote in pagerouter.h:308 > If you have routes as objects you could have an "active" property on the > route, removing the need for this invokable. 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. REPOSITORY R169 Kirigami REVISION DETAIL https://phabricator.kde.org/D28383 To: cblack, #kirigami, mart, davidedmundson Cc: ahiemstra, davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, mart
D28383: Add PageRouter component
cblack updated this revision to Diff 79080. cblack marked 5 inline comments as done. cblack added a comment. Address more feedback REPOSITORY R169 Kirigami CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28383?vs=79073=79080 BRANCH cblack/pagerouter REVISION DETAIL https://phabricator.kde.org/D28383 AFFECTED FILES Mainpage.dox src/CMakeLists.txt src/controls/PageRow.qml src/doc/PageRouter.qml src/doc/PageRouterCachePagesDo.qml src/doc/PageRouterCachePagesDont.qml src/doc/PageRouterColumnView.qml src/doc/PageRouterInitialRoute.qml src/doc/PageRouterRoutes.qml src/kirigamiplugin.cpp src/pagerouter.cpp src/pagerouter.h To: cblack, #kirigami, mart, davidedmundson Cc: ahiemstra, davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, mart
D28383: Add PageRouter component
cblack updated this revision to Diff 79073. cblack marked 2 inline comments as done. cblack added a comment. Consume ColumnView instead of PageRow REPOSITORY R169 Kirigami CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28383?vs=78948=79073 BRANCH cblack/pagerouter REVISION DETAIL https://phabricator.kde.org/D28383 AFFECTED FILES src/CMakeLists.txt src/controls/PageRow.qml src/kirigamiplugin.cpp src/pagerouter.cpp src/pagerouter.h To: cblack, #kirigami, mart, davidedmundson Cc: ahiemstra, davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, mart
D28383: Add PageRouter component
ahiemstra added inline comments.
INLINE COMMENTS
> mart wrote in pagerouter.h:84
> 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)
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.
> mart wrote in pagerouter.h:308
> quite ugly name..
> maybe isRouteActive
If you have routes as objects you could have an "active" property on the route,
removing the need for this invokable.
REPOSITORY
R169 Kirigami
REVISION DETAIL
https://phabricator.kde.org/D28383
To: cblack, #kirigami, mart, davidedmundson
Cc: ahiemstra, davidedmundson, plasma-devel, fbampaloukas, GB_2, domson,
dkardarakos, ngraham, apol, mart
D28383: Add PageRouter component
mart requested changes to this revision.
mart added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> pagerouter.cpp:154
> +
> +void PageRouter::setRoutes(QJSValue routes)
> +{
should add parsing and validation here
> pagerouter.cpp:199
> +{
> +QMetaObject::invokeMethod(m_pageRow, "pop");
> +}
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)
> pagerouter.h:55
> + * }
> + * routes: {
> + * "/home": homeComponent,
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
> pagerouter.h:84
> + * routes: {
> + * "/home": homeComponent
> + * }
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)
> pagerouter.h:121
> + */
> +Q_PROPERTY(QQuickItem* pageRow MEMBER m_pageRow)
> +
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
> pagerouter.h:168
> + */
> +Q_PROPERTY(bool cachePages MEMBER m_cachePages NOTIFY cachePagesChanged)
> +
this property would be better per page
> pagerouter.h:308
> + */
> +Q_INVOKABLE bool isNavigatedToRoute(QJSValue route);
> +
quite ugly name..
maybe isRouteActive
REPOSITORY
R169 Kirigami
REVISION DETAIL
https://phabricator.kde.org/D28383
To: cblack, #kirigami, mart, davidedmundson
Cc: davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, dkardarakos,
ngraham, apol, ahiemstra, mart
D28383: Add PageRouter component
cblack updated this revision to Diff 78948. cblack added a comment. Add more documentation comments REPOSITORY R169 Kirigami CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28383?vs=78946=78948 BRANCH cblack/pagerouter REVISION DETAIL https://phabricator.kde.org/D28383 AFFECTED FILES src/CMakeLists.txt src/kirigamiplugin.cpp src/pagerouter.cpp src/pagerouter.h To: cblack, #kirigami, mart, davidedmundson Cc: davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, ahiemstra, mart
D28383: Add PageRouter component
cblack updated this revision to Diff 78946. cblack added a comment. Get arc to properly diff REPOSITORY R169 Kirigami CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28383?vs=78927=78946 BRANCH cblack/pagerouter REVISION DETAIL https://phabricator.kde.org/D28383 AFFECTED FILES src/CMakeLists.txt src/kirigamiplugin.cpp src/pagerouter.cpp src/pagerouter.h To: cblack, #kirigami, mart, davidedmundson Cc: davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, ahiemstra, mart
D28383: Add PageRouter component
cblack updated this revision to Diff 78927. cblack added a comment. Add caching REPOSITORY R169 Kirigami CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28383?vs=78747=78927 BRANCH cblack/pagerouter REVISION DETAIL https://phabricator.kde.org/D28383 AFFECTED FILES src/kirigamiplugin.cpp src/pagerouter.cpp src/pagerouter.h To: cblack, #kirigami, mart, davidedmundson Cc: davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, ahiemstra, mart
D28383: Add PageRouter component
cblack updated this revision to Diff 78747. cblack added a comment. Get arc to deal with multiple commits properly REPOSITORY R169 Kirigami CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28383?vs=78746=78747 BRANCH cblack/pagerouter REVISION DETAIL https://phabricator.kde.org/D28383 AFFECTED FILES src/CMakeLists.txt src/kirigamiplugin.cpp src/pagerouter.cpp src/pagerouter.h To: cblack, #kirigami, mart, davidedmundson Cc: davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, ahiemstra, mart
D28383: Add PageRouter component
cblack updated this revision to Diff 78746. cblack added a comment. Add documentation comments REPOSITORY R169 Kirigami CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28383?vs=78744=78746 BRANCH cblack/pagerouter REVISION DETAIL https://phabricator.kde.org/D28383 AFFECTED FILES src/kirigamiplugin.cpp src/pagerouter.cpp src/pagerouter.h To: cblack, #kirigami, mart, davidedmundson Cc: davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, ahiemstra, mart
D28383: Add PageRouter component
davidedmundson requested changes to this revision. davidedmundson added a comment. This revision now requires changes to proceed. This needs way more description and explanation of the problem it's trying to solve. REPOSITORY R169 Kirigami REVISION DETAIL https://phabricator.kde.org/D28383 To: cblack, #kirigami, mart, davidedmundson Cc: davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, ahiemstra, mart
D28383: Add PageRouter component
cblack created this revision. cblack added reviewers: Kirigami, mart. Herald added a project: Kirigami. Herald added a subscriber: plasma-devel. cblack requested review of this revision. REVISION SUMMARY PageRouter is a component that manages named routes of pages. REPOSITORY R169 Kirigami BRANCH cblack/pagerouter REVISION DETAIL https://phabricator.kde.org/D28383 AFFECTED FILES src/CMakeLists.txt src/kirigamiplugin.cpp src/pagerouter.cpp src/pagerouter.h To: cblack, #kirigami, mart Cc: plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, ahiemstra, davidedmundson, mart
