[Kde-pim] Review Request: KMail composer autosave functionality

[Thomas McGuire]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/2556/#review3664
-----------------------------------------------------------


Matthew, thanks for the patch. I have not tested it yet, but it looks like autosave is revived correctly :)

As usual, I have made some comments below, inline to the diff. There are a few improvements which can be made from just merely reviving the autosave function to making it look nice as well.

I'd like to see the autosave recovery patch on startup integrated with this patch, so that both functions can be tested together before committing.

So please work on the comments posted below, make the recovery code work, and then post a new patch.

Welcome to the KDE PIM team!


/branches/work/akonadi-ports/kdepim/kmail/kmcomposewin.h
<http://reviewboard.kde.org/r/2556/#comment3004>

    Can you add very short API docs to say what the parameters are, especially msgNumber?
    
    



/branches/work/akonadi-ports/kdepim/kmail/kmcomposewin.cpp
<http://reviewboard.kde.org/r/2556/#comment3014>

    Can the applyAutoSave() function be merged with this function?
    
    Having so many autosave related functions is a bit confusing.



/branches/work/akonadi-ports/kdepim/kmail/kmcomposewin.cpp
<http://reviewboard.kde.org/r/2556/#comment3000>

    Please remove all the trailing spaces in this patch. Reviewboard marks them with these red markers, but you can also configure your editor to show trailing spaces.



/branches/work/akonadi-ports/kdepim/kmail/kmcomposewin.cpp
<http://reviewboard.kde.org/r/2556/#comment3005>

    You don't need the space at the end of the string, kDebug() inserts that automatically.



/branches/work/akonadi-ports/kdepim/kmail/kmcomposewin.cpp
<http://reviewboard.kde.org/r/2556/#comment3006>

    Errors for writing should also be checked, not only errors when opening.
    finalize() should be checked as well I guess.



/branches/work/akonadi-ports/kdepim/kmail/kmcomposewin.cpp
<http://reviewboard.kde.org/r/2556/#comment3007>

    In the old version, the messagebox was only shown once, as the old comment indicates.
    
    That should still be the case, otherwise a message box coming up every minute would be very annoying.



/branches/work/akonadi-ports/kdepim/kmail/kmcomposewin.cpp
<http://reviewboard.kde.org/r/2556/#comment3008>

    Looks like the variable mLastAutoSaveErrno can be removed now, it is unused.
    
    But something similar, maybe a simple bool, is still needed, so that the message box doesn't show up twice in case of an error.



/branches/work/akonadi-ports/kdepim/kmail/kmcomposewin.cpp
<http://reviewboard.kde.org/r/2556/#comment3009>

    Why is the autosave timer started here? Doesn't being in this if branch mean the autosave is active, and therefore the autosave timer should be disabled?



/branches/work/akonadi-ports/kdepim/kmail/kmcomposewin.cpp
<http://reviewboard.kde.org/r/2556/#comment3016>

    for const correctness, this should be "const Composer*", if possible.



/branches/work/akonadi-ports/kdepim/kmail/kmcomposewin.cpp
<http://reviewboard.kde.org/r/2556/#comment3011>

    Would probably be better to print a kWarning() in the error case instead :)



/branches/work/akonadi-ports/kdepim/kmail/kmcomposewin.cpp
<http://reviewboard.kde.org/r/2556/#comment3015>

    I'd suggest to rename continueAutoSave() to something like writeMessageToDisk(), for clarity.
    
    The continueAutoSave() function is just named like that for historical reasons.
    
    (Note that the code which changes the timer in the continueAutoSave() function should be moved to this function, then)



/branches/work/akonadi-ports/kdepim/kmail/kmcomposewin.cpp
<http://reviewboard.kde.org/r/2556/#comment3010>

    Add two spaces in front so it is indented correctly.



/branches/work/akonadi-ports/kdepim/kmail/kmcomposewin.cpp
<http://reviewboard.kde.org/r/2556/#comment3002>

    Having a variable called "mAutoSaveFilename", which in reality is a UUID, is confusing.



/branches/work/akonadi-ports/kdepim/kmail/kmcomposewin.cpp
<http://reviewboard.kde.org/r/2556/#comment3013>

    Coding style: Align the "<<" with the first one of the line above



/branches/work/akonadi-ports/kdepim/kmail/kmcomposewin.cpp
<http://reviewboard.kde.org/r/2556/#comment3012>

    Coding style: Personally I'd prefer
    
    foreach( const QString &file, autoSaveFiles )


- Thomas


On 2010-01-11 14:29:03, Matthew Leach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/2556/
> -----------------------------------------------------------
> 
> (Updated 2010-01-11 14:29:03)
> 
> 
> Review request for KDE PIM.
> 
> 
> Summary
> -------
> 
> This patch will add autosave functionality to the KMail composer window.
> 
> 
> Diffs
> -----
> 
>   /branches/work/akonadi-ports/kdepim/kmail/composer.h 1073034 
>   /branches/work/akonadi-ports/kdepim/kmail/kmcomposewin.h 1073034 
>   /branches/work/akonadi-ports/kdepim/kmail/kmcomposewin.cpp 1073034 
> 
> Diff: http://reviewboard.kde.org/r/2556/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Matthew
> 
>

_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/