[Kde-pim] Review Request: Merge "Mailing List Management" into "Folder Properties"

[Jonathan Marten]

> On Nov. 16, 2012, 1:20 p.m., Kevin Krammer wrote:
> > kmail/kmmainwidget.cpp, line 1638
> > <http://git.reviewboard.kde.org/r/107341/diff/1/?file=94875#file94875line1638>
> >
> >     The other slot only checks mCurrentFolder. do we need the mFolderTreeWidget check for some  specific reason?

Not sure why this check is needed, but it is unchanged from the original.


> On Nov. 16, 2012, 1:20 p.m., Kevin Krammer wrote:
> > kmail/kmmainwidget.h, line 266
> > <http://git.reviewboard.kde.org/r/107341/diff/1/?file=94874#file94874line266>
> >
> >     while this will work, why not rename the function to showCollectionProperties and make the slot call that function with QString(). 
> >     
> >     Another option would be to connect both actions to the same slot and store the page to show as user data on the action. The slot could then retrieve that independent of which action called it

Good suggestion to rename and add an additional slot, it avoids the confusion (even though it works properly) with a signal(bool) connected to formerly a slot(void) but now a slot (QString) with a default argument.  Will update diff.

Not sure that using action's user data is a good idea, the slot would have to use QObject::caller() (which is inadvisable) and the page selection is scattered around the source file instead if it being all together in slotFolderMailingListProperties().


> On Nov. 16, 2012, 1:20 p.m., Kevin Krammer wrote:
> > kmail/kmmainwidget.cpp, line 4646
> > <http://git.reviewboard.kde.org/r/107341/diff/1/?file=94875#file94875line4646>
> >
> >     you could just add it to the job as a property, like it is already done for collectionId

Have prototyped this, but there is a UI inconsistency if the resource is offline:  in this case there is no job created and slotCollectionPropertiesContinued() is called directly with NULL.  So there is no job to retrieve the property from and the dialogue opens at the first page instead of the intended one.  Better to keep the member variable.


- Jonathan


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


On Nov. 16, 2012, 12:20 p.m., Jonathan Marten wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107341/
> -----------------------------------------------------------
> 
> (Updated Nov. 16, 2012, 12:20 p.m.)
> 
> 
> Review request for KDEPIM.
> 
> 
> Description
> -------
> 
> This change merges the separate "Mailing List Management" dialogue into a tab of "Folder Properties", and also corrects its layout.
> 
> In order to maintain the same user experience, the actions and menu structure remain unchanged.  The "Mailing List Management" option in the "Folder" menu opens the "Folder Properties" dialogue at the appropriate tab (this requires the change to kdepimlibs submitted at https://git.reviewboard.kde.org/r/106623/).
> 
> The file that managed the dialogue (now the tab) is mostly unchanged, just renamed and with the corresponding class name changed.  Reviewboard doesn't detect that, hence the big diff.
> 
> In kmmainwidget, the signature of an existing function is changed.  This is not BC, but I'm assuming that this is not a problem in application code.  There is a BC-compatible way (but not SC) that this could be done if necessary.
> 
> 
> This addresses bug 115611.
>     http://bugs.kde.org/show_bug.cgi?id=115611
> 
> 
> Diffs
> -----
> 
>   kmail/CMakeLists.txt 588d63a 
>   kmail/collectionmailinglistpage.h PRE-CREATION 
>   kmail/collectionmailinglistpage.cpp PRE-CREATION 
>   kmail/kmmainwidget.h 390d232 
>   kmail/kmmainwidget.cpp a3668d5 
>   kmail/mailinglistpropertiesdialog.h 949566d 
>   kmail/mailinglistpropertiesdialog.cpp b5b2998 
> 
> Diff: http://git.reviewboard.kde.org/r/107341/diff/
> 
> 
> Testing
> -------
> 
> Built kdepim with this change, checked operation of dialogue and mailing list functions.
> 
> 
> Thanks,
> 
> Jonathan Marten
> 
>

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