[Kde-pim] Review Request: refactor kmail's recipient editor into a generic widget
unnamedrambler at gmail.com
unnamedrambler at gmail.com
Tue Jun 22 17:02:04 BST 2010
> On 2010-06-22 13:18:00, Thomas McGuire wrote:
> > - 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.
The reason there isn't a proper diff, and why it is difficult to tell what is old code, is because the original recipients editor was not generic in anyway. The recipients specific code was tightly coupled with the multiplying line code. This commit de-couples the two.
Now, all the recipient specific stuff (counting of recipients, ensuring at least one To: field, etc) is contained in messagecomposer/recipientseditor*-ng, and everything related to creating and displaying a multiplying line widget is in libkdepim.
> On 2010-06-22 13:18:00, Thomas McGuire wrote:
> > /branches/work/komo/kdepim/libkdepim/multiplyingline.h, line 58
> > <http://reviewboard.kde.org/r/4438/diff/1/?file=29577#file29577line58>
> >
> > 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.
ABC, abstract base class, sorry. The MultiplyingLineEditor Widget is composed of MultiplyingLines, which in turn contain the custom input fields. The lines are added/removed as the user types.
For example, the Recipients editor' line contains a combobox and lineedit.
> On 2010-06-22 13:18:00, Thomas McGuire wrote:
> > /branches/work/komo/kdepim/libkdepim/multiplyinglineeditor.h, line 96
> > <http://reviewboard.kde.org/r/4438/diff/1/?file=29579#file29579line96>
> >
> > const?
ah yes, good idea.
> On 2010-06-22 13:18:00, Thomas McGuire wrote:
> > /branches/work/komo/kdepim/libkdepim/multiplyinglineeditor.h, line 129
> > <http://reviewboard.kde.org/r/4438/diff/1/?file=29579#file29579line129>
> >
> > 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.
sure thing
> On 2010-06-22 13:18:00, Thomas McGuire wrote:
> > /branches/work/komo/kdepim/libkdepim/multiplyinglineeditor_p.h, line 38
> > <http://reviewboard.kde.org/r/4438/diff/1/?file=29581#file29581line38>
> >
> > Given this class name, I think the file name should be multiplyinglineview_p.cpp, to avoid confusion
will do
> On 2010-06-22 13:18:00, Thomas McGuire wrote:
> > /branches/work/komo/kdepim/libkdepim/multiplyinglineeditor_p.h, line 42
> > <http://reviewboard.kde.org/r/4438/diff/1/?file=29581#file29581line42>
> >
> > AFAIK explicit keyword is not needed when there are > 1 parameters
you're correct, don't know how that slipped in there.
> On 2010-06-22 13:18:00, Thomas McGuire wrote:
> > /branches/work/komo/kdepim/messagecomposer/recipient-ng.h, line 45
> > <http://reviewboard.kde.org/r/4438/diff/1/?file=29588#file29588line45>
> >
> > Is excluding the explicit check here ok? I wonder if implict conversion from QString to Recipient is otherwise possible
Well, do we want to allow something like this?
Recipient r = "john at foobar.com"
> On 2010-06-22 13:18:00, Thomas McGuire wrote:
> > /branches/work/komo/kdepim/messagecomposer/recipientseditorsidewidget.cpp, line 57
> > <http://reviewboard.kde.org/r/4438/diff/1/?file=29595#file29595line57>
> >
> > Use KPushButton instead of QPushButton
done
> On 2010-06-22 13:18:00, Thomas McGuire wrote:
> > /branches/work/komo/kdepim/messagecomposer/recipient-ng.h, line 48
> > <http://reviewboard.kde.org/r/4438/diff/1/?file=29588#file29588line48>
> >
> > 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.
yea, that style was left over from whoever originally implemented the recipients editor. I don't like it either, fixed.
> On 2010-06-22 13:18:00, Thomas McGuire wrote:
> > /branches/work/komo/kdepim/messagecomposer/recipient-ng.h, line 46
> > <http://reviewboard.kde.org/r/4438/diff/1/?file=29588#file29588line46>
> >
> > No need to implement that here, if it is empty
removed.
> On 2010-06-22 13:18:00, Thomas McGuire wrote:
> > /branches/work/komo/kdepim/messagecomposer/recipient-ng.h, line 34
> > <http://reviewboard.kde.org/r/4438/diff/1/?file=29588#file29588line34>
> >
> > This should definitly be in a namespace, to avoid name clashes with generic names like "Recipient"
fixed
> On 2010-06-22 13:18:00, Thomas McGuire wrote:
> > /branches/work/komo/kdepim/libkdepim/multiplyinglineeditor_p.cpp, line 70
> > <http://reviewboard.kde.org/r/4438/diff/1/?file=29582#file29582line70>
> >
> > foreach( const(?) MultiplyingLine *line, mLines )
fixed, but const not added, because the caller that receives the empty line is probably going to change the line in some way.
> On 2010-06-22 13:18:00, Thomas McGuire wrote:
> > /branches/work/komo/kdepim/libkdepim/multiplyinglineeditor_p.cpp, line 51
> > <http://reviewboard.kde.org/r/4438/diff/1/?file=29582#file29582line51>
> >
> > Put this in the initalizer list
done
> On 2010-06-22 13:18:00, Thomas McGuire wrote:
> > /branches/work/komo/kdepim/libkdepim/multiplyinglineeditor_p.cpp, line 45
> > <http://reviewboard.kde.org/r/4438/diff/1/?file=29582#file29582line45>
> >
> > Put the warning in the MOBILE_UI #ifdef here, so that it is only shown when building with KDEPIM_MOBILE_UI enabled
this whole #if block was actually removed, because the ability to set the font was removed.
> On 2010-06-22 13:18:00, Thomas McGuire wrote:
> > /branches/work/komo/kdepim/libkdepim/multiplyinglineeditor_p.cpp, line 42
> > <http://reviewboard.kde.org/r/4438/diff/1/?file=29582#file29582line42>
> >
> > Put this in the initalizer list
done
> On 2010-06-22 13:18:00, Thomas McGuire wrote:
> > /branches/work/komo/kdepim/libkdepim/multiplyinglineeditor_p.h, line 57
> > <http://reviewboard.kde.org/r/4438/diff/1/?file=29581#file29581line57>
> >
> > const &
done, and was fixed in other places as well.
> On 2010-06-22 13:18:00, Thomas McGuire wrote:
> > /branches/work/komo/kdepim/libkdepim/multiplyinglineeditor_p.h, line 55
> > <http://reviewboard.kde.org/r/4438/diff/1/?file=29581#file29581line55>
> >
> > "the data you want to add"? Not the data I want to remove?
typo, fixed.
> On 2010-06-22 13:18:00, Thomas McGuire wrote:
> > /branches/work/komo/kdepim/libkdepim/multiplyinglineeditor.cpp, line 49
> > <http://reviewboard.kde.org/r/4438/diff/1/?file=29580#file29580line49>
> >
> > Yes, please fix it :)
Ah, this was left over from the beginning of the refactor. The side widget is implemented in messagecomposer/ as it is specific to the recipients editor. So, I removed this entire commented out block.
- casey
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/4438/#review6231
-----------------------------------------------------------
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/
More information about the kde-pim
mailing list