Review Request 119530: kcoreaddons: Fix kautosave doesn't work with more than 1 file per application

[David Faure]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119530/#review64231
-----------------------------------------------------------


Nice patch, I love the ton of unittests :)


autotests/kautosavefiletest.cpp
<https://git.reviewboard.kde.org/r/119530/#comment44882>

    This isn't needed. init() is called before every test method. So need to call it once more at the very beginning (initTestCase).



autotests/kautosavefiletest.cpp
<https://git.reviewboard.kde.org/r/119530/#comment44883>

    Coding style: please remove spaces within parentheses (use search/replace or run the astyle-kdelibs script after reading the howto in there). kdelibs4 was very inconsistent, but KF5 is very consistent.



autotests/kautosavefiletest.cpp
<https://git.reviewboard.kde.org/r/119530/#comment44884>

    why no QVERIFY() here?



autotests/kautosavefiletest.cpp
<https://git.reviewboard.kde.org/r/119530/#comment44885>

    same



autotests/kautosavefiletest.cpp
<https://git.reviewboard.kde.org/r/119530/#comment44886>

    I think you can remove these two stubs, given all the tests you added :)



src/lib/io/kautosavefile.h
<https://git.reviewboard.kde.org/r/119530/#comment44887>

    Well, it's less convenient for people who want a non-pointer member, for instance.
    Look at QFile: it has a QFile(QObject * parent) constructor, and a setFileName method to set the filename. Whatever one might think of the usefulness of that, I think it's good for KAutoSaveFile to be close to QFile.
    So I would suggest to remove these two lines.



src/lib/io/kautosavefile.h
<https://git.reviewboard.kde.org/r/119530/#comment44888>

    How can you drop something you don't have? I'm a bit confused by the wording.



src/lib/io/kautosavefile.h
<https://git.reviewboard.kde.org/r/119530/#comment44889>

    Ideally the two usage scenarios would be described in the class documentation, rather than only in the destructor.



src/lib/io/kautosavefile.h
<https://git.reviewboard.kde.org/r/119530/#comment44891>

    (as above, I'm not sure about this todo)



src/lib/io/kautosavefile.h
<https://git.reviewboard.kde.org/r/119530/#comment44892>

    Ah, but no... you cannot add new virtual methods, that's binary incompatible.
    
    Does it have to be virtual?
    
    Adding a non-virtual method is fine (add a @since 5.2 to its documentation).



src/lib/io/kautosavefile.h
<https://git.reviewboard.kde.org/r/119530/#comment44893>

    If you want a method to be removed later (= KF6), you can already mark it as deprecated. But isn't allStaleFiles(app) nicer to read/write than staleFiles(QUrl(), app)? When reading such code, it's not clear why there's an empty url argument, while allStaleFiles is clear to the reader.
    Whether the method is useful, though, I don't know.
    
    In any case, let's decide: either keep, or deprecate. A @todo remove is kind of a half-baked decision which will never happen ;)



src/lib/io/kautosavefile.cpp
<https://git.reviewboard.kde.org/r/119530/#comment44894>

    The usual naming for this (in all of Qt and KF5) is rather kautosavefile_p.h



src/lib/io/kautosavefile.cpp
<https://git.reviewboard.kde.org/r/119530/#comment44902>

    Isn't the "autosavefile location" the same as this foo.kalock file's path but without the .kalock extension? Then I don't see much point in storing it inside the .kalock file again.



src/lib/io/kautosavefile.cpp
<https://git.reviewboard.kde.org/r/119530/#comment44895>

    16? I think you mean 8.
    2^3=8 and your table below has 8 lines, coincidentally ;)



src/lib/io/kautosavefileprivate.h
<https://git.reviewboard.kde.org/r/119530/#comment44896>

    no need for if before delete, delete null is valid (and a no-op)



src/lib/io/kautosavefileprivate.h
<https://git.reviewboard.kde.org/r/119530/#comment44897>

    Hihi, goodOpen, that's so French ;)



src/lib/io/kautosavefileprivate.cpp
<https://git.reviewboard.kde.org/r/119530/#comment44899>

    coding style: no spaces within (...) and also no space before commas  [this applies to the whole patch]



src/lib/io/kautosavefileprivate.cpp
<https://git.reviewboard.kde.org/r/119530/#comment44898>

    if it's not open, no need to close it



src/lib/io/kautosavefileprivate.cpp
<https://git.reviewboard.kde.org/r/119530/#comment44900>

    there's a fromLatin1(QByteArray) in Qt5 => you can and should remove the .constData()



src/lib/io/kautosavefileprivate.cpp
<https://git.reviewboard.kde.org/r/119530/#comment44901>

    them -> they


- David Faure


On July 29, 2014, 9:54 a.m., Andreas Xavier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/119530/
> -----------------------------------------------------------
> 
> (Updated July 29, 2014, 9:54 a.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> -------
> 
> kautosave doesn't work when any app tries to use a second filename, because it doesn't filter on filename.  The unit tests can be droppped into master to show the problem, if you remove the include on line 21.
> 
> This patch:
> 1. Adds unit tests to test more behavior mentioned in the header.
> 2. Fixes kautosave working with multple files per application.
> 3. Fixes filenaming brittleness, which would cause kautosave to randomly fail when the last 3 randomly generated characters in the filename matched any 3 consequtive chracters in the managed filename.
> 
> 
> Diffs
> -----
> 
>   src/lib/io/kautosavefile.cpp 13a13d7 
>   src/lib/io/kautosavefileprivate.h PRE-CREATION 
>   src/lib/io/kautosavefileprivate.cpp PRE-CREATION 
>   autotests/kautosavefiletest.h cf70f4c 
>   autotests/kautosavefiletest.cpp ec0309e 
>   src/lib/CMakeLists.txt 26eb5a1 
>   src/lib/io/kautosavefile.h 05cc3ae 
> 
> Diff: https://git.reviewboard.kde.org/r/119530/diff/
> 
> 
> Testing
> -------
> 
> Ran unit tests.
> Ran kdeedu with kanagram.
> 
> 
> Thanks,
> 
> Andreas Xavier
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20140811/31f05e52/attachment-0001.html>