[Kde-pim] Review Request 121639: Split out KF5Contacts dependency
Aleix Pol Gonzalez
aleixpol at kde.org
Mon Dec 29 15:08:57 GMT 2014
> On Dec. 29, 2014, 2:26 p.m., David Edmundson wrote:
> > src/metacontact.cpp, line 68
> > <https://git.reviewboard.kde.org/r/121639/diff/1/?file=335596#file335596line68>
> >
> > deliberately public?
Eh well... it's the private implementation. they are usually public, no?
> On Dec. 29, 2014, 2:26 p.m., David Edmundson wrote:
> > src/persondata.cpp, line 156
> > <https://git.reviewboard.kde.org/r/121639/diff/1/?file=335599#file335599line156>
> >
> > We're converting a list of variants to a string?
> >
> > I'm not entirely sure what the behaviour will be, but I doubt it's what we want.
Why do you say it's a list?
> On Dec. 29, 2014, 2:26 p.m., David Edmundson wrote:
> > autotests/fakecontactsource.cpp, line 57
> > <https://git.reviewboard.kde.org/r/121639/diff/1/?file=335573#file335573line57>
> >
> > currently this is unused, which means we can't be testing where we have multiple properties
> >
> > also I don't really understand what this line would be doing, turning a variant into a string and then into a stringlist and then into a variant?
I guess I'll have to document this better.
The idea is that if you request "all-something" you are requesting all the alternatives of "something" and requesting "something" returns just the preferred value by any metric.
I guess this might not be a very intuitive API, maybe we'd prefer to have an extra argument in customProperty specifying the cardinality? something like enum Amount { Preferred, All } and then we know that All is a list of the type in Preferred.
> On Dec. 29, 2014, 2:26 p.m., David Edmundson wrote:
> > src/defaultcontactmonitor.cpp, line 38
> > <https://git.reviewboard.kde.org/r/121639/diff/1/?file=335589#file335589line38>
> >
> > given we now control AbstractContact we can put the ID in that.
> >
> > Will simplify a few APIs
So you propose it should be a property? or?
> On Dec. 29, 2014, 2:26 p.m., David Edmundson wrote:
> > src/metacontact.cpp, line 52
> > <https://git.reviewboard.kde.org/r/121639/diff/1/?file=335596#file335596line52>
> >
> > merging behaviour isn't this simple.
> > It change on the property, especially for cardinality.
> >
> >
> > if I request a name I don't want to get a list, and it's especially important as we can use the redesign to not bother querying for data we're not going to show.
> >
> >
> > It also makes the usage more complex where clients now have to start handling lists, and grouping all emails (now a list of lists) together.
See the first comment in here, note the "all-" part of the if().
- Aleix
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121639/#review72493
-----------------------------------------------------------
On Dec. 24, 2014, 1:07 a.m., Aleix Pol Gonzalez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121639/
> -----------------------------------------------------------
>
> (Updated Dec. 24, 2014, 1:07 a.m.)
>
>
> Review request for KDEPIM, Telepathy, Christian Mollekopf, David Edmundson, and Martin Klapetek.
>
>
> Repository: libkpeople
>
>
> Description
> -------
>
> Last week we had a couple of discussions regarding KPeople relationship with KDE PIM and KContacts and we agreed that we want to keep KContacts as a framework that is good at dealing with vCards, which leaves KPeople without a good way to send around its data.
>
> This patch proposes an internal AbstractContact class that the backends will have to re-implement. At the moment it only requires a "customProperty" that lets us fetch whatever is needed.
> Furthermore, it also includes a set of convenience methods in PersonData to have a type-safe API.
>
>
> Diffs
> -----
>
> CMakeLists.txt 3df5f0b
> autotests/CMakeLists.txt 676f170
> autotests/fakecontactsource.h 981fe03
> autotests/fakecontactsource.cpp 269f94f
> autotests/persondatatests.cpp 47a6213
> examples/CMakeLists.txt 74bd188
> examples/duplicates.cpp 72ff53b
> src/CMakeLists.txt 534066b
> src/backends/abstractcontact.h PRE-CREATION
> src/backends/abstractcontact.cpp PRE-CREATION
> src/backends/abstractpersonaction.h 703063e
> src/backends/abstractpersonaction.cpp be93d90
> src/backends/allcontactsmonitor.h 47e5dfd
> src/backends/allcontactsmonitor.cpp e517f17
> src/backends/basepersonsdatasource.h 3c25e28
> src/backends/basepersonsdatasource.cpp b794917
> src/backends/contactmonitor.h cf25953
> src/backends/contactmonitor.cpp ceb5d74
> src/declarative/personactionsmodel.cpp 4b34e15
> src/defaultcontactmonitor.cpp 36c31a7
> src/defaultcontactmonitor_p.h 8fc2812
> src/duplicatesfinder.cpp c6a1014
> src/global.h 9540623
> src/global.cpp 3ef834b
> src/match.cpp 3a93aba
> src/match_p.h 5727270
> src/metacontact.cpp bba6b65
> src/metacontact_p.h 0d738cc
> src/persondata.h 66b19a3
> src/persondata.cpp dab9f61
> src/personpluginmanager.cpp f249b5f
> src/personsmodel.h db907d4
> src/personsmodel.cpp ca6e972
> src/plugins/akonadi/akonadidatasource.cpp ae4ace1
> src/widgets/abstractfieldwidgetfactory.h 1d2d8aa
> src/widgets/persondetailsdialog.cpp 8ca0a99
> src/widgets/persondetailsview.cpp b7535bd
> src/widgets/plugins/emaildetailswidget.h 72a96ee
> src/widgets/plugins/emaildetailswidget.cpp 839c5f6
> src/widgets/plugins/emails.h ede1686
> src/widgets/plugins/emails.cpp d69101b
>
> Diff: https://git.reviewboard.kde.org/r/121639/diff/
>
>
> Testing
> -------
>
> Builds, the test passes. ktp tests work, somewhat.
> Also there's few tests altogether.
>
>
> Thanks,
>
> Aleix Pol Gonzalez
>
>
_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/
More information about the kde-pim
mailing list