Review Request 119530: kcoreaddons: Fix kautosave doesn't work with more than 1 file per application
Andreas Xavier
andxav at zoho.com
Mon Aug 11 21:18:57 UTC 2014
> On Aug. 11, 2014, 8:23 a.m., David Faure wrote:
> > Nice patch, I love the ton of unittests :)
Awesome review. Thanks. I think I fixed all the concerns, I ran astyle and posted a new diff.
> On Aug. 11, 2014, 8:23 a.m., David Faure wrote:
> > autotests/kautosavefiletest.cpp, line 56
> > <https://git.reviewboard.kde.org/r/119530/diff/1/?file=294193#file294193line56>
> >
> > 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.
I have run astyle-kdelibs
> On Aug. 11, 2014, 8:23 a.m., David Faure wrote:
> > autotests/kautosavefiletest.cpp, line 60
> > <https://git.reviewboard.kde.org/r/119530/diff/1/?file=294193#file294193line60>
> >
> > why no QVERIFY() here?
init() is not a test. It is only intended to setup a clean test state. If errors are indicated in the initialization code, then it really indicates an error in the previous test or KSomeOtherApplication. It would also mean that if a test crash you would have to run the unittests twice: once to generate the error in the initialization basically telling you that the previous test crashed, and once to get the new results.
> On Aug. 11, 2014, 8:23 a.m., David Faure wrote:
> > src/lib/io/kautosavefile.h, line 162
> > <https://git.reviewboard.kde.org/r/119530/diff/1/?file=294195#file294195line162>
> >
> > How can you drop something you don't have? I'm a bit confused by the wording.
This was bleed through of the implementation detail of keeping the .kalock file, which is required when using kautosavefile as backup. The .kalock file preserves the link between the kautosave file and the real file.
> On Aug. 11, 2014, 8:23 a.m., David Faure wrote:
> > src/lib/io/kautosavefile.h, line 232
> > <https://git.reviewboard.kde.org/r/119530/diff/1/?file=294195#file294195line232>
> >
> > 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).
It was supposed to be a reimplmentation of the remove in QFile. I thought it fell under the
https://techbase.kde.org/Policies/Binary_Compatibility_Issues_With_C++#Adding_a_reimplemented_virtual_function
exception.
open, close, resize and destructor are all virtual. remove is not and I missed that.
Calling remove on an unlocked kautosavefile will remove it regardless of lock, and break the intended usage. I don't see a way around it while preserving compatibility with Qfile.
I added a warning in the header about the breakage.
I added housekeeping code in the stalefile code. If people are passing KAutoSaveFiles around as Qfile (,which is expected usage) something will inevitably call remove(), so housekeeping is required.
I fixed unit tests that called remove() expecting to not be able to remove unlocked files.
I added a unit test of the housekeeping.
> On Aug. 11, 2014, 8:23 a.m., David Faure wrote:
> > src/lib/io/kautosavefile.h, line 273
> > <https://git.reviewboard.kde.org/r/119530/diff/1/?file=294195#file294195line273>
> >
> > 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 ;)
I agree @todo remove is half-baked.
Initially, there were 2 parallel implementations of almost identical behavior with independent bugs.
The semantics for the staleFiles are inconsistent.
Passing no filename returns all the files.
Passing no appname returns files for just this 1 app.
There is no way to get all the autosave files for 1 file for all apps (which you care aboue if multiple apps are using the same library to edit the same file), or to get all the autosave files for all apps.
There is no way to clean up, or even show autosave files that are accumulating in the directory.
The files returned are not necessarily stale, there could be a live locker, which requires looping over all of the files to attempt to lock it before checking if it is the one that you want.
If there is a live locker there is no way to find out, although KAutoSave definitely knows.
There is no way to fetch only crash recovery kautosaves.
I am not sure what the correct mix of functions is, but I'd suggest something like:
getLockInfo(pid, hostname, appname)
static autoSaveFiles(filename = defaults to all filenames
, flags = defaults to all types: locked & unlocked, crashed & not running(clean exit) & running locker
, app = defaults to all apps);
This allows people to do what they want:
Did I crash, while editing zfile?
autoSaveFiles(zfile, Crash, me);
Did anyone crash, while editing zfile?
autoSaveFiles(zfile, Crash);
Is anyone else working on zfile?
autoSaveFiles(zfile, Running);
Are there timed saves for zfile?
autoSaveFiles(zfile, NotRunning & NotCrashed);
How much junk is there in the autosave directory?
autoSaveFiles();
Who do I have to kill to lock this file?
getLockInfo(pid, hostname, appname)
This patch is pretty long as it is, and it does take this from non functional with more than one file to functional. I think fixing this without at least one application in front of me as a use case is foolish. I may submit a further patch/set of patches when I work on the autosave for libKdeEdu.
- Andreas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119530/#review64231
-----------------------------------------------------------
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/13f8cdc1/attachment-0001.html>
More information about the Kde-frameworks-devel
mailing list