Review Request 111308: Disallow changing URI of PersonData after creation

[Martin Klapetek]

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

Ship it!


Watch out for personitem.cpp, looks like you have my patch applied.


src/persondata.cpp
<http://git.reviewboard.kde.org/r/111308/#comment25899>

    & uri -> &uri



src/persondata.cpp
<http://git.reviewboard.kde.org/r/111308/#comment25900>

    & contacItd -> &contactId



src/persondata.cpp
<http://git.reviewboard.kde.org/r/111308/#comment25901>

    I know it's the original code, but I think this could also use "if (d->uri == uri) { return; }



src/widgets/plugins/mergecontactswidget.cpp
<http://git.reviewboard.kde.org/r/111308/#comment25902>

    This shouldn't be part of this patch (it's fixed in my branch)


- Martin Klapetek


On June 30, 2013, 10:33 p.m., David Edmundson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111308/
> -----------------------------------------------------------
> 
> (Updated June 30, 2013, 10:33 p.m.)
> 
> 
> Review request for Telepathy, Aleix Pol Gonzalez and Vishesh Handa.
> 
> 
> Description
> -------
> 
> Disallow changing URI of PersonData after creation
> 
> Currently setUri and setContactId are public which would cause a lot of bugs if changed.
> Instead replace with two clear static methods; loadFromUri and loadFromContactId.
> 
> In practice they were never used more than once.
> 
> A declarative wrapper using QDeclarativeParserStatus provides the same functionality
> in QML. A wrapper is needed anyway as it means we can now put more complex data types in PersonData.
> 
> 
> bugs that used to exist which are no longer relevant:
>  - every single plugin for PersonsViewer does not respond to uriChanged
>  - personsactionsmodel did not respond to uriChanged()
>  - contactId would return the last user set with setContactId not the current contacts ID (which is misleading/wrong anyway, as a person can have multiple IDs)
>  - isPerson property was not updated when changed
>  - PeronDetailsView kept an invalid empty PersonData on construction with a lifespan of the parent.. for no reason.
> 
> Given we want people to write plugins, we need to make it easier.
> PersonData code is also simpler as it doesn't have to be careful about wiping old resources in setUri.
> 
> Downside is you can't have a PersonData on the stack. Bit of a effort for the unit tests.. but not in the real world I don't think.
> 
> I believe someone (vishesh?) also suggested this at the Pineda sprint. 
> 
> 
> Diffs
> -----
> 
>   src/abstractpersonplugin.h 60508b693fd6406b77f4622bff4c4419fb13c50e 
>   src/abstractpersonplugin.cpp 3a41f3bcff668994de70d77c77fd5a4a061dadde 
>   src/autotests/tests/persondatatests.cpp 623fb5f752cca6d98aa261a72a34b740d74b92e8 
>   src/declarative/CMakeLists.txt 9b33b85429c65ce832f37edecebf2ecdf467c476 
>   src/declarative/declarativepersondata.h PRE-CREATION 
>   src/declarative/declarativepersondata.cpp PRE-CREATION 
>   src/declarative/peopleqmlplugin.cpp c991064e011f5115683adb90955c0e3a5dbd156c 
>   src/examples/personwidget.cpp bc2eaf2805891a201fbbf8cb20420764652e34c7 
>   src/personactionsmodel.cpp 458006213442bcd24bb08216594aed1deb7fb171 
>   src/persondata.h 177f2451c5c432d3cb6add454716bd292540bb7f 
>   src/persondata.cpp 2341bd4f4215bcc2a671f75a118c488e870c98ac 
>   src/personitem.h 91fcd3cc6558625622d17b02805cc4f04382c51d 
>   src/personitem.cpp 6086e41afbd028d830c6a3771dad9fd69d3627cc 
>   src/personpluginmanager.h 38f13816485e69a4a2acab45b73c978d6448be1a 
>   src/personpluginmanager.cpp 487f1bbde1cf122b4c475d8a33bc7514bc9bcc43 
>   src/personsmodel.h 4b6a829d5c3b62e7f057fd15c821460b1e8c5df3 
>   src/personsmodel.cpp 813d0afd6add7d8c33a9d08729ce9c5acadc421f 
>   src/plugins/core/emailplugin.h ebffe6451b0a8cd74d934227e1f771643acab402 
>   src/plugins/core/emailplugin.cpp 83babd4aa71627d6172794a6878a15b0738ad15e 
>   src/resourcewatcherservice.h 94210ef7ac754eebd324c3d55c391378e1dc07d9 
>   src/resourcewatcherservice.cpp 1342c99a597a3203c17aeb187ec2740af1e0ed00 
>   src/widgets/persondetailsview.cpp f655eb2ec0a16f00239b679263354632a6183fee 
>   src/widgets/plugins/mergecontactswidget.cpp 51e11dc8d2457297336616e6ee211493b0f5fa08 
> 
> Diff: http://git.reviewboard.kde.org/r/111308/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20130701/0a23640f/attachment.html>