----------------------------------------------------------- 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 > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
