[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-core-devel
Subject:    Re: Review Request: Correctly handle the cases where a directory
From:       "Jonathan Marten" <jjm () keelhaul ! me ! uk>
Date:       2009-07-02 12:34:05
Message-ID: 20090702123405.22093.73937 () localhost
[Download RAW message or body]


-----------------------------------------------------------
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


[prev in list] [next in list] [prev in thread] [next in thread]