Review Request: Correctly handle the cases where a directory (without filename) is passed to KFileDialog::getSave*

Jonathan Marten jjm at keelhaul.me.uk
Thu Jul 2 13:34:05 BST 2009


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/803/
-----------------------------------------------------------

(Updated 2009-07-02 05:34:05.356619)


Review request for kdelibs.


Changes
-------

Know how you feel about getting lost - every routine in kfiledialog.cpp seems to do things slightly differently.  Diff r3 makes everything as consistent as possible by:  (a) not doing anything special with the 'startDir' parameter, just passing it in to KFileWidget unchanged;  (b) setting KFile::LocalOnly for all the *fileName operations where a local file is expected.

I didn't add a kWarning() for the non-local starting place, it was there in a slightly different place already (but only in getSaveFileName, not in getOpenFileName/getOpenFileNames.  Not sure if there is any point to this warning, apart from for application developers.  IMHO KFileWidget is the place to enforce this, but it's difficult to do there because of the separation of the constructor and setMode().


Summary
-------

Fix bug 194900 by simplifying the code in KFileDialog.  For the getSaveFileName/getSaveUrl cases, this originally used to create a KFileDialog and then used setSelection() on it to set the starting location and/or initial file name.  Doing this seems to always insert the basename of the setSelection() argument into the file name field of the dialogue - even if it is not present or is a directory.

Now the starting or special directory is passed directly to the KFileDialog constructor, which handles the cases where this is a directory correctly.  (The KFileWidget constructor does a KIO::stat() to check the filename/path, and decides what to do based on that).  This simplifies the code in KFileDialog too.

Extend the test cases (tests/kfiledialogtest.cpp) to check for these cases.  Since this test has grown quite a bit, add a caption to the dialogue of each sub-test to indicate what is being tested.


This addresses bug 194900.
    https://bugs.kde.org/show_bug.cgi?id=194900


Diffs (updated)
-----

  /trunk/KDE/kdelibs/kio/kfile/kencodingfiledialog.cpp 988105 
  /trunk/KDE/kdelibs/kio/kfile/kfiledialog.cpp 988105 
  /trunk/KDE/kdelibs/kio/kfile/tests/kfiledialogtest.cpp 988105 

Diff: http://reviewboard.kde.org/r/803/diff


Testing
-------

Checked operation of the file dialogue using the test case and the code in the bug report, correct operation observed.


Thanks,

Jonathan





More information about the kde-core-devel mailing list