Review Request: Maemo fixes in Kcal

[Alvaro Manera]

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

Review request for KDE PIM.


Summary
-------

This is a combined patch of all the changes we have in our trunk, before the implementation of the RecurrenceID.

These changes include:
* Contacts in the incidence. (I don't know if you need this, as you have the addressbook) ¿?
* Some small fixes in our side (like copy constructor in listbase).
* Handling the updates. This fixes the bug that you end up having a duplicate entry in the Hash table when modifying an event.


Diffs
-----

  trunk/KDE/kdepimlibs/kcal/alarm.cpp 1016210
  trunk/KDE/kdepimlibs/kcal/duration.cpp 1016210
  trunk/KDE/kdepimlibs/kcal/event.cpp 1016210
  trunk/KDE/kdepimlibs/kcal/incidence.cpp 1016210
  trunk/KDE/kdepimlibs/kcal/incidencebase.cpp 1016210
  trunk/KDE/kdepimlibs/kcal/journal.cpp 1016210
  trunk/KDE/kdepimlibs/kcal/listbase.h 1016210
  trunk/KDE/kdepimlibs/kcal/todo.cpp 1016210

Diff: http://reviewboard.kde.org/r/1489/diff


Testing
-------


Thanks,

Alvaro

_______________________________________________
KDE PIM mailing list kde-pim@...
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/

[Allen Winter]

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


> These changes include:
> * Contacts in the incidence. (I don't know if you need this, as you have the addressbook) ¿?

No need. But I also would rather not have a fork at Nokia, so I'm ok adding contacts.
But
1) would Persons be better than simple QStrings for the contacts?
2) where is the associated incidencebase.h patch?

> * Handling the updates. This fixes the bug that you end up having a duplicate entry in the Hash table when modifying an event.
My fault.  Csaba submitted this a few months ago.

I'll try to get the non-contacts patches committed ASAP.

- Allen


