New KPluginSelector. Finished.

[David Faure]

On Thursday 15 May 2008, Rafael =?utf-8?q?Fern=C3=A1ndez_L=C3=B3pez?= wrote:
> The patch is at http://media.ereslibre.es/2008/05/kdelibs-kpluginselector.diff. 

Yay, more code for me to review :-)

Here are some (rather minor) implementation comments:

+int KPluginSelector::Private::PluginModel::rowCount(const QModelIndex &parent) const
 {
+    Q_UNUSED(parent)
+    return pluginEntryList.count();
 }
Better write something like
    if (parent.isValid()) return 0; else return pluginEntryList.count();
to ensure that your model doesn't look like a tree (if the view asks for rowCount(childIndex), you want to return 0, not >0).

+        case PluginInfoRole: {
+            QVariant var;
+            var.setValue(pluginEntry->pluginInfo);
+            return var;
=> return QVariant::fromValue(pluginEntry->pluginInfo);

setData() does static_cast<PluginEntry*>(index.internalPointer())-> without an invalid-index check and/or a null check.

+    , checkBox(new QCheckBox)
+    , pushButton(new KPushButton)
Any reason why you're not passing this as parent object instead of the explicit delete in the destructor?

+    KPushButton *aboutPushButton = static_cast<KPushButton*>(widgets[2]);
+    aboutPushButton->resize(aboutPushButton->sizeHint());
+    aboutPushButton->move(option.rect.width() - MARGIN - aboutPushButton->sizeHint().width(), option.rect.height() / 2 - widgets[2]->sizeHint().height() / 2);
This calls widgets[2] twice, and sizeHint() on it three times. sizeHint can do quite a lot of font metrics and calculations, I suggest using a temporary var for holding the sizeHint() value. Same thing with the other two buttons of course.

+    configurePushButton->setVisible(index.model()->data(index, ServicesCountRole).toBool());
Hmm, should it really be created and then hidden, instead of not being created at all if this role says false?

+    const QModelIndex index = focusedIndex();
+    pluginSelector_d->dependenciesWidget->clearDependencies();
+    KPluginInfo pluginInfo = index.model()->data(index, PluginInfoRole).value<KPluginInfo>();
+    pluginSelector_d->updateDependencies(pluginInfo, state);
+    const_cast<QAbstractItemModel*>(index.model())->setData(index, state, Qt::CheckStateRole);
You can avoid the const_cast if you don't make the index const. Const temps are good, but not if they introduce a const_cast :-)

(qobject_cast<KCModuleProxy*>(mainWidget))->moduleInfo()
Should this cast be checked? I'm not 100% sure from reading the code, that in this code path, mainWidget will always be a KCModuleProxy.
Another solution would be a KCModuleProxy* temp variable.

+    KDialog *configDialog = new KDialog(itemView());
+        if (configDialog->exec() == QDialog::Accepted) {
[...]
+    delete configDialog;
Create the modal dialog on the stack instead? Safer in case there's an early return in the code one day.

+    struct PluginEntry; [...]
+class KPluginSelector::Private::PluginEntry
The struct/class mismatch will give a warning on windows. Choose one or the other.
I also suggest grouping the two bools next to each other, to save 4 bytes of padding.

-- 
David Faure, faure at kde.org, sponsored by Trolltech to work on KDE,
Konqueror (http://www.konqueror.org), and KOffice (http://www.koffice.org).