[Kde-pim] Review Request 119462: Per attendee customization of email [4] - individual mail dialog

[Kevin Krammer]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119462/#review63145
-----------------------------------------------------------



incidenceeditor-ng/individualmaildialog.h
<https://git.reviewboard.kde.org/r/119462/#comment43900>

    indentation looks off by three



incidenceeditor-ng/individualmaildialog.h
<https://git.reviewboard.kde.org/r/119462/#comment43899>

    getters should be const



incidenceeditor-ng/individualmaildialog.cpp
<https://git.reviewboard.kde.org/r/119462/#comment43901>

    remember @action:button :)



incidenceeditor-ng/individualmaildialog.cpp
<https://git.reviewboard.kde.org/r/119462/#comment43902>

    you can do
    new QGridLayout(widget) instead of creating the layout without parent and then calling widget->setLayout



incidenceeditor-ng/individualmaildialog.cpp
<https://git.reviewboard.kde.org/r/119462/#comment43906>

    we treat the foreach macro like a variation of the for keyword, so space after foreach



incidenceeditor-ng/individualmaildialog.cpp
<https://git.reviewboard.kde.org/r/119462/#comment43905>

    hmm, I am not sure, but I think at least the first word's first letter should be uppercase.
    and ideally @item:inlistbox, see https://techbase.kde.org/Development/Tutorials/Localization/i18n_Semantics#Context_Markers
    and space after comma



incidenceeditor-ng/individualmaildialog.cpp
<https://git.reviewboard.kde.org/r/119462/#comment43907>

    that looks fragile, a change to the string in the constructor would make the check fail.
    
    My suggestion is to define an enum for the three options and use the enum as the index value on QComboBox::insertItem (instead of addItem). Then you can compare currentIndex() against the enum value for "edit mail"



incidenceeditor-ng/individualmailjobfactory.h
<https://git.reviewboard.kde.org/r/119462/#comment43908>

    or maybe not have defaults at the method declarations at all, just have them were these methods are called?



incidenceeditor-ng/individualmailjobfactory.cpp
<https://git.reviewboard.kde.org/r/119462/#comment43910>

    it looks like you only needs those two variables for checking "is an email in this list".
    So maybe the better type would be QSet<QString>?



incidenceeditor-ng/individualmailjobfactory.cpp
<https://git.reviewboard.kde.org/r/119462/#comment43909>

    Hmm.
    As Sergio noted on the other review, it is unfortunate that we duplicate code between Person/Attendee methods and this function.
    What if we could use Person/Attendee here, we could avoid further usage of this util function and potentially remove it at some point. What do others think?



incidenceeditor-ng/individualmailjobfactory.cpp
<https://git.reviewboard.kde.org/r/119462/#comment43911>

    any specific reason the openDialog function doesn't takte a list of Attendee?



incidenceeditor-ng/opencomposerjob.cpp
<https://git.reviewboard.kde.org/r/119462/#comment43912>

    comments in English please :)


- Kevin Krammer


On Juli 25, 2014, 12:32 nachm., Sandro Knauß wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/119462/
> -----------------------------------------------------------
> 
> (Updated Juli 25, 2014, 12:32 nachm.)
> 
> 
> Review request for KDEPIM.
> 
> 
> Repository: kdepim
> 
> 
> Description
> -------
> 
> The last patch of this serie.
> Crating the actual individual mail dialog, to select which attendee should get an update and for whom a composer should be opened.
> 
> 
> Diffs
> -----
> 
>   incidenceeditor-ng/CMakeLists.txt 9097780defc9922424b36cdb7f576a806c8f1fd4 
>   incidenceeditor-ng/editoritemmanager.cpp 7cc3614f6343ead48e899c8ff95caaa2a56dd530 
>   incidenceeditor-ng/individualmaildialog.h PRE-CREATION 
>   incidenceeditor-ng/individualmaildialog.cpp PRE-CREATION 
>   incidenceeditor-ng/individualmailjobfactory.h PRE-CREATION 
>   incidenceeditor-ng/individualmailjobfactory.cpp PRE-CREATION 
>   incidenceeditor-ng/opencomposerjob.h PRE-CREATION 
>   incidenceeditor-ng/opencomposerjob.cpp PRE-CREATION 
>   korganizer/calendarview.cpp df3d74139c2363857adecee308b8a0144183f52d 
> 
> Diff: https://git.reviewboard.kde.org/r/119462/diff/
> 
> 
> Testing
> -------
> 
> Sending much of invatations around :)
> 
> 
> File Attachments
> ----------------
> 
> The new dialog
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/07/25/ac439091-58fa-4d8d-a365-810126ff963b__individual-mail.png
> 
> 
> Thanks,
> 
> Sandro Knauß
> 
>

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