[Kde-pim] Review Request: Proposal to speed up first sync

[Jason Kasper]

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

Ship it!


Awesome work, Bertjan! =:) Thanks so much for doing this! If you can address the things I've listed below, I think you should definitely submit this afterwards. =:)


trunk/KDE/kdepim/kpilot/conduits/base/record.h
<http://reviewboard.kde.org/r/776/#comment764>

    Can you add a little more as to what we're using this for? In the future, I can imagine it getting confusing as to why we have summaries, titles, and such, and now an additional description in the base class. I think something like this would be good:
    
    Returns the description of this record. We use this to create a faster lookup for records, which is especially crucial for first sync situations with large databases. While each implementing class will have its own interpretation of what this contains (calendar and todo records will use the summary, contacts will use some form of firstname/lastname, memos will use title, etc.), what is important is that this contain something meaningful to both HH and PC records.



trunk/KDE/kdepim/kpilot/conduits/base/recordconduit.h
<http://reviewboard.kde.org/r/776/#comment765>

    Hm. How is fFirstSyncSet different than fSyncedPcRecords? If it is different, then we should probably have a better description since "ids of already synced pc records" sounds an awful lot like "fSyncedPcRecords". Also, it's not a Hash and "allready" -> "already" =;P



trunk/KDE/kdepim/kpilot/conduits/base/recordconduit.cc
<http://reviewboard.kde.org/r/776/#comment766>

    Hm. Can you take out Doug's stuff and let's crossport that to trunk on its own to keep this clean? Also, I think we'll want to crossport your work (without Doug's stuff) to branches/KDE/4.2, once we're comfortable with it. I don't know how many more releases we're planning on having of the 4.2 branch, but if we're planning on having even one more, it would be good to get this in there. I know we're not planning on having a 4.2.5 release, but I'd feel better just in case. =:)



trunk/KDE/kdepim/kpilot/conduits/base/recordconduit.cc
<http://reviewboard.kde.org/r/776/#comment767>

    Can you please change this to be a DEBUGKPILOT? I don't want this to go anywhere user-visible.  Also, "minutes -> " minutes.



trunk/KDE/kdepim/kpilot/conduits/base/recordconduit.cc
<http://reviewboard.kde.org/r/776/#comment768>

    Awesome!!! =:)


- Jason


On 2009-05-29 02:42:08, Bertjan Broeksema wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/776/
> -----------------------------------------------------------
> 
> (Updated 2009-05-29 02:42:08)
> 
> 
> Review request for KDE PIM, Douglas Harms and Jason Kasper.
> 
> 
> Summary
> -------
> 
> Use a QSet during first sync to reduce the amount of time for looking up
> matches. Each time a match is found the id of the matched pc record is added to
> the set.
> 
> The next time findMatch is called the it will again iterate over all pcrecords
> but if the pcrecord id is in the set allready the equal method will not be called.
> 
> I did not test it yet, this patch is just to show the idea.
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdepim/kpilot/conduits/todo/todohhrecord.h 974238 
>   trunk/KDE/kdepim/kpilot/conduits/todo/todoakonadirecord.h 974238 
>   trunk/KDE/kdepim/kpilot/conduits/todo/todoakonadirecord.cc 974238 
>   trunk/KDE/kdepim/kpilot/conduits/keyringconduit/keyringhhrecord.cc 974238 
>   trunk/KDE/kdepim/kpilot/conduits/keyringconduit/keyringhhrecord.h 974238 
>   trunk/KDE/kdepim/kpilot/conduits/contacts/contactshhrecord.cc 974238 
>   trunk/KDE/kdepim/kpilot/conduits/base/tests/testrecord.cc 974238 
>   trunk/KDE/kdepim/kpilot/conduits/base/tests/testrecordconduit.h 974238 
>   trunk/KDE/kdepim/kpilot/conduits/calendar/calendarakonadirecord.h 974238 
>   trunk/KDE/kdepim/kpilot/conduits/calendar/calendarakonadirecord.cc 974238 
>   trunk/KDE/kdepim/kpilot/conduits/calendar/calendarhhrecord.h 974238 
>   trunk/KDE/kdepim/kpilot/conduits/calendar/calendarhhrecord.cc 974238 
>   trunk/KDE/kdepim/kpilot/conduits/contacts/contactsakonadirecord.h 974238 
>   trunk/KDE/kdepim/kpilot/conduits/contacts/contactsakonadirecord.cc 974238 
>   trunk/KDE/kdepim/kpilot/conduits/contacts/contactshhrecord.h 974238 
>   trunk/KDE/kdepim/kpilot/conduits/base/tests/rcfirstsynctest.cc 974238 
>   trunk/KDE/kdepim/kpilot/conduits/base/tests/testdataproxy.h 974238 
>   trunk/KDE/kdepim/kpilot/conduits/base/tests/testdataproxy.cc 974238 
>   trunk/KDE/kdepim/kpilot/conduits/base/tests/testhhdataproxy.h 974238 
>   trunk/KDE/kdepim/kpilot/conduits/base/tests/testhhdataproxy.cc 974238 
>   trunk/KDE/kdepim/kpilot/conduits/base/tests/testhhrecord.h 974238 
>   trunk/KDE/kdepim/kpilot/conduits/base/tests/testhhrecord.cc 974238 
>   trunk/KDE/kdepim/kpilot/conduits/base/tests/testrecord.h 974238 
>   trunk/KDE/kdepim/kpilot/conduits/base/recordconduit.cc 974238 
>   trunk/KDE/kdepim/kpilot/conduits/base/recordconduit.h 974238 
>   trunk/KDE/kdepim/kpilot/conduits/base/record.h 974238 
>   trunk/KDE/kdepim/kpilot/conduits/base/hhdataproxy.cc 974238 
>   trunk/KDE/kdepim/kpilot/conduits/base/dataproxy.cc 974238 
>   trunk/KDE/kdepim/kpilot/conduits/base/dataproxy.h 974238 
>   trunk/KDE/kdepim/kpilot/conduits/akonadibase/akonadidataproxy.cc 974238 
>   trunk/KDE/kdepim/kpilot/conduits/todo/todohhrecord.cc 974238 
> 
> Diff: http://reviewboard.kde.org/r/776/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Bertjan
> 
>

_______________________________________________
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/