This revision was automatically updated to reflect the committed changes.
Closed by commit R241:805b295541db: Change an assert to do as the function
documentation tells (authored by jtamate).
REPOSITORY
R241 KIO
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D10071?vs=25907=25955
dfaure accepted this revision.
REPOSITORY
R241 KIO
BRANCH
firstChildUrl (branched from master)
REVISION DETAIL
https://phabricator.kde.org/D10071
To: jtamate, #frameworks, dfaure
Cc: ngraham
jtamate updated this revision to Diff 25907.
jtamate edited the summary of this revision.
jtamate added a comment.
Improved, in my opinion, one comment.
REPOSITORY
R241 KIO
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D10071?vs=25890=25907
BRANCH
firstChildUrl (branched from
dfaure added a comment.
Actually, maybe parentPath == "/" would be more readable then, otherwise the
next reader will wonder, how it can end with a slash after StripTrailingSlash
was used...
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D10071
To: jtamate,
dfaure accepted this revision.
dfaure added a comment.
This revision is now accepted and ready to land.
Ah, OK (yes, this is why you should use `arc diff` to upload patches...) :-)
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D10071
To: jtamate, #frameworks, dfaure
jtamate added inline comments.
INLINE COMMENTS
> dfaure wrote in urlutil_p.h:69
> s/S/s/ twice
>
> You could also just use adjustedCurrentUrl.path(QUrl::StripTrailingSlash), no?
> Then you wouldn't have to deal with the fact that the path might end with a
> slash
> (and it would also cover the
jtamate updated this revision to Diff 25890.
jtamate marked an inline comment as done.
jtamate added a comment.
replaced two uppercase S
REPOSITORY
R241 KIO
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D10071?vs=25883=25890
REVISION DETAIL
https://phabricator.kde.org/D10071
dfaure added a comment.
Heh, see the point of writing unittests that actually test the bug ;) It lead
to a different fix so it was clearly useful ;)
INLINE COMMENTS
> urlutil_p.h:69
> const QString parentPath = adjustedCurrentUrl.path();
> +// if the parent path ends in /
jtamate updated this revision to Diff 25883.
jtamate added a comment.
Do the right thing in the case of urls that end in / after the slash
stripping.
This new test fails without the patch.
QCOMPARE(KIO::UrlUtil::firstChildUrl(lUrl("/d"), lUrl("/")), lUrl("/d"));
REPOSITORY
R241 KIO
dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.
Strange that the "bug report" mentions /d but the unittests doesn't have
anything like that.
In fact, is it just me, or does the "extended" unittest pass without the fix?
My
jtamate updated this revision to Diff 25872.
jtamate edited the test plan for this revision.
jtamate added a comment.
Added more unit tests with some comments.
REPOSITORY
R241 KIO
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D10071?vs=25869=25872
REVISION DETAIL
dfaure added a comment.
IIRC this method is unittested, right? Please extend the unittest.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D10071
To: jtamate, #frameworks, dfaure
jtamate created this revision.
jtamate added reviewers: Frameworks, dfaure.
Restricted Application added a project: Frameworks.
jtamate requested review of this revision.
REVISION SUMMARY
Instead of an assert do as the documentation tells: return an invalid Url.
- In case both URLs are
13 matches
Mail list logo
