dfaure added a comment.
The unittest in this commit appears to break in CI.
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
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
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,
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
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
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
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
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
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
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
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
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
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
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,
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
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
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
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
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
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
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.
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
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
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"
>
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
> +{
> +
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
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
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
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
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
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
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
33 matches
Mail list logo
