D10071: Change an assert to do as the function documentation tells
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 REVISION DETAIL https://phabricator.kde.org/D10071 AFFECTED FILES autotests/urlutiltest.cpp src/filewidgets/urlutil_p.h To: jtamate, #frameworks, dfaure Cc: ngraham
D10071: Change an assert to do as the function documentation tells
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
D10071: Change an assert to do as the function documentation tells
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 master) REVISION DETAIL https://phabricator.kde.org/D10071 AFFECTED FILES autotests/urlutiltest.cpp src/filewidgets/urlutil_p.h To: jtamate, #frameworks, dfaure Cc: ngraham
D10071: Change an assert to do as the function documentation tells
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, #frameworks, dfaure Cc: ngraham
D10071: Change an assert to do as the function documentation tells
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 Cc: ngraham
D10071: Change an assert to do as the function documentation tells
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 case where it ends with two or more consecutive > slashes) It is already done (it's a pity that this can't be seen creating the patch using the web): const QUrl adjustedLastUrl = lastUrl.adjusted(QUrl::StripTrailingSlash); const QUrl adjustedCurrentUrl = currentUrl.adjusted(QUrl::StripTrailingSlash); but in the case of the new test, the values are: "/d" and "/" from the originals: "file:///d" and "file:///" REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10071 To: jtamate, #frameworks, dfaure Cc: ngraham
D10071: Change an assert to do as the function documentation tells
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 AFFECTED FILES autotests/urlutiltest.cpp src/filewidgets/urlutil_p.h To: jtamate, #frameworks, dfaure Cc: ngraham
D10071: Change an assert to do as the function documentation tells
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 / (after Stripping the trailing Slash) > +// one letter more is a valid path, otherwise two letters are needed. 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 case where it ends with two or more consecutive slashes) REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10071 To: jtamate, #frameworks, dfaure Cc: ngraham
D10071: Change an assert to do as the function documentation tells
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
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D10071?vs=25872=25883
REVISION DETAIL
https://phabricator.kde.org/D10071
AFFECTED FILES
autotests/urlutiltest.cpp
src/filewidgets/urlutil_p.h
To: jtamate, #frameworks, dfaure
D10071: Change an assert to do as the function documentation tells
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 testing seems to indicate that. A good unittest for a bugfix has to *fail* without the fix applied ;) REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10071 To: jtamate, #frameworks, dfaure
D10071: Change an assert to do as the function documentation tells
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 https://phabricator.kde.org/D10071 AFFECTED FILES autotests/urlutiltest.cpp src/filewidgets/urlutil_p.h To: jtamate, #frameworks, dfaure
D10071: Change an assert to do as the function documentation tells
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
D10071: Change an assert to do as the function documentation tells
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 equal, an invalid URL is returned TEST PLAN How to assert: In dolphin, in the location bar, change it into text and enter into one level directory from root, for example /d When it changes into the breadcrum, press into the root one and crash (in Debug build).รง I've been using this patch one month. And it doesn't affect the unit tests. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10071 AFFECTED FILES src/filewidgets/urlutil_p.h To: jtamate, #frameworks, dfaure
