[Kde-pim] Re: Review Request: Add CollectionProperties method to select default pages to display

[David Jarvie]

> On 2010-12-15 21:53:45, Tobias Koenig wrote:
> > Hej David,
> > 
> > I reverted your patch because it breaks the system of using custom pages in the properties dialog.
> > Every properties page should have an unique QObject::objectName set (e.g. it's full qualified class name), so has the CollectionGeneralPage and the CachePolicyPage.
> > If no names are passed to the CollectionPropertiesDialog constructor, all registered default pages will be displayed, otherwise the pages with the given names.
> > This has the advantage that the caller of the CollectionPropertiesDialog has the full control of the pages that shall be displayed and their order. An additional option to enable/disable the default pages is just confusing and makes the API more difficult to use.
> > I havn't been aware of your reviewboard patch until you commited it, therefor no comments from me before. Sorry for the confusion.
> > 
> > Ciao,
> > Tobias

I see that you added the new constructor after this reviewboard was created, hence the confusion.

There is an advantage in the way you implemented it in that the page order can be specified. The one issue I have is that the object names for the default pages don't seem to be discoverable except by reading the source code. There needs to be a way to fetch the names, both for reasons of discoverability, and to ensure that applications don't break if the names changed. I'm not sure which class should provide access to the names - perhaps CollectionGeneralPropertiesPage and CachePolicyPage should have public static methods to return their object names. In addition, since the default pages are defined in CollectionPropertiesDialog, I think that CollectionPropertiesDialog should have a method which returns the object name of a specified default page type, as in the following code:

class CollectionPropertiesDialog
{
   ...
enum DefaultPage
{
    GeneralPage,
    CachePage
};
static QString defaultPageObjectName(DefaultPage page)
{
    switch (page)
    {
        case GeneralPage:  return CollectionGeneralPropertiesPage::objectName();
    }
    return QString();
}

   ...
};


- David


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


On 2010-11-16 00:03:17, David Jarvie wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/5855/
> -----------------------------------------------------------
> 
> (Updated 2010-11-16 00:03:17)
> 
> 
> Review request for KDE PIM.
> 
> 
> Summary
> -------
> 
> Provide more flexible options for displaying an Akonadi collection properties dialog. Use of an enum allows an application to select which default page(s) to show. The existing useDefaultPage() is deprecated in favour of the new useDefaultPages() method.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdepimlibs/akonadi/collectionpropertiesdialog.h 1196227 
>   /trunk/KDE/kdepimlibs/akonadi/collectionpropertiesdialog.cpp 1196227 
> 
> Diff: http://svn.reviewboard.kde.org/r/5855/diff
> 
> 
> Testing
> -------
> 
> Works in KAlarm.
> 
> 
> Thanks,
> 
> David
> 
>

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