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,
dfaure closed this revision.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D4937
To: dfaure, fvogt
Cc: fvogt, #frameworks
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
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
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
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
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,
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
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
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
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
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
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,
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
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,
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:
16 matches
Mail list logo
