[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-core-devel
Subject:    Re: New KPluginSelector. Finished.
From:       David Faure <faure () kde ! org>
Date:       2008-05-16 11:15:00
Message-ID: 200805161315.01543.faure () kde ! org
[Download RAW message or body]

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@kde.org, sponsored by Trolltech to work on KDE,
Konqueror (http://www.konqueror.org), and KOffice (http://www.koffice.org).


[prev in list] [next in list] [prev in thread] [next in thread]