[Kde-pim] Review Request: refactor kmail's recipient editor into a generic widget

Tue Jun 22 14:17:49 BST 2010

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

- I think the current recipient editor should be replaced, instead of keeping it around and having a second "NG" version. Having only one copy will prevent confusion and 
  prevent lost commits when merging recipient editor stuff around. So please drop the NG suffix and remove the original version. Unfortunatley, this copying makes it 
  difficult to see the real changes in this patch, and the history of the old code is lost. Since you use git-svn, please commit with git svn dcommit --find-copies-
  harder, that should help with this problem when comitting, it will correcetly do a "svn cp" for those files that bear resemblence with the original ones, thus keeping 
  history
- Please commit this to the komo branch only for now, and merge after the freeze is over. I think some strings are moved around, which will probably not work
- There are build warnings which should be fixed, like 'KPIM::MultiplyingLineEditor::mMultiplyingLineFactory’ will be initialized after...'
- The tests don't build: messagecomposer/tests/recipientseditortest.cpp:56: undefined reference to `MessageComposer::RecipientsEditor::RecipientsEditor(QWidget*)'
- There are some trailing spaces in the patch, which should be removed

So in the end, I mostly didn't review the .cpp files, as I don't see a proper diff, and don't know which are your changes and which ones are old code. Too much to review.
I tried running it, and that seems to work fine, so all is good.
Bertjan needs to have a look to see if the API matches what he expects in the attendee editor.

/branches/work/komo/kdepim/libkdepim/multiplyingline.h
<http://reviewboard.kde.org/r/4438/#comment5804>

    I don't understand this comment at all. What is ABC? From the docs. I don't understand at all why this class is pure virtual and what the intended subclasses should be.

/branches/work/komo/kdepim/libkdepim/multiplyinglineeditor.h
<http://reviewboard.kde.org/r/4438/#comment5805>

    const?

/branches/work/komo/kdepim/libkdepim/multiplyinglineeditor.h
<http://reviewboard.kde.org/r/4438/#comment5812>

    I know it was like this before, but I don't think it is needed. Changing the font should be done in the system settings, for all line edits, but not by individual applications.

    So I think this should be removed.

/branches/work/komo/kdepim/libkdepim/multiplyinglineeditor.cpp
<http://reviewboard.kde.org/r/4438/#comment5806>

    Yes, please fix it :)

/branches/work/komo/kdepim/libkdepim/multiplyinglineeditor_p.h
<http://reviewboard.kde.org/r/4438/#comment5807>

    Given this class name, I think the file name should be multiplyinglineview_p.cpp, to avoid confusion

/branches/work/komo/kdepim/libkdepim/multiplyinglineeditor_p.h
<http://reviewboard.kde.org/r/4438/#comment5808>

    AFAIK explicit keyword is not needed when there are > 1 parameters

/branches/work/komo/kdepim/libkdepim/multiplyinglineeditor_p.h
<http://reviewboard.kde.org/r/4438/#comment5810>

    "the data you want to add"? Not the data I want to remove?

/branches/work/komo/kdepim/libkdepim/multiplyinglineeditor_p.h
<http://reviewboard.kde.org/r/4438/#comment5811>

    const &

/branches/work/komo/kdepim/libkdepim/multiplyinglineeditor_p.cpp
<http://reviewboard.kde.org/r/4438/#comment5813>

    Put this in the initalizer list

/branches/work/komo/kdepim/libkdepim/multiplyinglineeditor_p.cpp
<http://reviewboard.kde.org/r/4438/#comment5809>

    Put the warning in the MOBILE_UI #ifdef here, so that it is only shown when building with KDEPIM_MOBILE_UI enabled

/branches/work/komo/kdepim/libkdepim/multiplyinglineeditor_p.cpp
<http://reviewboard.kde.org/r/4438/#comment5814>

    Put this in the initalizer list

/branches/work/komo/kdepim/libkdepim/multiplyinglineeditor_p.cpp
<http://reviewboard.kde.org/r/4438/#comment5815>

    foreach( const(?) MultiplyingLine *line, mLines )

/branches/work/komo/kdepim/messagecomposer/recipient-ng.h
<http://reviewboard.kde.org/r/4438/#comment5817>

    This should definitly be in a namespace, to avoid name clashes with generic names like "Recipient"

