Review Request: Changed plugin structure and added convenience methods

Thu Jan 13 20:00:27 CET 2011

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

Certainly looks like a good improvement. Especially how the plugins look after.

There's a few trivial changes below. Watch out for your braces on "if"s and "for" loops.

Also I'm wondering if it's worth getting rid of the parameterValues method in the plugin interface, as the accounts KCM can just fetch it from the model.
It would be bad to let the model and a copy of the list get out of sync.
It should be replaced with a method called "save" or something similar, so plugin developers have a chance to call setData() on the relevant model fields if they are not using your handleParameter() methods.

When you do merge this, could you make sure plugins are merged at the same time. I want to minimise the amount of time when the whole thing doesn't compile.

src/KCMTelepathyAccounts/abstract-account-parameters-widget.cpp
<http://git.reviewboard.kde.org/r/100376/#comment658>

    You'll want to wrap this in "if(labelWidget)"

    otherwise you're going to get crashes later.

src/KCMTelepathyAccounts/abstract-account-parameters-widget.cpp
<http://git.reviewboard.kde.org/r/100376/#comment661>

    I would rather use the indexForParameter method. 

    then 

    if (!index.isValid || model->parameterAt(index).type != parameter.type() {
     //hide widgets
    }
    else {
     //mapping stuff

    }

    Makes this a bit easier to read, and saves you looping through the model data twice.

src/KCMTelepathyAccounts/abstract-account-parameters-widget.cpp
<http://git.reviewboard.kde.org/r/100376/#comment659>

    coding standard

    foreach (..) {

src/KCMTelepathyAccounts/abstract-account-parameters-widget.cpp
<http://git.reviewboard.kde.org/r/100376/#comment660>

    if ( ) {

src/KCMTelepathyAccounts/parameter-edit-model.cpp
<http://git.reviewboard.kde.org/r/100376/#comment671>

    index needs checking that it's valid. Otherwise we crash.

src/KCMTelepathyAccounts/parameter-edit-model.cpp
<http://git.reviewboard.kde.org/r/100376/#comment670>

    I'm not sure I like this.

    firstly it's better not to duplicate code

    case Qt::DisplayRole:
     /*Fall through*/
    case Qt::EditRole:
     //do stuff
     break;

    that way if you have to change one, it's fine.

    I don't like having EditRole, and ValueRole when they both do the same thing. We should just have one.

src/KCMTelepathyAccounts/parameter-edit-model.cpp
<http://git.reviewboard.kde.org/r/100376/#comment662>

    It'd be quicker and easier to just call
    data = QVariant(m_items.at(index.row())->defaultValue());

src/KCMTelepathyAccounts/parameter-edit-model.cpp
<http://git.reviewboard.kde.org/r/100376/#comment663>

    If you implement the above comment you don't need to do this.

src/KCMTelepathyAccounts/parameter-edit-model.cpp
<http://git.reviewboard.kde.org/r/100376/#comment664>

    don't add commented out code. 

src/KCMTelepathyAccounts/parameter-edit-model.cpp
<http://git.reviewboard.kde.org/r/100376/#comment672>

    if (!index.isValid())

src/KCMTelepathyAccounts/parameter-edit-model.cpp
<http://git.reviewboard.kde.org/r/100376/#comment673>

    index.isValid() should stop the need for having this.

src/KCMTelepathyAccounts/parameter-edit-model.cpp
<http://git.reviewboard.kde.org/r/100376/#comment665>

    simply:

    return index(i);

src/KCMTelepathyAccounts/parameter-edit-widget.cpp
<http://git.reviewboard.kde.org/r/100376/#comment669>

    delete it if it's not needed.

src/KCMTelepathyAccounts/parameter-edit-widget.cpp
<http://git.reviewboard.kde.org/r/100376/#comment666>

    you may as well just not implement this method, the AbstractAccountParametersWidget will return the values.

src/KCMTelepathyAccounts/parameter-edit-widget.cpp
<http://git.reviewboard.kde.org/r/100376/#comment667>

    you may as well just not implement this method, the AbstractAccountParametersWidget will return the values.

src/KCMTelepathyAccounts/parameter-item.cpp
<http://git.reviewboard.kde.org/r/100376/#comment668>

    don't commit commented out code.

src/add-account-assistant.cpp
<http://git.reviewboard.kde.org/r/100376/#comment657>

    call your variable parameterModel or something indicating what it is a model of. 

    Right now it's like having a variable called "string" or "number".

- David

On Jan. 13, 2011, 6:05 p.m., Dominik Schmidt wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100376/
> -----------------------------------------------------------
> 
> (Updated Jan. 13, 2011, 6:05 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Summary
> -------
> 
> This got a little huge, I'm sorry.
> 
> This patch refactors the way plugins work.
> It uses (kind of) the methods from the last review request but additionally uses the QDataWidgetMapper as suggested (and partly implemented) by Thomas - please tell me your mail address!
> 
> Also it avoids passing around ParameterLists and Values and instead passes the ParameterEditModel through all levels.
> 
> 
> Diffs
> -----
> 
>   src/KCMTelepathyAccounts/CMakeLists.txt 082f3352c4e5855d87b594620e7e317160b557c1 
>   src/KCMTelepathyAccounts/abstract-account-parameters-widget.h 44bba673ee0479f7253e6c2ac0860888362d0642 
>   src/KCMTelepathyAccounts/abstract-account-parameters-widget.cpp d0ad71a3660ed2514817a67e6fd2a283481b54d9 
>   src/KCMTelepathyAccounts/abstract-account-ui.h a90395ca4e429fa37ccf935f19fcd0063397d4e9 
>   src/KCMTelepathyAccounts/abstract-account-ui.cpp a2a09c0485e2eec5e64c96a91ddaddea2747e68d 
>   src/KCMTelepathyAccounts/account-edit-widget.h 9e8ac689eb15d7666a3be06f98af5a67703efd6c 
>   src/KCMTelepathyAccounts/account-edit-widget.cpp d78cf93fbd9bb905ef2b382e07b0ac780f2f9e3b 
>   src/KCMTelepathyAccounts/generic-advanced-options-widget.h PRE-CREATION 
>   src/KCMTelepathyAccounts/generic-advanced-options-widget.cpp PRE-CREATION 
>   src/KCMTelepathyAccounts/include/GenericAdvancedOptionsWidget PRE-CREATION 
>   src/KCMTelepathyAccounts/include/ParameterEditModel PRE-CREATION 
>   src/KCMTelepathyAccounts/parameter-edit-model.h 4eac5a7512c8e0a1f90f7d570f69d0659b61c2b6 
>   src/KCMTelepathyAccounts/parameter-edit-model.cpp a30481ef12ef11533aab26b3200c94eb50139f49 
>   src/KCMTelepathyAccounts/parameter-edit-widget.h 566cbe1227adb7e0bd7ee4637f1263273f83333b 
>   src/KCMTelepathyAccounts/parameter-edit-widget.cpp 7dd1e530d167a860564200351dea5ae3cd00f9a8 
>   src/KCMTelepathyAccounts/parameter-item.cpp 8f907f7988b10d0e58f5beddf04ebedd54fdfb00 
>   src/add-account-assistant.cpp 184cf93ef8c7b1df944c3fb54cde8f5b6749cec2 
>   src/edit-account-dialog.cpp 45ec8407f943da77175d6f320fe13b92b75874f1 
> 
> Diff: http://git.reviewboard.kde.org/r/100376/diff
> 
> 
> Testing
> -------
> 
> Yes, works for me for adding and editing accounts.
> 
> 
> Thanks,
> 
> Dominik
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-telepathy/attachments/20110113/de5b26e5/attachment-0001.htm 