On 2009-08-31 09:00:09, Alvaro Manera wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1489/
> -----------------------------------------------------------
>
> (Updated 2009-08-31 09:00:09)
>
>
> Review request for KDE PIM.
>
>
> Summary
> -------
>
> This is a combined patch of all the changes we have in our trunk, before the implementation of the RecurrenceID.
>
> These changes include:
> * Contacts in the incidence. (I don't know if you need this, as you have the addressbook) ¿?
> * Some small fixes in our side (like copy constructor in listbase).
> * Handling the updates. This fixes the bug that you end up having a duplicate entry in the Hash table when modifying an event.
>
>
> Diffs
> -----
>
>   trunk/KDE/kdepimlibs/kcal/alarm.cpp 1016210
>   trunk/KDE/kdepimlibs/kcal/duration.cpp 1016210
>   trunk/KDE/kdepimlibs/kcal/event.cpp 1016210
>   trunk/KDE/kdepimlibs/kcal/incidence.cpp 1016210
>   trunk/KDE/kdepimlibs/kcal/incidencebase.cpp 1016210
>   trunk/KDE/kdepimlibs/kcal/journal.cpp 1016210
>   trunk/KDE/kdepimlibs/kcal/listbase.h 1016210
>   trunk/KDE/kdepimlibs/kcal/todo.cpp 1016210
>
> Diff: http://reviewboard.kde.org/r/1489/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alvaro
>
>

_______________________________________________
KDE PIM mailing list kde-pim@...
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/

[Allen Winter]

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


please submit a new patch that includes incidencebase.h.  The current patch does not compile.

- Allen


On 2009-08-31 09:00:09, Alvaro Manera wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1489/
> -----------------------------------------------------------
>
> (Updated 2009-08-31 09:00:09)
>
>
> Review request for KDE PIM.
>
>
> Summary
> -------
>
> This is a combined patch of all the changes we have in our trunk, before the implementation of the RecurrenceID.
>
> These changes include:
> * Contacts in the incidence. (I don't know if you need this, as you have the addressbook) ¿?
> * Some small fixes in our side (like copy constructor in listbase).
> * Handling the updates. This fixes the bug that you end up having a duplicate entry in the Hash table when modifying an event.
>
>
> Diffs
> -----
>
>   trunk/KDE/kdepimlibs/kcal/alarm.cpp 1016210
>   trunk/KDE/kdepimlibs/kcal/duration.cpp 1016210
>   trunk/KDE/kdepimlibs/kcal/event.cpp 1016210
>   trunk/KDE/kdepimlibs/kcal/incidence.cpp 1016210
>   trunk/KDE/kdepimlibs/kcal/incidencebase.cpp 1016210
>   trunk/KDE/kdepimlibs/kcal/journal.cpp 1016210
>   trunk/KDE/kdepimlibs/kcal/listbase.h 1016210
>   trunk/KDE/kdepimlibs/kcal/todo.cpp 1016210
>
> Diff: http://reviewboard.kde.org/r/1489/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alvaro
>
>

_______________________________________________
KDE PIM mailing list kde-pim@...
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/

[Alvaro Manera]

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

(Updated 2009-09-02 13:07:15.412634)


Review request for KDE PIM.


Changes
-------

The missing incidencebase.h

Sorry!


Summary
-------

This is a combined patch of all the changes we have in our trunk, before the implementation of the RecurrenceID.

These changes include:
* Contacts in the incidence. (I don't know if you need this, as you have the addressbook) ¿?
* Some small fixes in our side (like copy constructor in listbase).
* Handling the updates. This fixes the bug that you end up having a duplicate entry in the Hash table when modifying an event.


Diffs (updated)
-----

  trunk/KDE/kdepimlibs/kcal/incidencebase.h 1006190

Diff: http://reviewboard.kde.org/r/1489/diff


Testing
-------


Thanks,

Alvaro

_______________________________________________
KDE PIM mailing list kde-pim@...
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/

[Allen Winter]

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


Doesn't compile.  there must be more missing bits to the patch.

/data/kde/trunk/KDE/kdepimlibs/kcal/icalformat.cpp: In member function 'KCal::Incidence* KCal::ICalFormat::fromString(const QString&)':                                                      
/data/kde/trunk/KDE/kdepimlibs/kcal/icalformat.cpp:207: error: cannot declare variable 'cal' to be of abstract type 'KCal::CalendarLocal'                                                    
/data/kde/trunk/KDE/kdepimlibs/kcal/calendarlocal.h:44: note:   because the following virtual functions are pure within 'KCal::CalendarLocal':                                                
/data/kde/trunk/KDE/kdepimlibs/kcal/incidencebase.h:173: note:  virtual void KCal::IncidenceBase::IncidenceObserver::incidenceUpdate(KCal::IncidenceBase*)                                    
/data/kde/trunk/KDE/kdepimlibs/kcal/icalformat.cpp: In member function 'QString KCal::ICalFormat::toICalString(KCal::Incidence*)':                                                            
/data/kde/trunk/KDE/kdepimlibs/kcal/icalformat.cpp:292: error: cannot declare variable 'cal' to be of abstract type 'KCal::CalendarLocal'                                                    
/data/kde/trunk/KDE/kdepimlibs/kcal/calendarlocal.h:44: note:   since type 'KCal::CalendarLocal' has pure virtual functions                                                                  
/

- Allen


On 2009-09-02 13:07:15, Alvaro Manera wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1489/
> -----------------------------------------------------------
>
> (Updated 2009-09-02 13:07:15)
>
>
> Review request for KDE PIM.
>
>
> Summary
> -------
>
> This is a combined patch of all the changes we have in our trunk, before the implementation of the RecurrenceID.
>
> These changes include:
> * Contacts in the incidence. (I don't know if you need this, as you have the addressbook) ¿?
> * Some small fixes in our side (like copy constructor in listbase).
> * Handling the updates. This fixes the bug that you end up having a duplicate entry in the Hash table when modifying an event.
>
>
> Diffs
> -----
>
>   trunk/KDE/kdepimlibs/kcal/incidencebase.h 1006190
>
> Diff: http://reviewboard.kde.org/r/1489/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alvaro
>
>

_______________________________________________
KDE PIM mailing list kde-pim@...
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/

[David Jarvie-4]

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


Only the patch for incidencebase.h is included - the rest of your patch is missing.


trunk/KDE/kdepimlibs/kcal/incidencebase.h
<http://reviewboard.kde.org/r/1489/#comment1547>

    This should be "@param incidenceBase a pointer to an IncidenceBase object." (Omit "is".)
    The reason is that when the API documentation is produced, the parameter names are listed in italics, with their descriptions following. Have a look at the apidox to see how this appears.
   
    The same comment applies to the other method descriptions also.



trunk/KDE/kdepimlibs/kcal/incidencebase.h
<http://reviewboard.kde.org/r/1489/#comment1548>

    Remove "is"



trunk/KDE/kdepimlibs/kcal/incidencebase.h
<http://reviewboard.kde.org/r/1489/#comment1549>

    Remove "is"


- David


On 2009-09-02 13:07:15, Alvaro Manera wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1489/
> -----------------------------------------------------------
>
> (Updated 2009-09-02 13:07:15)
>
>
> Review request for KDE PIM.
>
>
> Summary
> -------
>
> This is a combined patch of all the changes we have in our trunk, before the implementation of the RecurrenceID.
>
> These changes include:
> * Contacts in the incidence. (I don't know if you need this, as you have the addressbook) ¿?
> * Some small fixes in our side (like copy constructor in listbase).
> * Handling the updates. This fixes the bug that you end up having a duplicate entry in the Hash table when modifying an event.
>
>
> Diffs
> -----
>
>   trunk/KDE/kdepimlibs/kcal/incidencebase.h 1006190
>
> Diff: http://reviewboard.kde.org/r/1489/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alvaro
>
>

_______________________________________________
KDE PIM mailing list kde-pim@...
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/

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


>>  Handling the updates. This fixes the bug that you end up having a duplicate entry in the Hash table when modifying an event.

We probably can close http://reviewboard.kde.org/r/495/ which also fixes this problem.

- Sergio


On 2009-09-02 13:07:15, Alvaro Manera wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1489/
> -----------------------------------------------------------
>
> (Updated 2009-09-02 13:07:15)
>
>
> Review request for KDE PIM.
>
>
> Summary
> -------
>
> This is a combined patch of all the changes we have in our trunk, before the implementation of the RecurrenceID.
>
> These changes include:
> * Contacts in the incidence. (I don't know if you need this, as you have the addressbook) ¿?
> * Some small fixes in our side (like copy constructor in listbase).
> * Handling the updates. This fixes the bug that you end up having a duplicate entry in the Hash table when modifying an event.
>
>
> Diffs
> -----
>
>   trunk/KDE/kdepimlibs/kcal/incidencebase.h 1006190
>
> Diff: http://reviewboard.kde.org/r/1489/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alvaro
>
>

_______________________________________________
KDE PIM mailing list kde-pim@...
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/

[Alvaro Manera]

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

(Updated 2009-10-14 06:57:11.291776)


Review request for KDE PIM.


Changes
-------

This is the one. This time it compiles without problems. All the files needed patched.


Summary
-------

This is a combined patch of all the changes we have in our trunk, before the implementation of the RecurrenceID.

These changes include:
* Contacts in the incidence. (I don't know if you need this, as you have the addressbook) ¿?
* Some small fixes in our side (like copy constructor in listbase).
* Handling the updates. This fixes the bug that you end up having a duplicate entry in the Hash table when modifying an event.


Diffs (updated)
-----

  trunk/KDE/kdepimlibs/kcal/alarm.cpp 1034333
  trunk/KDE/kdepimlibs/kcal/calendarlocal.h 1034333
  trunk/KDE/kdepimlibs/kcal/calendarlocal.cpp 1034333
  trunk/KDE/kdepimlibs/kcal/calendarnull.h 1034333
  trunk/KDE/kdepimlibs/kcal/calendarnull.cpp 1034333
  trunk/KDE/kdepimlibs/kcal/duration.cpp 1034333
  trunk/KDE/kdepimlibs/kcal/event.cpp 1034333
  trunk/KDE/kdepimlibs/kcal/incidence.cpp 1034333
  trunk/KDE/kdepimlibs/kcal/incidencebase.h 1034333
  trunk/KDE/kdepimlibs/kcal/incidencebase.cpp 1034333
  trunk/KDE/kdepimlibs/kcal/journal.cpp 1034333
  trunk/KDE/kdepimlibs/kcal/listbase.h 1034333
  trunk/KDE/kdepimlibs/kcal/todo.cpp 1034333

Diff: http://reviewboard.kde.org/r/1489/diff


Testing
-------


Thanks,

Alvaro

_______________________________________________
KDE PIM mailing list kde-pim@...
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/

[David Jarvie-4]

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



trunk/KDE/kdepimlibs/kcal/duration.cpp
<http://reviewboard.kde.org/r/1489/#comment1959>

    The durations are _not_ strictly equal if one is in terms of seconds and the other in terms of days. When a day crosses a daylight saving time change, the two produce different results. So this change should not be implemented.



trunk/KDE/kdepimlibs/kcal/incidencebase.h
<http://reviewboard.kde.org/r/1489/#comment1960>

    A clearer wording would be:
   
    Call this to notify the observers when the IncidenceBase object is about to be changed. Call updated() after the changes have been completed.


- David


On 2009-10-14 06:57:11, Alvaro Manera wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1489/
> -----------------------------------------------------------
>
> (Updated 2009-10-14 06:57:11)
>
>
> Review request for KDE PIM.
>
>
> Summary
> -------
>
> This is a combined patch of all the changes we have in our trunk, before the implementation of the RecurrenceID.
>
> These changes include:
> * Contacts in the incidence. (I don't know if you need this, as you have the addressbook) ¿?
> * Some small fixes in our side (like copy constructor in listbase).
> * Handling the updates. This fixes the bug that you end up having a duplicate entry in the Hash table when modifying an event.
>
>
> Diffs
> -----
>
>   trunk/KDE/kdepimlibs/kcal/alarm.cpp 1034333
>   trunk/KDE/kdepimlibs/kcal/calendarlocal.h 1034333
>   trunk/KDE/kdepimlibs/kcal/calendarlocal.cpp 1034333
>   trunk/KDE/kdepimlibs/kcal/calendarnull.h 1034333
>   trunk/KDE/kdepimlibs/kcal/calendarnull.cpp 1034333
>   trunk/KDE/kdepimlibs/kcal/duration.cpp 1034333
>   trunk/KDE/kdepimlibs/kcal/event.cpp 1034333
>   trunk/KDE/kdepimlibs/kcal/incidence.cpp 1034333
>   trunk/KDE/kdepimlibs/kcal/incidencebase.h 1034333
>   trunk/KDE/kdepimlibs/kcal/incidencebase.cpp 1034333
>   trunk/KDE/kdepimlibs/kcal/journal.cpp 1034333
>   trunk/KDE/kdepimlibs/kcal/listbase.h 1034333
>   trunk/KDE/kdepimlibs/kcal/todo.cpp 1034333
>
> Diff: http://reviewboard.kde.org/r/1489/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alvaro
>
>

_______________________________________________
KDE PIM mailing list kde-pim@...
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/

[Alvaro Manera]

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

(Updated 2009-10-15 06:49:46.104898)


Review request for KDE PIM.


Changes
-------

David is right. The duration fix is removed.


Summary
-------

This is a combined patch of all the changes we have in our trunk, before the implementation of the RecurrenceID.

These changes include:
* Contacts in the incidence. (I don't know if you need this, as you have the addressbook) ¿?
* Some small fixes in our side (like copy constructor in listbase).
* Handling the updates. This fixes the bug that you end up having a duplicate entry in the Hash table when modifying an event.


Diffs (updated)
-----

  trunk/KDE/kdepimlibs/kcal/alarm.cpp 1034333
  trunk/KDE/kdepimlibs/kcal/calendarlocal.h 1034333
  trunk/KDE/kdepimlibs/kcal/calendarlocal.cpp 1034333
  trunk/KDE/kdepimlibs/kcal/calendarnull.h 1034333
  trunk/KDE/kdepimlibs/kcal/calendarnull.cpp 1034333
  trunk/KDE/kdepimlibs/kcal/event.cpp 1034333
  trunk/KDE/kdepimlibs/kcal/incidence.cpp 1034333
  trunk/KDE/kdepimlibs/kcal/incidencebase.h 1034333
  trunk/KDE/kdepimlibs/kcal/incidencebase.cpp 1034333
  trunk/KDE/kdepimlibs/kcal/journal.cpp 1034333
  trunk/KDE/kdepimlibs/kcal/listbase.h 1034333
  trunk/KDE/kdepimlibs/kcal/todo.cpp 1034333

Diff: http://reviewboard.kde.org/r/1489/diff


Testing
-------


Thanks,

Alvaro

_______________________________________________
KDE PIM mailing list kde-pim@...
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/

[Alvaro Manera]

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

(Updated 2009-10-15 06:56:18.031463)


Review request for KDE PIM.


Changes
-------

Forgot to correct the "is" from David's comments. And I also removed the other ones in other functions.


Summary
-------

This is a combined patch of all the changes we have in our trunk, before the implementation of the RecurrenceID.

These changes include:
* Contacts in the incidence. (I don't know if you need this, as you have the addressbook) ¿?
* Some small fixes in our side (like copy constructor in listbase).
* Handling the updates. This fixes the bug that you end up having a duplicate entry in the Hash table when modifying an event.


Diffs (updated)
-----

  trunk/KDE/kdepimlibs/kcal/alarm.cpp 1034333
  trunk/KDE/kdepimlibs/kcal/calendarlocal.h 1034333
  trunk/KDE/kdepimlibs/kcal/calendarlocal.cpp 1034333
  trunk/KDE/kdepimlibs/kcal/calendarnull.h 1034333
  trunk/KDE/kdepimlibs/kcal/calendarnull.cpp 1034333
  trunk/KDE/kdepimlibs/kcal/event.cpp 1034333
  trunk/KDE/kdepimlibs/kcal/incidence.cpp 1034333
  trunk/KDE/kdepimlibs/kcal/incidencebase.h 1034333
  trunk/KDE/kdepimlibs/kcal/incidencebase.cpp 1034333
  trunk/KDE/kdepimlibs/kcal/journal.cpp 1034333
  trunk/KDE/kdepimlibs/kcal/listbase.h 1034333
  trunk/KDE/kdepimlibs/kcal/todo.cpp 1034333

Diff: http://reviewboard.kde.org/r/1489/diff


Testing
-------


Thanks,

Alvaro

_______________________________________________
KDE PIM mailing list kde-pim@...
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/

[David Jarvie-4]

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



trunk/KDE/kdepimlibs/kcal/incidencebase.h
<http://reviewboard.kde.org/r/1489/#comment1977>

    Don't use the word "after" since this could suggest after the changes, which is potentially misleading. I suggest:
   
    "before the IncidenceBase object is changed."
   
    or
   
    "when the IncidenceBase object is about to be changed."


- David


On 2009-10-15 06:56:18, Alvaro Manera wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1489/
> -----------------------------------------------------------
>
> (Updated 2009-10-15 06:56:18)
>
>
> Review request for KDE PIM.
>
>
> Summary
> -------
>
> This is a combined patch of all the changes we have in our trunk, before the implementation of the RecurrenceID.
>
> These changes include:
> * Contacts in the incidence. (I don't know if you need this, as you have the addressbook) ¿?
> * Some small fixes in our side (like copy constructor in listbase).
> * Handling the updates. This fixes the bug that you end up having a duplicate entry in the Hash table when modifying an event.
>
>
> Diffs
> -----
>
>   trunk/KDE/kdepimlibs/kcal/alarm.cpp 1034333
>   trunk/KDE/kdepimlibs/kcal/calendarlocal.h 1034333
>   trunk/KDE/kdepimlibs/kcal/calendarlocal.cpp 1034333
>   trunk/KDE/kdepimlibs/kcal/calendarnull.h 1034333
>   trunk/KDE/kdepimlibs/kcal/calendarnull.cpp 1034333
>   trunk/KDE/kdepimlibs/kcal/event.cpp 1034333
>   trunk/KDE/kdepimlibs/kcal/incidence.cpp 1034333
>   trunk/KDE/kdepimlibs/kcal/incidencebase.h 1034333
>   trunk/KDE/kdepimlibs/kcal/incidencebase.cpp 1034333
>   trunk/KDE/kdepimlibs/kcal/journal.cpp 1034333
>   trunk/KDE/kdepimlibs/kcal/listbase.h 1034333
>   trunk/KDE/kdepimlibs/kcal/todo.cpp 1034333
>
> Diff: http://reviewboard.kde.org/r/1489/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alvaro
>
>

_______________________________________________
KDE PIM mailing list kde-pim@...
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/

[Alvaro Manera]

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

(Updated 2009-10-15 11:06:55.465219)


Review request for KDE PIM.


Changes
-------

Fix in the comments


Summary
-------

This is a combined patch of all the changes we have in our trunk, before the implementation of the RecurrenceID.

These changes include:
* Contacts in the incidence. (I don't know if you need this, as you have the addressbook) ¿?
* Some small fixes in our side (like copy constructor in listbase).
* Handling the updates. This fixes the bug that you end up having a duplicate entry in the Hash table when modifying an event.


Diffs (updated)
-----

  trunk/KDE/kdepimlibs/kcal/alarm.cpp 1035467
  trunk/KDE/kdepimlibs/kcal/calendarlocal.h 1035467
  trunk/KDE/kdepimlibs/kcal/calendarlocal.cpp 1035467
  trunk/KDE/kdepimlibs/kcal/calendarnull.h 1035467
  trunk/KDE/kdepimlibs/kcal/calendarnull.cpp 1035467
  trunk/KDE/kdepimlibs/kcal/event.cpp 1035467
  trunk/KDE/kdepimlibs/kcal/incidence.cpp 1035467
  trunk/KDE/kdepimlibs/kcal/incidencebase.h 1035467
  trunk/KDE/kdepimlibs/kcal/incidencebase.cpp 1035467
  trunk/KDE/kdepimlibs/kcal/journal.cpp 1035467
  trunk/KDE/kdepimlibs/kcal/listbase.h 1035467
  trunk/KDE/kdepimlibs/kcal/todo.cpp 1035467

Diff: http://reviewboard.kde.org/r/1489/diff


Testing
-------


Thanks,

Alvaro

_______________________________________________
KDE PIM mailing list kde-pim@...
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/

[Allen Winter]

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


there are some minor problems (like forgetting the @since 4.4 apidox tags)
which I fixed locally.

there is a major problem of binary incompatibility by adding the incidenceUpdate() pure virtual.
Not quite sure how to deal with
the other questions is: would you be willing to use Person objects with the new Contacts methods instead of QStrings?

- Allen


On 2009-10-15 11:06:55, Alvaro Manera wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1489/
> -----------------------------------------------------------
>
> (Updated 2009-10-15 11:06:55)
>
>
> Review request for KDE PIM.
>
>
> Summary
> -------
>
> This is a combined patch of all the changes we have in our trunk, before the implementation of the RecurrenceID.
>
> These changes include:
> * Contacts in the incidence. (I don't know if you need this, as you have the addressbook) ¿?
> * Some small fixes in our side (like copy constructor in listbase).
> * Handling the updates. This fixes the bug that you end up having a duplicate entry in the Hash table when modifying an event.
>
>
> Diffs
> -----
>
>   trunk/KDE/kdepimlibs/kcal/alarm.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/calendarlocal.h 1035467
>   trunk/KDE/kdepimlibs/kcal/calendarlocal.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/calendarnull.h 1035467
>   trunk/KDE/kdepimlibs/kcal/calendarnull.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/event.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/incidence.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/incidencebase.h 1035467
>   trunk/KDE/kdepimlibs/kcal/incidencebase.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/journal.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/listbase.h 1035467
>   trunk/KDE/kdepimlibs/kcal/todo.cpp 1035467
>
> Diff: http://reviewboard.kde.org/r/1489/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alvaro
>
>

_______________________________________________
KDE PIM mailing list kde-pim@...
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/

[Alvaro Manera]

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

(Updated 2009-10-21 14:48:51.218395)


Review request for KDE PIM.


Changes
-------

New version of the diff. I forgot to add the changes in icalformat_p.cpp
To answer also to Allen: It is not so easy to use the Person to store the Contact. By the RFC the Contact can be a lot of things, and not necessary a Name / Email. We are storing it to be able to export it afterwards (round trip compatibility). And that's why I realized that I forgot to include the icalformat_p diff.


Summary
-------

This is a combined patch of all the changes we have in our trunk, before the implementation of the RecurrenceID.

These changes include:
* Contacts in the incidence. (I don't know if you need this, as you have the addressbook) ¿?
* Some small fixes in our side (like copy constructor in listbase).
* Handling the updates. This fixes the bug that you end up having a duplicate entry in the Hash table when modifying an event.


Diffs (updated)
-----

  trunk/KDE/kdepimlibs/kcal/alarm.cpp 1035467
  trunk/KDE/kdepimlibs/kcal/calendarlocal.h 1035467
  trunk/KDE/kdepimlibs/kcal/calendarlocal.cpp 1035467
  trunk/KDE/kdepimlibs/kcal/calendarnull.h 1035467
  trunk/KDE/kdepimlibs/kcal/calendarnull.cpp 1035467
  trunk/KDE/kdepimlibs/kcal/event.cpp 1035467
  trunk/KDE/kdepimlibs/kcal/icalformat_p.cpp 1035467
  trunk/KDE/kdepimlibs/kcal/incidence.cpp 1035467
  trunk/KDE/kdepimlibs/kcal/incidencebase.h 1035467
  trunk/KDE/kdepimlibs/kcal/incidencebase.cpp 1035467
  trunk/KDE/kdepimlibs/kcal/journal.cpp 1035467
  trunk/KDE/kdepimlibs/kcal/listbase.h 1035467
  trunk/KDE/kdepimlibs/kcal/todo.cpp 1035467

Diff: http://reviewboard.kde.org/r/1489/diff


Testing
-------


Thanks,

Alvaro

_______________________________________________
KDE PIM mailing list kde-pim@...
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/

[Alvaro Manera]

> On 2009-10-15 14:45:03, Allen Winter wrote:
> > there are some minor problems (like forgetting the @since 4.4 apidox tags)
> > which I fixed locally.
> >
> > there is a major problem of binary incompatibility by adding the incidenceUpdate() pure virtual.
> > Not quite sure how to deal with
> > the other questions is: would you be willing to use Person objects with the new Contacts methods instead of QStrings?

And what is your proposal to get rid of the binary incompatibility? Because I don't want to wait to kde5 to get it in. :)


- Alvaro


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


On 2009-10-21 14:48:51, Alvaro Manera wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1489/
> -----------------------------------------------------------
>
> (Updated 2009-10-21 14:48:51)
>
>
> Review request for KDE PIM.
>
>
> Summary
> -------
>
> This is a combined patch of all the changes we have in our trunk, before the implementation of the RecurrenceID.
>
> These changes include:
> * Contacts in the incidence. (I don't know if you need this, as you have the addressbook) ¿?
> * Some small fixes in our side (like copy constructor in listbase).
> * Handling the updates. This fixes the bug that you end up having a duplicate entry in the Hash table when modifying an event.
>
>
> Diffs
> -----
>
>   trunk/KDE/kdepimlibs/kcal/alarm.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/calendarlocal.h 1035467
>   trunk/KDE/kdepimlibs/kcal/calendarlocal.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/calendarnull.h 1035467
>   trunk/KDE/kdepimlibs/kcal/calendarnull.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/event.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/icalformat_p.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/incidence.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/incidencebase.h 1035467
>   trunk/KDE/kdepimlibs/kcal/incidencebase.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/journal.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/listbase.h 1035467
>   trunk/KDE/kdepimlibs/kcal/todo.cpp 1035467
>
> Diff: http://reviewboard.kde.org/r/1489/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alvaro
>
>

_______________________________________________
KDE PIM mailing list kde-pim@...
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/

[Kevin Krammer]

> On 2009-10-15 14:45:03, Allen Winter wrote:
> > there are some minor problems (like forgetting the @since 4.4 apidox tags)
> > which I fixed locally.
> >
> > there is a major problem of binary incompatibility by adding the incidenceUpdate() pure virtual.
> > Not quite sure how to deal with
> > the other questions is: would you be willing to use Person objects with the new Contacts methods instead of QStrings?
>
> Alvaro Manera wrote:
>     And what is your proposal to get rid of the binary incompatibility? Because I don't want to wait to kde5 to get it in. :)

One option is to create an IncidenceObserverV2 which inherits IncidenceObserver and has this additional method.
Then either registerObserver() or update() need to decide which of one of them is the base for the observer at hand.


- Kevin


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


On 2009-10-21 14:48:51, Alvaro Manera wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1489/
> -----------------------------------------------------------
>
> (Updated 2009-10-21 14:48:51)
>
>
> Review request for KDE PIM.
>
>
> Summary
> -------
>
> This is a combined patch of all the changes we have in our trunk, before the implementation of the RecurrenceID.
>
> These changes include:
> * Contacts in the incidence. (I don't know if you need this, as you have the addressbook) ¿?
> * Some small fixes in our side (like copy constructor in listbase).
> * Handling the updates. This fixes the bug that you end up having a duplicate entry in the Hash table when modifying an event.
>
>
> Diffs
> -----
>
>   trunk/KDE/kdepimlibs/kcal/alarm.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/calendarlocal.h 1035467
>   trunk/KDE/kdepimlibs/kcal/calendarlocal.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/calendarnull.h 1035467
>   trunk/KDE/kdepimlibs/kcal/calendarnull.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/event.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/icalformat_p.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/incidence.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/incidencebase.h 1035467
>   trunk/KDE/kdepimlibs/kcal/incidencebase.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/journal.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/listbase.h 1035467
>   trunk/KDE/kdepimlibs/kcal/todo.cpp 1035467
>
> Diff: http://reviewboard.kde.org/r/1489/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alvaro
>
>

_______________________________________________
KDE PIM mailing list kde-pim@...
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/

[Allen Winter]

> On 2009-10-15 14:45:03, Allen Winter wrote:
> > there are some minor problems (like forgetting the @since 4.4 apidox tags)
> > which I fixed locally.
> >
> > there is a major problem of binary incompatibility by adding the incidenceUpdate() pure virtual.
> > Not quite sure how to deal with
> > the other questions is: would you be willing to use Person objects with the new Contacts methods instead of QStrings?
>
> Alvaro Manera wrote:
>     And what is your proposal to get rid of the binary incompatibility? Because I don't want to wait to kde5 to get it in. :)
>
> Kevin Krammer wrote:
>     One option is to create an IncidenceObserverV2 which inherits IncidenceObserver and has this additional method.
>     Then either registerObserver() or update() need to decide which of one of them is the base for the observer at hand.
>

Alvaro, I think we should trying using signals and slots as described in http://techbase.kde.org/Policies/Binary_Compatibility_Issues_With_C++#Adding_new_virtual_functions_to_leaf_classes "Using signals instead of virtual functions".  

Would you want to investigate doing that?

Else, we can commit Contacts stuff now only and also we need to deal with the recurrenceID stuff.


- Allen


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


On 2009-10-21 14:48:51, Alvaro Manera wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1489/
> -----------------------------------------------------------
>
> (Updated 2009-10-21 14:48:51)
>
>
> Review request for KDE PIM.
>
>
> Summary
> -------
>
> This is a combined patch of all the changes we have in our trunk, before the implementation of the RecurrenceID.
>
> These changes include:
> * Contacts in the incidence. (I don't know if you need this, as you have the addressbook) ¿?
> * Some small fixes in our side (like copy constructor in listbase).
> * Handling the updates. This fixes the bug that you end up having a duplicate entry in the Hash table when modifying an event.
>
>
> Diffs
> -----
>
>   trunk/KDE/kdepimlibs/kcal/alarm.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/calendarlocal.h 1035467
>   trunk/KDE/kdepimlibs/kcal/calendarlocal.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/calendarnull.h 1035467
>   trunk/KDE/kdepimlibs/kcal/calendarnull.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/event.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/icalformat_p.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/incidence.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/incidencebase.h 1035467
>   trunk/KDE/kdepimlibs/kcal/incidencebase.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/journal.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/listbase.h 1035467
>   trunk/KDE/kdepimlibs/kcal/todo.cpp 1035467
>
> Diff: http://reviewboard.kde.org/r/1489/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alvaro
>
>

_______________________________________________
KDE PIM mailing list kde-pim@...
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/

[Alvaro Manera]

> On 2009-10-15 14:45:03, Allen Winter wrote:
> > there are some minor problems (like forgetting the @since 4.4 apidox tags)
> > which I fixed locally.
> >
> > there is a major problem of binary incompatibility by adding the incidenceUpdate() pure virtual.
> > Not quite sure how to deal with
> > the other questions is: would you be willing to use Person objects with the new Contacts methods instead of QStrings?
>
> Alvaro Manera wrote:
>     And what is your proposal to get rid of the binary incompatibility? Because I don't want to wait to kde5 to get it in. :)
>
> Kevin Krammer wrote:
>     One option is to create an IncidenceObserverV2 which inherits IncidenceObserver and has this additional method.
>     Then either registerObserver() or update() need to decide which of one of them is the base for the observer at hand.
>
>
> Allen Winter wrote:
>     Alvaro, I think we should trying using signals and slots as described in http://techbase.kde.org/Policies/Binary_Compatibility_Issues_With_C++#Adding_new_virtual_functions_to_leaf_classes "Using signals instead of virtual functions".  
>    
>     Would you want to investigate doing that?
>    
>     Else, we can commit Contacts stuff now only and also we need to deal with the recurrenceID stuff.

We cannot follow the signal slot approach. The new virtual is in the IncidenceObserver class. We need to add the inheritance to QObject. And even if we could, Calendar class inherits from it (and also from QObject). We will need to change the inheritance order again.
This way we cannot fix the ABI break.
The other option is to create the V2 class. And add the additional method. But this is valid for me, KDE will have the fix in code, but no possibility of use it. Because to do it, the inheritance change in Calendar will be needed.
Any other ideas?


- Alvaro


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


On 2009-10-21 14:48:51, Alvaro Manera wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1489/
> -----------------------------------------------------------
>
> (Updated 2009-10-21 14:48:51)
>
>
> Review request for KDE PIM.
>
>
> Summary
> -------
>
> This is a combined patch of all the changes we have in our trunk, before the implementation of the RecurrenceID.
>
> These changes include:
> * Contacts in the incidence. (I don't know if you need this, as you have the addressbook) ¿?
> * Some small fixes in our side (like copy constructor in listbase).
> * Handling the updates. This fixes the bug that you end up having a duplicate entry in the Hash table when modifying an event.
>
>
> Diffs
> -----
>
>   trunk/KDE/kdepimlibs/kcal/alarm.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/calendarlocal.h 1035467
>   trunk/KDE/kdepimlibs/kcal/calendarlocal.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/calendarnull.h 1035467
>   trunk/KDE/kdepimlibs/kcal/calendarnull.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/event.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/icalformat_p.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/incidence.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/incidencebase.h 1035467
>   trunk/KDE/kdepimlibs/kcal/incidencebase.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/journal.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/listbase.h 1035467
>   trunk/KDE/kdepimlibs/kcal/todo.cpp 1035467
>
> Diff: http://reviewboard.kde.org/r/1489/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alvaro
>
>

_______________________________________________
KDE PIM mailing list kde-pim@...
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/

[Kevin Krammer]

> On 2009-10-15 14:45:03, Allen Winter wrote:
> > there are some minor problems (like forgetting the @since 4.4 apidox tags)
> > which I fixed locally.
> >
> > there is a major problem of binary incompatibility by adding the incidenceUpdate() pure virtual.
> > Not quite sure how to deal with
> > the other questions is: would you be willing to use Person objects with the new Contacts methods instead of QStrings?
>
> Alvaro Manera wrote:
>     And what is your proposal to get rid of the binary incompatibility? Because I don't want to wait to kde5 to get it in. :)
>
> Kevin Krammer wrote:
>     One option is to create an IncidenceObserverV2 which inherits IncidenceObserver and has this additional method.
>     Then either registerObserver() or update() need to decide which of one of them is the base for the observer at hand.
>
>
> Allen Winter wrote:
>     Alvaro, I think we should trying using signals and slots as described in http://techbase.kde.org/Policies/Binary_Compatibility_Issues_With_C++#Adding_new_virtual_functions_to_leaf_classes "Using signals instead of virtual functions".  
>    
>     Would you want to investigate doing that?
>    
>     Else, we can commit Contacts stuff now only and also we need to deal with the recurrenceID stuff.
>
> Alvaro Manera wrote:
>     We cannot follow the signal slot approach. The new virtual is in the IncidenceObserver class. We need to add the inheritance to QObject. And even if we could, Calendar class inherits from it (and also from QObject). We will need to change the inheritance order again.
>     This way we cannot fix the ABI break.
>     The other option is to create the V2 class. And add the additional method. But this is valid for me, KDE will have the fix in code, but no possibility of use it. Because to do it, the inheritance change in Calendar will be needed.
>     Any other ideas?

The private can inherit the new interface. it is not an exported class


- Kevin


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


On 2009-10-21 14:48:51, Alvaro Manera wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1489/
> -----------------------------------------------------------
>
> (Updated 2009-10-21 14:48:51)
>
>
> Review request for KDE PIM.
>
>
> Summary
> -------
>
> This is a combined patch of all the changes we have in our trunk, before the implementation of the RecurrenceID.
>
> These changes include:
> * Contacts in the incidence. (I don't know if you need this, as you have the addressbook) ¿?
> * Some small fixes in our side (like copy constructor in listbase).
> * Handling the updates. This fixes the bug that you end up having a duplicate entry in the Hash table when modifying an event.
>
>
> Diffs
> -----
>
>   trunk/KDE/kdepimlibs/kcal/alarm.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/calendarlocal.h 1035467
>   trunk/KDE/kdepimlibs/kcal/calendarlocal.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/calendarnull.h 1035467
>   trunk/KDE/kdepimlibs/kcal/calendarnull.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/event.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/icalformat_p.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/incidence.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/incidencebase.h 1035467
>   trunk/KDE/kdepimlibs/kcal/incidencebase.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/journal.cpp 1035467
>   trunk/KDE/kdepimlibs/kcal/listbase.h 1035467
>   trunk/KDE/kdepimlibs/kcal/todo.cpp 1035467
>
> Diff: http://reviewboard.kde.org/r/1489/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alvaro
>
>

_______________________________________________
KDE PIM mailing list kde-pim@...
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/