Review Request 112438: Merge plugin for SMS CM (pintxo)

[David Edmundson]

> On Sept. 2, 2013, 12:54 a.m., David Edmundson wrote:
> > plugins/pintxo/main-options-widget.cpp, line 40
> > <http://git.reviewboard.kde.org/r/112438/diff/1/?file=186179#file186179line40>
> >
> >     The superclass already has a DataWidgetMapper with a handy method
> >     
> >     handleParamater() to connect up the UI to the model.
> >     
> >     Is there a reason to redo this?
> 
> Anant Kamath wrote:
>     Because handleParameter() takes the currentText property from combo boxes. I needed to send a different property (selectedSimIdentifier) instead of the currentText (the displayed text of the combo box). Hence redid it.

In general, if a library doesn't do what you need it to do - edit the library.
Don't clone it all and re-invent everything, if everyone did that we wouldn't get anywhere and the code would be huge.

You could have just added an extra argument to handleParameter, or even just exposed the datamapper from the superclass.

In this particular case I think if you follow my suggestion of marking that property as USER, you won't have to do this at all.
"The USER attribute indicates whether the property is designated as the user-facing or user-editable property for the class. Normally, there is only one USER property per class (default false). e.g., QAbstractButton::checked is the user editable property for (checkable) buttons. Note that QItemDelegate gets and sets a widget's USER property."

(http://harmattan-dev.nokia.com/docs/library/html/qt4/properties.html)


> On Sept. 2, 2013, 12:54 a.m., David Edmundson wrote:
> > plugins/pintxo/modem-combobox.h, line 36
> > <http://git.reviewboard.kde.org/r/112438/diff/1/?file=186181#file186181line36>
> >
> >     do you not also need a set method?
> >     
> >     Otherwise when you edit an account the second time it will show the wrong value.
> 
> Anant Kamath wrote:
>     As per the current behaviour, if the modem that was chosen (or any modem) while creating the account, is not connected when editing, then the combobox will be blank, and not allow you to save the settings.
>     If there is any modem connected, it will appear in the list, which you can choose as your (new) modem.
>     I'm not sure a set method is required here?

I'm confused.

Let's pretend I have 2 modems.
I create an account and set the modem in the combo box.

Then I edit my account, where is the code to make it show the correct modem in that combo box?


- David


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


On Sept. 2, 2013, 1:22 a.m., Anant Kamath wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112438/
> -----------------------------------------------------------
> 
> (Updated Sept. 2, 2013, 1:22 a.m.)
> 
> 
> Review request for Telepathy and David Edmundson.
> 
> 
> Description
> -------
> 
> Merge plugin for the SMS connection manager (pintxo)
> 
> 
> Diffs
> -----
> 
>   plugins/pintxo/pintxo-account-ui.h PRE-CREATION 
>   plugins/pintxo/pintxo-account-ui-plugin.cpp PRE-CREATION 
>   plugins/pintxo/pintxo-account-ui-plugin.h PRE-CREATION 
>   plugins/pintxo/modem-combobox.h PRE-CREATION 
>   plugins/pintxo/modem-combobox.cpp PRE-CREATION 
>   plugins/pintxo/main-options-widget.ui PRE-CREATION 
>   plugins/pintxo/main-options-widget.cpp PRE-CREATION 
>   plugins/pintxo/main-options-widget.h PRE-CREATION 
>   plugins/pintxo/ktpaccountskcm_plugin_pintxo.desktop.cmake PRE-CREATION 
>   plugins/pintxo/Messages.sh PRE-CREATION 
>   plugins/pintxo/CMakeLists.txt PRE-CREATION 
>   plugins/CMakeLists.txt b2af316891af14232c5aaa3eebcfd17802c6a1ca 
>   data/profiles/sms.profile PRE-CREATION 
>   plugins/pintxo/pintxo-account-ui.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/112438/diff/
> 
> 
> Testing
> -------
> 
> Builds, works
> 
> 
> File Attachments
> ----------------
> 
> Screenshot
>   http://git.reviewboard.kde.org/media/uploaded/files/2013/09/02/am1.jpeg
> 
> 
> Thanks,
> 
> Anant Kamath
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20130902/70f6e12b/attachment-0001.html>