D28383: Add PageRouter component

[Marco Martin]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20200401/78764b8d/attachment.html>