It turns out that setSelection() cannot take both relative paths (filenames)
and absolute URLs as input. "a:b" can be both, and URLs for files called "a#b"
were handled wrongly.
CCBUG: 376365
Details
Details
- Reviewers
fvogt - Commits
- R241:f49f34727ee6: Add KFileWidget::setSelectedUrl()
Diff Detail
Diff Detail
- Repository
- R241 KIO
- Branch
- master
- Lint
No Linters Available - Unit
No Unit Test Coverage
| src/filewidgets/kfilewidget.h | ||
|---|---|---|
| 169 | This function only works correctly for URLs, as filenames can contain ':'s. | |
| src/filewidgets/kfilewidget.h | ||
|---|---|---|
| 169 | Very good point. But there might be other users of this class who need the ability to select by filename.
| |
| src/filewidgets/kfilewidget.h | ||
|---|---|---|
| 169 | Currently that works with setSelectedUrl as well as it handles relative urls (basically escaped filenames) as well, but it would definitely be a good shortcut, so I'm for option #2 | |
| src/filewidgets/kfilewidget.h | ||
|---|---|---|
| 169 | I guess you mean "with setSelection". | |
| src/filewidgets/kfilewidget.h | ||
|---|---|---|
| 169 | No, I meant setSelectedUrl indeed. It works for relative URLs as well. | |
| src/filewidgets/kfilewidget.h | ||
|---|---|---|
| 169 | Ah right, OK. But people are not going to URL-escape filenames and then we'll have the same bug again ;-) They'll write QUrl(fileName) which won't work for this purpose, for instance for files called "a:b" and "a#b" it will lead to QUrl("a:b") or QUrl("a#b") which will be parsed as "scheme==a" and "fragment==b" respectively, | |
Comment Actions
I just found that this bug was very likely introduced by the fix for https://bugs.kde.org/show_bug.cgi?id=369216, proving that a method that takes a badly defined QString leads to some code (kdialog) calling it with a path and other code (filedialog integration plugin) calling it with URLs, which only leads to ping-pong fixes ;)
As much as I like ping-pong, I'll write unittests so that it stops there ;)
Comment Actions
I thought about it, but the call sites I found didn't actually need that. So unless you know of one, I changed my mind to "let's wait for someone to request it". Although I guess it won't happen because people will just pass a QUrl instead ;-)
Comment Actions
Yes, but constructing a relative QUrl isn't really that easy to do (definitely not obvious at that) and so future users will either pass a filename with QUrl(filename) (broken) or store the absolute path themselves, which is unnecessary memory use and more complicated.
I just noticed that with KIOFILEWIDGETS_NO_DEPRECATED defined the header file will not include the declaration but the cpp file still has the definition of the method, so it won't compile...
Comment Actions
BTW don't port the callers to this new method yet, we'll have to wait until the KF5 minimum requirement is raised in kdialog and plasma-integration. I made a TODO for June ;)