[Kde-pim] Review Request:

[Thomas McGuire]

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


I like having a "Open With" button, was missing that in several cases already.
What I don't really like is the strange mix of KMessagebox and KDialog, can't this be made easier, like only a KMessageBox?


trunk/KDE/kdepim/kmail/attachmentdialog.h
<http://reviewboard.kde.org/r/1219/#comment1248>

    Please enclose the class in the KMail namespace, to avoid symbol clashes.



trunk/KDE/kdepim/kmail/attachmentdialog.h
<http://reviewboard.kde.org/r/1219/#comment1247>

    why don't you derive from KDialog directly?
    Then you don't need your custom exec(), KDialog pointer and such.
    Actually, I don't see why a KDialog should be used either, I thought KMessagebox can handle all this?



trunk/KDE/kdepim/kmail/attachmentdialog.h
<http://reviewboard.kde.org/r/1219/#comment1246>

    coding style: spaces inside of parenthesis



trunk/KDE/kdepim/kmail/attachmentdialog.cpp
<http://reviewboard.kde.org/r/1219/#comment1245>

    Missing copyright.



trunk/KDE/kdepim/kmail/attachmentdialog.cpp
<http://reviewboard.kde.org/r/1219/#comment1251>

    read http://techbase.kde.org/Development/Tutorials/Localization/i18n_Semantics.
    after that, you can use much better markup than '%1'. But that is no requirement, only a suggestion.



trunk/KDE/kdepim/kmail/attachmentdialog.cpp
<http://reviewboard.kde.org/r/1219/#comment1249>

    !isEmpty() seems more easy to read



trunk/KDE/kdepim/kmail/attachmentdialog.cpp
<http://reviewboard.kde.org/r/1219/#comment1250>

    I thought the don't ask again stuff is handled implicitly by kmessagebox?



trunk/KDE/kdepim/kmail/kmreaderwin.cpp
<http://reviewboard.kde.org/r/1219/#comment1243>

    QString() instead of ""



trunk/KDE/kdepim/kmail/kmreaderwin.cpp
<http://reviewboard.kde.org/r/1219/#comment1244>

    coding style: spaces inside of parenthesis, e.g. ( choice == AttachmentDialog::Open ) instead of (choice == AttachmentDialog::Open) (also in other places)


- Thomas


On 2009-08-02 21:14:45, Martin Koller wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1219/
> -----------------------------------------------------------
> 
> (Updated 2009-08-02 21:14:45)
> 
> 
> Review request for KDE PIM.
> 
> 
> Summary
> -------
> 
> According to https://bugs.kde.org/show_bug.cgi?id=201374
> add the "Open With ..." button to the attachment handling dialog even when KDE found a matching application
> to open the attachment with.
> 
> 
> This addresses bug 201374.
>     https://bugs.kde.org/show_bug.cgi?id=201374
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdepim/kmail/CMakeLists.txt 1005628 
>   trunk/KDE/kdepim/kmail/attachmentdialog.h PRE-CREATION 
>   trunk/KDE/kdepim/kmail/attachmentdialog.cpp PRE-CREATION 
>   trunk/KDE/kdepim/kmail/kmreaderwin.cpp 1005719 
> 
> Diff: http://reviewboard.kde.org/r/1219/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Martin
> 
>

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