D4937: Add KFileWidget::setSelectedUrl()
dfaure added a comment. 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 ;) REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D4937 To: dfaure, fvogt Cc: fvogt, #frameworks
D4937: Add KFileWidget::setSelectedUrl()
dfaure closed this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D4937 To: dfaure, fvogt Cc: fvogt, #frameworks
D4937: Add KFileWidget::setSelectedUrl()
fvogt accepted this revision. This revision is now accepted and ready to land. REPOSITORY R241 KIO BRANCH master REVISION DETAIL https://phabricator.kde.org/D4937 To: dfaure, fvogt Cc: fvogt, #frameworks
D4937: Add KFileWidget::setSelectedUrl()
dfaure updated this revision to Diff 12280. dfaure added a comment. Add test for relative URL and document the proper way to do it REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D4937?vs=12255=12280 BRANCH master REVISION DETAIL https://phabricator.kde.org/D4937 AFFECTED FILES autotests/kfilewidgettest.cpp src/filewidgets/kfilewidget.cpp src/filewidgets/kfilewidget.h To: dfaure, fvogt Cc: fvogt, #frameworks
D4937: Add KFileWidget::setSelectedUrl()
fvogt requested changes to this revision. fvogt added a comment. This revision now requires changes to proceed. 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... REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D4937 To: dfaure, fvogt Cc: fvogt, #frameworks
D4937: Add KFileWidget::setSelectedUrl()
dfaure added a comment. 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 ;-) REPOSITORY R241 KIO BRANCH master REVISION DETAIL https://phabricator.kde.org/D4937 To: dfaure, fvogt Cc: fvogt, #frameworks
D4937: Add KFileWidget::setSelectedUrl()
fvogt accepted this revision. fvogt added a comment. This revision is now accepted and ready to land. Looks good to me, although a new setSelectedFilename method would be nice to have REPOSITORY R241 KIO BRANCH master REVISION DETAIL https://phabricator.kde.org/D4937 To: dfaure, fvogt Cc: fvogt, #frameworks
D4937: Add KFileWidget::setSelectedUrl()
dfaure updated this revision to Diff 12255. dfaure added a comment. deprecate setSelection REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D4937?vs=12170=12255 BRANCH master REVISION DETAIL https://phabricator.kde.org/D4937 AFFECTED FILES autotests/kfilewidgettest.cpp src/filewidgets/kfilewidget.cpp src/filewidgets/kfilewidget.h To: dfaure, fvogt Cc: fvogt, #frameworks
D4937: Add KFileWidget::setSelectedUrl()
dfaure added a comment. 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 ;) REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D4937 To: dfaure, fvogt Cc: fvogt, #frameworks
D4937: Add KFileWidget::setSelectedUrl()
dfaure added inline comments.
INLINE COMMENTS
> fvogt wrote in kfilewidget.h:150
> No, I meant setSelectedUrl indeed. It works for relative URLs as well.
> Both the function itself and setLocationText check url.isRelative()
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,
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D4937
To: dfaure, fvogt
Cc: fvogt, #frameworks
D4937: Add KFileWidget::setSelectedUrl()
fvogt added inline comments. INLINE COMMENTS > dfaure wrote in kfilewidget.h:150 > I guess you mean "with setSelection". > I made setSelectedUrl handle only full URLs. No, I meant setSelectedUrl indeed. It works for relative URLs as well. Both the function itself and setLocationText check url.isRelative() REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D4937 To: dfaure, fvogt Cc: fvogt, #frameworks
D4937: Add KFileWidget::setSelectedUrl()
dfaure added inline comments. INLINE COMMENTS > fvogt wrote in kfilewidget.h:150 > 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 I guess you mean "with setSelection". I made setSelectedUrl handle only full URLs. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D4937 To: dfaure, fvogt Cc: fvogt, #frameworks
D4937: Add KFileWidget::setSelectedUrl()
fvogt added inline comments. INLINE COMMENTS > dfaure wrote in kfilewidget.h:150 > Very good point. > > But there might be other users of this class who need the ability to select > by filename. > I think this means we have two options: > > - changing setSelection to only handle filenames, including those with a ':' > (bad, not backwards compatible) > - adding a setSelectedFileName(const QString ), and then we can > deprecated setSelection(QString). 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 REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D4937 To: dfaure, fvogt Cc: fvogt, #frameworks
D4937: Add KFileWidget::setSelectedUrl()
dfaure added inline comments. INLINE COMMENTS > fvogt wrote in kfilewidget.h:150 > This function only works correctly for URLs, as filenames can contain ':'s. > However, for those setSelectedUrl is the right function, so I'd mark this > function as deprecated Very good point. But there might be other users of this class who need the ability to select by filename. I think this means we have two options: - changing setSelection to only handle filenames, including those with a ':' (bad, not backwards compatible) - adding a setSelectedFileName(const QString ), and then we can deprecated setSelection(QString). REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D4937 To: dfaure, fvogt Cc: fvogt, #frameworks
D4937: Add KFileWidget::setSelectedUrl()
fvogt requested changes to this revision. fvogt added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > kfilewidget.h:150 > */ > void setSelection(const QString ); > This function only works correctly for URLs, as filenames can contain ':'s. However, for those setSelectedUrl is the right function, so I'd mark this function as deprecated REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D4937 To: dfaure, fvogt Cc: fvogt, #frameworks
D4937: Add KFileWidget::setSelectedUrl()
dfaure created this revision. Restricted Application added a project: Frameworks. REVISION SUMMARY 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 REPOSITORY R241 KIO BRANCH master REVISION DETAIL https://phabricator.kde.org/D4937 AFFECTED FILES src/filewidgets/kfilewidget.cpp src/filewidgets/kfilewidget.h To: dfaure Cc: #frameworks
