DevHeads.net

Review Request 119014: KUrlRequester: fixing handling of start directory

Review request for kdelibs.

Bugs: 92237
<a href="http://bugs.kde.org/show_bug.cgi?id=92237" title="http://bugs.kde.org/show_bug.cgi?id=92237">http://bugs.kde.org/show_bug.cgi?id=92237</a>

Repository: kdelibs

Description
The handling of the start directory in the KUrlRequester is only halfway implemented.
More in detail:
- m_startDir (the private field holding the start directory) is not initialised - the default value of startDir() is an empty URL instead of the current working dir.
- when the start dir changes, it is not always passed to the KUrlCompletion object of the lineedit. The suggestions showed when entering a relative path into the LineEdit might be wrong as a consequence.
- when selecting a file, the start directory does not change to the directory of the selected file - the API doc says it should.
- when the user entered a relative path into the LineEdit, url() returns a relative path instead of an absolute one.

This patch should fix these issues.

Diffs
kio/kfile/kurlrequester.h 2083d4c
kio/kfile/kurlrequester.cpp 661b428

Diff: <a href="https://git.reviewboard.kde.org/r/119014/diff/" title="https://git.reviewboard.kde.org/r/119014/diff/">https://git.reviewboard.kde.org/r/119014/diff/</a>

Testing

Thanks,

Simon Bachmann

Comments

Re: Review Request 119014: KUrlRequester: fixing handling of sta

By Simon Bachmann at 08/01/2014 - 14:39

(Updated Aug. 1, 2014, 9:39 nachm.)

Review request for kdelibs.

Bugs: 92237
<a href="http://bugs.kde.org/show_bug.cgi?id=92237" title="http://bugs.kde.org/show_bug.cgi?id=92237">http://bugs.kde.org/show_bug.cgi?id=92237</a>

Repository: kdelibs

Description
The handling of the start directory in the KUrlRequester is only halfway implemented.
More in detail:
- m_startDir (the private field holding the start directory) is not initialised - the default value of startDir() is an empty URL instead of the current working dir.
- when the start dir changes, it is not always passed to the KUrlCompletion object of the lineedit. The suggestions showed when entering a relative path into the LineEdit might be wrong as a consequence.
- when selecting a file, the start directory does not change to the directory of the selected file - the API doc says it should.
- when the user entered a relative path into the LineEdit, url() returns a relative path instead of an absolute one.

This patch should fix these issues.

Diffs (updated)
kio/kfile/kurlrequester.h 2083d4c
kio/kfile/kurlrequester.cpp 661b428

Diff: <a href="https://git.reviewboard.kde.org/r/119014/diff/" title="https://git.reviewboard.kde.org/r/119014/diff/">https://git.reviewboard.kde.org/r/119014/diff/</a>

Testing

Thanks,

Simon Bachmann

Re: Review Request 119014: KUrlRequester: fixing handling of sta

By Simon Bachmann at 10/19/2014 - 04:18

(Updated Oct. 19, 2014, 9:18 a.m.)

Status
This change has been marked as submitted.

Review request for kdelibs.

Bugs: 92237
<a href="http://bugs.kde.org/show_bug.cgi?id=92237" title="http://bugs.kde.org/show_bug.cgi?id=92237">http://bugs.kde.org/show_bug.cgi?id=92237</a>

Repository: kdelibs

Description
The handling of the start directory in the KUrlRequester is only halfway implemented.
More in detail:
- m_startDir (the private field holding the start directory) is not initialised - the default value of startDir() is an empty URL instead of the current working dir.
- when the start dir changes, it is not always passed to the KUrlCompletion object of the lineedit. The suggestions showed when entering a relative path into the LineEdit might be wrong as a consequence.
- when selecting a file, the start directory does not change to the directory of the selected file - the API doc says it should.
- when the user entered a relative path into the LineEdit, url() returns a relative path instead of an absolute one.

This patch should fix these issues.

Diffs
kio/kfile/kurlrequester.h 2083d4c
kio/kfile/kurlrequester.cpp 661b428

Diff: <a href="https://git.reviewboard.kde.org/r/119014/diff/" title="https://git.reviewboard.kde.org/r/119014/diff/">https://git.reviewboard.kde.org/r/119014/diff/</a>

Testing

Thanks,

Simon Bachmann

Re: Review Request 119014: KUrlRequester: fixing handling of sta

By David Faure at 10/19/2014 - 04:29

I think this misses the "remember url as defaultStartDir and update startdir for autocompletion" code path for the case of the directory-selection dialog (KUrlRequester::KUrlRequesterPrivate::_k_slotOpenDialog, see emit m_parent->urlSelected line, just like the other one where you added code -> that code should be factorized)

- David Faure

On Oct. 19, 2014, 9:18 a.m., Simon Bachmann wrote:

Re: Review Request 119014: KUrlRequester: fixing handling of sta

By David Faure at 10/19/2014 - 04:12

Ship it!

Sorry for the delay -- too many review requests....

I'll commit this.

- David Faure

