Instead of an assert do as the documentation tells: return an invalid Url.
- In case both URLs are equal, an invalid URL is returned
Details
Details
- Reviewers
dfaure - Group Reviewers
Frameworks - Commits
- R241:805b295541db: Change an assert to do as the function documentation tells
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.
Diff Detail
Diff Detail
- Repository
- R241 KIO
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
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 ;)
Comment Actions
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"));
Comment Actions
Heh, see the point of writing unittests that actually test the bug ;) It lead to a different fix so it was clearly useful ;)
| src/filewidgets/urlutil_p.h | ||
|---|---|---|
| 69 | s/S/s/ twice You could also just use adjustedCurrentUrl.path(QUrl::StripTrailingSlash), no? | |
Comment Actions
replaced two uppercase S
| src/filewidgets/urlutil_p.h | ||
|---|---|---|
| 69 | 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: | |
Comment Actions
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...