[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-pim
Subject: Re: [Kde-pim] Review Request: Allow creation of new top level
From: "Thomas McGuire" <mcguire () kde ! org>
Date: 2009-02-17 10:29:53
Message-ID: 20090217102953.14715.12080 () localhost
[Download RAW message or body]
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/105/#review145
-----------------------------------------------------------
Thanks, patch looks fine, with a few minor coding style thingies and one little bug, \
which I've added inline comments for, to the diff.
trunk/KDE/kdepim/kmail/folderselectiontreewidget.h
<http://reviewboard.kde.org/r/105/#comment72>
Please add API doc here and above, as new functions should be documented.
trunk/KDE/kdepim/kmail/folderselectiontreewidget.h
<http://reviewboard.kde.org/r/105/#comment71>
In KMail, members are prefixed with m, normally, so make it mCreateFolderAction.
Also, there is already a private section above, which Szymon confusingly added to \
the start of the function.
trunk/KDE/kdepim/kmail/folderselectiontreewidget.cpp
<http://reviewboard.kde.org/r/105/#comment70>
This change looks wrong to me.
The logic here is: If the item has no folder or if it has a folder and that \
folder is readonly, disable selection for that item.
trunk/KDE/kdepim/kmail/folderselectiontreewidget.cpp
<http://reviewboard.kde.org/r/105/#comment73>
Remove these trailing spaces here (nice that reviewboards marks them in red)
trunk/KDE/kdepim/kmail/folderselectiontreewidget.cpp
<http://reviewboard.kde.org/r/105/#comment74>
Coding style: spaces at inside of parenthesis
trunk/KDE/kdepim/kmail/folderselectiontreewidget.cpp
<http://reviewboard.kde.org/r/105/#comment75>
Put the assignment in a seperate line from the if statement, easier to read that \
way.
- Thomas
On 2009-02-16 10:31:03, Jonathan Marten wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/105/
> -----------------------------------------------------------
>
> (Updated 2009-02-16 10:31:03)
>
>
> Review request for KDE PIM.
>
>
> Summary
> -------
>
> Fix for the old problem of not being able to create a new subfolder in certain \
> places when using the folder selection tree view - in particular, not being able to \
> create a new top level local folder.
> Also tidied up the operation of this dialogue, centralising the decisions in \
> FolderSelectionTreeWidget instead of having them duplicated in \
> FolderSelectionDialog. Changed the signal/slot connections between the two. Use a \
> KAction for the popup menu and enable/disable it as required.
> Disable the "OK" button in the dialogue where it is not allowed. Previously a \
> folder with "No Content", e.g. the "Local Folders" or an IMAP root, could be \
> selected as a filter or move/copy destination.
>
> This addresses bug 128796.
> https://bugs.kde.org/show_bug.cgi?id=128796
>
>
> Diffs
> -----
>
> trunk/KDE/kdepim/kmail/folderselectiondialog.h 926798
> trunk/KDE/kdepim/kmail/folderselectiondialog.cpp 926798
> trunk/KDE/kdepim/kmail/folderselectiontreewidget.h 926798
> trunk/KDE/kdepim/kmail/folderselectiontreewidget.cpp 926798
>
> Diff: http://reviewboard.kde.org/r/105/diff
>
>
> Testing
> -------
>
> Checked operation of all uses of FolderSelectionDialog or FolderRequester for \
> correct enable/disable of "OK" and "New Subfolder" buttons.
> Checked creation of new subfolder where this is allowed.
>
>
> Thanks,
>
> Jonathan
>
>
_______________________________________________
KDE PIM mailing list kde-pim@kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic