[Kde-pim] Review Request: Addition of "Maintenance" tab to the folder properties dialogue

[Thomas McGuire]

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


Ok, I added some comments to the diff, please take them into account.
Please also take Ingo's suggestions into account, especially about not showing
the compact button when it doesn't make sense.

I noticed you left the code to embed the shortcut and expiry tabs as well.
I general, I don't like such code.
If you don't get the OK for embedding those tabs, please make sure to remove
this again.
It can stay in for now (although some inline comments probably say otherwise,
ignore them)


/trunk/KDE/kdepim/kmail/folderstorage.cpp
<http://reviewboard.kde.org/r/48/#comment38>

    Make that 'folder"' instead of 'folder "', kDebug nowadays auto-adds the space.



/trunk/KDE/kdepim/kmail/folderview.cpp
<http://reviewboard.kde.org/r/48/#comment39>

    I think you can remove this comment and the one below



/trunk/KDE/kdepim/kmail/kmfolderdialog.h
<http://reviewboard.kde.org/r/48/#comment51>

    Please rebase the patch on the lasted trunk, I introduced a conflict here, sorry.



/trunk/KDE/kdepim/kmail/kmfolderdialog.h
<http://reviewboard.kde.org/r/48/#comment40>

    Worth moving to the base class, maybe?



/trunk/KDE/kdepim/kmail/kmfolderdialog.h
<http://reviewboard.kde.org/r/48/#comment42>

    Is this method used now or a leftover from an old patch?



/trunk/KDE/kdepim/kmail/kmfolderdialog.h
<http://reviewboard.kde.org/r/48/#comment41>

    Leftovers from an earlier patch? If so, please remove.



/trunk/KDE/kdepim/kmail/kmfolderdialog.cpp
<http://reviewboard.kde.org/r/48/#comment43>

    Use 0, not NULL, and initalize them in the initalizer list.
    Same for other places where NULL is used



/trunk/KDE/kdepim/kmail/kmfolderdialog.cpp
<http://reviewboard.kde.org/r/48/#comment45>

    Minor coding style: align the parameter with the first one (indentation)



/trunk/KDE/kdepim/kmail/kmfolderdialog.cpp
<http://reviewboard.kde.org/r/48/#comment44>

    to the initalizer list, please.



/trunk/KDE/kdepim/kmail/kmfolderdialog.cpp
<http://reviewboard.kde.org/r/48/#comment46>

    minor coding style: please camelCase variable names (also in other places)



/trunk/KDE/kdepim/kmail/kmfolderdialog.cpp
<http://reviewboard.kde.org/r/48/#comment49>

    coding style: please indent those, and add the things after the case statement in a seprate line.
    Same for other switch statements



/trunk/KDE/kdepim/kmail/kmfolderdialog.cpp
<http://reviewboard.kde.org/r/48/#comment52>

    "Tasks" instead of "Task", right? Also the others. Code sharing with the General tab possible, who also has these strings?



/trunk/KDE/kdepim/kmail/kmfolderdialog.cpp
<http://reviewboard.kde.org/r/48/#comment50>

    QString() instead of QString::null
    Also in other places



/trunk/KDE/kdepim/kmail/kmfolderdialog.cpp
<http://reviewboard.kde.org/r/48/#comment53>

    probably need i18n context here



/trunk/KDE/kdepim/kmail/kmfolderdialog.cpp
<http://reviewboard.kde.org/r/48/#comment47>

    It is there, according to http://api.kde.org/4.x-api/kdelibs-apidocs/kio/html/namespacemembers_0x63.html#index_c



/trunk/KDE/kdepim/kmail/kmfolderdialog.cpp
<http://reviewboard.kde.org/r/48/#comment54>

    I think we already have code to get the folder size somewhere (like it is shown in the folder tree), why not use that?



/trunk/KDE/kdepim/kmail/kmmainwidget.cpp
<http://reviewboard.kde.org/r/48/#comment48>

    Completey remove the action, I'd say


- Thomas


On 2009-02-09 06:24:23, Jonathan Marten wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/48/
> -----------------------------------------------------------
> 
> (Updated 2009-02-09 06:24:23)
> 
> 
> Review request for KDE PIM.
> 
> 
> Summary
> -------
> 
> Addition of "Maintenance" tab to the folder properties dialogue.  This displays some information about the folder and index files and the compaction status.  The actions "Rebuild Index" and "Rebuild IMAP Cache" have been moved from the folder popup menu to here.
> 
> Added function enableCompaction() to folderstorage.{cpp,h} to allow the folder to be compacted (after user confirmation) if the "compaction unsafe" flag has been set.  There was previously no way to do this apart from editing the kmailrc configuration file.
> 
> Removed slotAssignShortcut() from folderview.{cpp,h} - the corresponding action from the main window is used instead.
> 
> Moved the "mark_all_as_read" action back to the top of the folder popup menu, as suggested by aseigo.
> 
> 
> This addresses bug 115611.
>     https://bugs.kde.org/show_bug.cgi?id=115611
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdepim/kmail/folderstorage.h 923792 
>   /trunk/KDE/kdepim/kmail/folderstorage.cpp 923792 
>   /trunk/KDE/kdepim/kmail/folderview.h 923792 
>   /trunk/KDE/kdepim/kmail/folderview.cpp 923792 
>   /trunk/KDE/kdepim/kmail/identitydialog.cpp 923792 
>   /trunk/KDE/kdepim/kmail/kmfolderdialog.h 923792 
>   /trunk/KDE/kdepim/kmail/kmfolderdialog.cpp 923792 
>   /trunk/KDE/kdepim/kmail/kmmainwidget.h 923792 
>   /trunk/KDE/kdepim/kmail/kmmainwidget.cpp 923792 
> 
> Diff: http://reviewboard.kde.org/r/48/diff
> 
> 
> Testing
> -------
> 
> Built KMail from trunk with these patches, checked operation of all functions.
> 
> 
> Thanks,
> 
> Jonathan
> 
>

_______________________________________________
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/