Instead of an assert do as the documentation tells: return an invalid Url.
- In case both URLs are equal, an invalid URL is returned
dfaure |
Frameworks |
Instead of an assert do as the documentation tells: return an invalid Url.
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.
No Linters Available |
No Unit Test Coverage |
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 ;)
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"));
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? |
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: |
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...