Review Request 121099: 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/121099/#review70440
-----------------------------------------------------------


I am still very unsure about adding all this complexity with two lock files, some of it in order to add a second feature to KAutoSaveFile ("backups"). That doesn't really make this patch just a bugfix....

The docu patch mentions two usage scenarios, but the second one is about backups, not autosave. The backup feature belongs to another class (see KBackup).

I would much rather see a patch for bugfixing the established use case for KAutoSaveFile (autosaving in order to recover unsaved data after a crash), and just that, no "let's add another feature while at it".
I might be missing something though, please enlighten me in that case.


autotests/kautosavefiletest.cpp
<https://git.reviewboard.kde.org/r/121099/#comment49256>

    two spaces before "



autotests/kautosavefiletest.cpp
<https://git.reviewboard.kde.org/r/121099/#comment49257>

    two spaces after comma



autotests/kautosavefiletest.cpp
<https://git.reviewboard.kde.org/r/121099/#comment49258>

    same - and there's another dozen that follow ;)



autotests/kautosavefiletest.cpp
<https://git.reviewboard.kde.org/r/121099/#comment49260>

    this instance isn't deleted anywhere, it seems



autotests/kautosavefiletest.cpp
<https://git.reviewboard.kde.org/r/121099/#comment49259>

    Better create on stack, for auto cleanup.



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

    two spaces after removed



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

    typo: destrctor -> destructor ; comma should be at end of last line



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

    This is for the first "usage scenario" of the previous paragraph, right? It's a bit confusing right now, to detail steps without associating them with the corresponding usage scenario.
    
    Or does this work for both? I don't really understand the second use case.


- David Faure


On Nov. 11, 2014, 1:16 a.m., Jeremy Whiting wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121099/
> -----------------------------------------------------------
> 
> (Updated Nov. 11, 2014, 1:16 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.
> 
> Original patch by Andreas Xavier (https://git.reviewboard.kde.org/r/119530)
> 
> Items from original review have been fixed.
> 
> 
> Diffs
> -----
> 
>   src/lib/io/kautosavefile_p.cpp PRE-CREATION 
>   autotests/kautosavefiletest.h cf70f4c6a4e1f093c431eff6ff25e6f510e84a53 
>   autotests/kautosavefiletest.cpp ec0309e5fda95e01bbed31bd2fe91f9b7a48bec0 
>   src/lib/CMakeLists.txt 1dc56270ab5157af706b7745cfa88ae11e16184a 
>   src/lib/io/kautosavefile.h 05cc3aedc248665c8727ade3b86b524275c013ca 
>   src/lib/io/kautosavefile.cpp 13a13d7cfb26194be3edf7cbdcd1d39309b79465 
>   src/lib/io/kautosavefile_p.h PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/121099/diff/
> 
> 
> Testing
> -------
> 
> It builds and the test passes.
> 
> 
> Thanks,
> 
> Jeremy Whiting
> 
>

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