On Aug. 1, 2014, 7:39 p.m., Simon Bachmann wrote:

Re: Review Request 119014: KUrlRequester: fixing handling of sta

By David Faure at 07/14/2014 - 18:15

kio/kfile/kurlrequester.cpp
<https://git.reviewboard.kde.org/r/119014/#comment43312>

the space before '(' shouldn't be there

kio/kfile/kurlrequester.cpp
<https://git.reviewboard.kde.org/r/119014/#comment43313>

move declaration further down to where it's first used (i.e. before if(comp)

kio/kfile/kurlrequester.cpp
<https://git.reviewboard.kde.org/r/119014/#comment43314>

move down, closer to first use (this is not C)

kio/kfile/kurlrequester.cpp
<https://git.reviewboard.kde.org/r/119014/#comment43320>

Calling toLocalFile on a relative url seems wrong to me (relative = no scheme, while toLocalFile() is for urls with the scheme "file"). (I'm even surprised this works...)

I guess you want path() instead.

kio/kfile/kurlrequester.cpp
<https://git.reviewboard.kde.org/r/119014/#comment43315>

merge with previous line

kio/kfile/kurlrequester.cpp
<https://git.reviewboard.kde.org/r/119014/#comment43319>

The notion of the "current path" in a GUI program (started graphically) makes little sense to me, but I'm not sure what this should be instead .... so if this is closer to the earlier behavior, I won't object.

Alternatives would be the home dir or the document path...

kio/kfile/kurlrequester.cpp
<https://git.reviewboard.kde.org/r/119014/#comment43316>

Many things in KIO break with relative urls. In my mind, a KUrl is almost always absolute, even though isRelative() indeed exists. I would not worry about it, or at most I would replace the comment with
if (startDir.isRelative()) {
kWarning() << "relative URLs are not supported:" << startDir;
return;
}

kio/kfile/kurlrequester.cpp
<https://git.reviewboard.kde.org/r/119014/#comment43317>

unbalanced spaces inside ()

space after ! shouldn't be there

kio/kfile/kurlrequester.cpp
<https://git.reviewboard.kde.org/r/119014/#comment43318>

upUrl has a lot more magic than what you want here (e.g. stripping fragments, etc.).

Do this instead:
m_startDir = newUrl;
m_startDir.setPath(m_startDir.directory());

and when you later port this to Qt5/KF5:
m_startDir = newUrl.adjusted(QUrl::RemoveFilename);

- David Faure

On June 29, 2014, 3:04 p.m., Simon Bachmann wrote:

Re: Review Request 119014: KUrlRequester: fixing handling of sta

By Simon Bachmann at 07/15/2014 - 14:45

Well, I'm just implementing what the api doc says: "The default [...] directory is the current working directory". For graphically started programs the current path usually is $HOME anyway, AFAIK.

- Simon

On Juni 29, 2014, 5:04 nachm., Simon Bachmann wrote:

Re: Review Request 119014: KUrlRequester: fixing handling of sta

By David Faure at 07/09/2014 - 03:40

Looks good, but I'd feel safer if the unittest (kio/tests/kurlrequestertest.cpp) was:
1) checked for no regressions, and
2) extended to cover the case of relative paths, and other things this patch is fixing.

kio/kfile/kurlrequester.h
<https://git.reviewboard.kde.org/r/119014/#comment43132>

trailing space (here and everywhere reviewboard shows red - please fix)

- David Faure

On June 29, 2014, 3:04 p.m., Simon Bachmann wrote:

Re: Review Request 119014: KUrlRequester: fixing handling of sta

By David Faure at 07/14/2014 - 18:15

Oops. I'm sorry, this is indeed not a unittest, but a GUI test program. In KF5 we have cleanly separated the two (tests vs autotests), but in kdelibs4 it was all mixed up.

I was confusing (in my head) with the existing unittests for KUrlCompletion (as used by kurlrequester), kio/tests/kurlcompletiontest.cpp.

I suppose a unittest for kurlrequester *could* be written though, by inspecting the fileDialog() to see how it got set up by KUrlRequester. But due to the lack of an existing test to start from, I can't force you to write additional tests as part of this change :)
Still, if you feel like writing a unittest for kurlrequester, that would be much appreciated.

- David

On June 29, 2014, 3:04 p.m., Simon Bachmann wrote:

Re: Review Request 119014: KUrlRequester: fixing handling of sta

By Simon Bachmann at 07/14/2014 - 16:15

I need some help with the unittest:
I do understand the concept of "regular" unittests - the ones where you basically test output against expected values.
However, kurlrequestertest.cpp (and many other tests in kio/tests) seems to be a different kind of test, and I'm not sure I fully understand how they work.
It is my understanding that the test in question is interactive, i.e. the tester has to click through a series of dialogs showing variations of the widget in question and decide if the test passed, based on comments in the source, behaviour of the widget and console output.
Is that correct?
Are there any guidelines / best practices for these tests?
Thanks

- Simon

On Juni 29, 2014, 5:04 nachm., Simon Bachmann wrote: