Review Request 128743: Addressee: Implemented a way to determine if the birthday has been set with a time

[Harald Sitter]

> On Aug. 26, 2016, 2:54 p.m., Friedrich W. H. Kossebau wrote:
> > src/addressee.h, line 290
> > <https://git.reviewboard.kde.org/r/128743/diff/3/?file=475263#file475263line290>
> >
> >     What is the API/ABI policy for KContacts?
> >     This is an ABI change, when it comes to upgrading libKF5AkonadiContact without recompiling all consumers of it.
> >     
> >     Could this perhaps be done with an overload method
> >     `void setBirthday(const QDateTime &birthday, bool withTime);` instead (and a comment to merge both into one on the next ABI change occasion)?
> 
> Daniel Vrátil wrote:
>     KContacts (or any other PIM library for that matter) currently does not have any API/ABI guarantees. We try not to break interfaces unless really necessary and we try to only do those changes between major releases, especially with those "low-tier" libraries.
> 
> Friedrich W. H. Kossebau wrote:
>     The 'KF5' in the lib name suggests some kind of stability though ;) IMHO a lib should only use the name KF5 once it is really part, but well, noone around from the marketing department :)
>     
>     But more important, please note at least `@since WHATEVER VERSION` in the API dox for this new method signature. Your API users will be grateful for that, incl. me :)

So. This breaks an interface libkolab uses, it would be swell if it didn't. Also doing this as a separate method setBirthdayWithTime is loads more expressive IMO.


- Harald


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128743/#review98701
-----------------------------------------------------------


On Aug. 26, 2016, 9:15 a.m., Benni Hill wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128743/
> -----------------------------------------------------------
> 
> (Updated Aug. 26, 2016, 9:15 a.m.)
> 
> 
> Review request for KDEPIM-Libraries and Laurent Montel.
> 
> 
> Repository: kcontacts
> 
> 
> Description
> -------
> 
> - My aim was to implement a way to determine if there has been set a explicit time with the date of birth and that the time is only exported to a vcard if it has been explicitly specified. Some vcard implementations (e.g. on Android) ignore the birthday field if it comes with a time.
> The problem here is that `QDateTime` (when set with a valid date) always has a valid time (midnight by default). I therefore added the boolean `withTime` to `setBirthday()` which is stored inside `Addressee` and can be queried with `birthdayHasTime()`. I also added `setBirthday(QDate)`.
> I changed `VCardTool` to make use of this new methods when importing/exporting vcards.
> Probably `KContacts::Field` and `KContacts::LDIFConverter` also should be adjusted.
> To make use of this change in KAddressbook, (at least?) akonadi-contatcs has to be adjusted as well (in `PersonalEditorWidget::storeContact()`):
> ```
> --- a/src/editor/personaleditor/personaleditorwidget.cpp
> +++ b/src/editor/personaleditor/personaleditorwidget.cpp
> @@ -83,12 +83,7 @@ void PersonalEditorWidget::loadContact(const KContacts::Addressee &contact)
>  void PersonalEditorWidget::storeContact(KContacts::Addressee &contact)
>  {
>      // dates group
> -    QDateTime birthday = QDateTime(mBirthdateWidget->date(), QTime(), contact.birthday().timeSpec());
> -    // This is needed because the constructor above sets the time component
> -    // of the QDateTime to midnight.  We want it to stay invalid.
> -    birthday.setTime(QTime());
> -
> -    contact.setBirthday(birthday);
> +    contact.setBirthday(mBirthdateWidget->date());
>      Akonadi::Utils::storeCustom(contact, QStringLiteral("X-Anniversary"), mAnniversaryWidget->date().toString(Qt::ISODate));
>  
>      // family group
> ```
> 
> - Unrelated to this changes I implemented a way in VCardTool to import/export dates without a year. Dates without a year ('--MMdd') are imported as year=-1 and during export, years <= 0 are written as '--'. (This also has to be implemented in the user inteface some where to make any use.)
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt 0b4786f 
>   autotests/addresseetest.cpp 156d642 
>   autotests/birthdaytest.h 6535c42 
>   autotests/birthdaytest.cpp 4c5313b 
>   autotests/datetimetest.h 329810e 
>   autotests/datetimetest.cpp 8517f00 
>   autotests/ldifconvertertest.h da9220f 
>   autotests/ldifconvertertest.cpp 9d3971e 
>   src/addressee.h 4dc0dfd 
>   src/addressee.cpp 9f4dd2e 
>   src/converter/ldifconverter.cpp d1fee94 
>   src/field.cpp 3406c9d 
>   src/vcardtool.h 48890ed 
>   src/vcardtool.cpp aa5654d 
> 
> Diff: https://git.reviewboard.kde.org/r/128743/diff/
> 
> 
> Testing
> -------
> 
> Added autotests:
> 
> - Exporting birthdays without a time.
> - Importing/Exporting dates without a year.
> 
> 
> Thanks,
> 
> Benni Hill
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-pim/attachments/20160913/ff2134d1/attachment.html>