/branches/work/komo/kdepim/messagecomposer/recipient-ng.h
<http://reviewboard.kde.org/r/4438/#comment5816>

    Is excluding the explicit check here ok? I wonder if implict conversion from QString to Recipient is otherwise possible

/branches/work/komo/kdepim/messagecomposer/recipient-ng.h
<http://reviewboard.kde.org/r/4438/#comment5818>

    No need to implement that here, if it is empty

/branches/work/komo/kdepim/messagecomposer/recipient-ng.h
<http://reviewboard.kde.org/r/4438/#comment5819>

    I see this in multiple places: Personally, I think it would be nicer to specifiy the same argument name as in the .cpp file, instead of leaving it out.

/branches/work/komo/kdepim/messagecomposer/recipientseditorsidewidget.cpp
<http://reviewboard.kde.org/r/4438/#comment5820>

    Use KPushButton instead of QPushButton

- Thomas

On 2010-06-21 21:31:35, casey wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4438/
> -----------------------------------------------------------
> 
> (Updated 2010-06-21 21:31:35)
> 
> 
> Review request for KDE PIM, Bertjan Broeksema and Thomas McGuire.
> 
> 
> Summary
> -------
> 
> This commit refactors the recipients editor in kmail into a generic widget (libkdepim/MultiplyingLineEditor). This new widget can now be used in other similar situations (such as the event attendee editor, which is coming soon TM ).
> 
> I implemented this new editor alongside the original recipients editor, so some renaming of classes might yet be appropriate.
> 
> 
> Diffs
> -----
> 
>   /branches/work/komo/kdepim/kmail/kmcomposewin.h 1139614 
>   /branches/work/komo/kdepim/kmail/kmcomposewin.cpp 1139614 
>   /branches/work/komo/kdepim/libkdepim/CMakeLists.txt 1139614 
>   /branches/work/komo/kdepim/libkdepim/multiplyingline.h PRE-CREATION 
>   /branches/work/komo/kdepim/libkdepim/multiplyingline.cpp PRE-CREATION 
>   /branches/work/komo/kdepim/libkdepim/multiplyinglineeditor.h PRE-CREATION 
>   /branches/work/komo/kdepim/libkdepim/multiplyinglineeditor.cpp PRE-CREATION 
>   /branches/work/komo/kdepim/libkdepim/multiplyinglineeditor_p.h PRE-CREATION 
>   /branches/work/komo/kdepim/libkdepim/multiplyinglineeditor_p.cpp PRE-CREATION 
>   /branches/work/komo/kdepim/messagecomposer/CMakeLists.txt 1139614 
>   /branches/work/komo/kdepim/messagecomposer/composerviewbase.h 1139614 
>   /branches/work/komo/kdepim/messagecomposer/composerviewbase.cpp 1139614 
>   /branches/work/komo/kdepim/messagecomposer/distributionlistdialog.h 1139614 
>   /branches/work/komo/kdepim/messagecomposer/distributionlistdialog.cpp 1139614 
>   /branches/work/komo/kdepim/messagecomposer/recipient-ng.h PRE-CREATION 
>   /branches/work/komo/kdepim/messagecomposer/recipient-ng.cpp PRE-CREATION 
>   /branches/work/komo/kdepim/messagecomposer/recipientline-ng.h PRE-CREATION 
>   /branches/work/komo/kdepim/messagecomposer/recipientline-ng.cpp PRE-CREATION 
>   /branches/work/komo/kdepim/messagecomposer/recipientseditor-ng.h PRE-CREATION 
>   /branches/work/komo/kdepim/messagecomposer/recipientseditor-ng.cpp PRE-CREATION 
>   /branches/work/komo/kdepim/messagecomposer/recipientseditorsidewidget.h PRE-CREATION 
>   /branches/work/komo/kdepim/messagecomposer/recipientseditorsidewidget.cpp PRE-CREATION 
>   /branches/work/komo/kdepim/messagecomposer/recipientspicker.h 1139614 
> 
> Diff: http://reviewboard.kde.org/r/4438/diff
> 
> 
> Testing
> -------
> 
> Opened up kmail outfitted with the new recipient editor and another kmail with the older version side by side, and tested inputs to verify they exhibited same behavior.
> 
> 
> Thanks,
> 
> casey
> 
>

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