New KPluginSelector. Finished.

[Rafael Fernández López]

Hi all,

Wow David, stunning. Thanks for you 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).

Ok, since I use QAbstractListModel as model, which (watching the code) 
directly ignores parent parameter, I thought that would be fine, but for 
clearness I can write that. No problem with me.

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

Oops, ok. I missed that method.

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

I will add the invalid index check. If it is a valid index, it is impossible 
to don't have a PluginEntry* in the internal pointer, so a null check is 
unneeded, only with a valid index check is enough.

> +    , 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?

No, both are correct. I will make the itemview() be the parent if you prefer 
so, but no explicit reason why I did that.

> +    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.

That's always good. Fixing it.

> +    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?

Yeah, that's a Goya "limitation". Since we want to use widget pooling some day 
and right now is not implemented, at the moment if you have a row with 5 
widgets and the rest (99 rows for example) with no widgets, you will create 5 
* 100 widgets. On the method where you create widgets there is no parameters 
(no QModelIndex), so you have to create, and then on the update method hide 
those widgets you don't want there.

I suggested ervin to add the QModelIndex to the create method of Goya but he 
disagreed because that would make the widget pooling harder on the future.

> +    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
> :-)

I don't get this. QModelIndex's model() method returns a "const 
QAbstractItemModel*". Nothing I can do about it...

> (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.

I will do for security purposes, doesn't harm.

> +    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.

Hmm, yeah.

> +    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.

My fault, it was a struct before. Fixed the repositioning of variables.


Regards,
Rafael Fernández López.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20080516/802b0058/attachment.sig>