> 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-Telepathy mailing list [email protected] https://mail.kde.org/mailman/listinfo/kde-telepathy
