[Kde-pim] Questionable change to KCal::Period class

[David Jarvie]

I'm repeating this posting with a different subject line, and top-posting, to 
elicit some response.

The commit below alters the kcal Period class in what I think is an 
undesirable way. It adds summary and location members to the class, which I 
don't think belong there. What is the relevance of these members to a class 
which encapsulates a time period? Their values come from custom properties in 
iCalendar, and AFAICS only one user of the Period class (FreeBusy) will ever 
use them. Instead of changing the Period class, FreeBusy should store this 
data itself, perhaps by using a little class something along the lines of

class FBPeriod : public Period
{
  public:
    QString mSummary;
    QString mLocation;
};

It may be necessary to juggle things or add new methods to keep FreeBusy's API 
BC, but I think this is preferable to polluting the Period class for 
non-FreeBusy users. It's a bit reminiscent of the Pilot-special stuff which 
was once in icalformatimpl, which was eventually moved to where it belongs, 
in KPilot itself.


On Thu 21 February 2008 16:57:26 Kevin Ottens wrote:
> SVN commit 777804 by ervin:
>
> Prokde35-s Item 61: Extended free/busy list support
> (kdepimlibs part)
>
> Merged revisions 744087-744110 via svnmerge from
> svn+ssh://ervin@svn.kde.org/home/kde/branches/kdepim/enterprise/kdepim
>
> ................
>   r744087 | vkrause | 2007-12-02 18:08:00 +0100 (dim, 02 déc 2007) | 2
> lines
>
>   Support for reading extended free/busy lists.
> ................
>   r744092 | vkrause | 2007-12-02 18:19:34 +0100 (dim, 02 déc 2007) | 2
> lines
>
>   Show extended free/busy information.
> ................
>   r744110 | vkrause | 2007-12-02 18:47:07 +0100 (dim, 02 déc 2007) | 9
> lines
>
>   Merged revisions 689908 via svnmerge from
>   https://vkrause@svn.kde.org/home/kde/branches/KDE/3.5/kdepim
>
>   ........
>     r689908 | bvirlet | 2007-07-19 15:23:54 +0200 (Thu, 19 Jul 2007) | 3
> lines
>
>     Remove these signal emissions already done in the subwidget.
>   ........
> ................
>
>
>
>  M  +9 -2      icalformat_p.cpp
>  M  +22 -0     period.cpp
>  M  +5 -0      period.h
>  M  +1 -0      tests/CMakeLists.txt
>  A             tests/testfb.cpp  
> branches/kdepim/enterprise/kdepim/libkcal/tests/testfb.cpp#744087 [License:
> LGPL (v2+)]
>
>
> --- trunk/KDE/kdepimlibs/kcal/icalformat_p.cpp #777803:777804
> @@ -56,6 +56,7 @@
>  #include <ktzfiletimezone.h>
>  #include <ksystemtimezone.h>
>  #include <klocale.h>
> +#include <kcodecs.h>
>
>  #include <cstdlib>
>
> @@ -1177,13 +1178,19 @@
>      {
>        icalperiodtype icalperiod = icalproperty_get_freebusy( p );
>        KDateTime period_start = readICalUtcDateTime( p, icalperiod.start );
> +      Period period;
>        if ( !icaltime_is_null_time( icalperiod.end ) ) {
>          KDateTime period_end = readICalUtcDateTime( p, icalperiod.end );
> -        periods.append( Period( period_start, period_end ) );
> +        period = Period( period_start, period_end );
>        } else {
>          Duration duration ( readICalDuration( icalperiod.duration ) );
> -        periods.append( Period( period_start, duration ) );
> +        period = Period( period_start, duration );
>        }
> +      QByteArray param = icalproperty_get_parameter_as_string( p,
> "X-SUMMARY" );
> +      period.setSummary( QString::fromUtf8( 
> KCodecs::base64Decode( param ) ) );
> +      param = 
> icalproperty_get_parameter_as_string( p, "X-LOCATION" );
> +      period.setLocation( QString::fromUtf8( KCodecs::base64Decode( 
param ) ) );
> +      periods.append( period );
>        break;
>      }
>
> --- trunk/KDE/kdepimlibs/kcal/period.cpp #777803:777804
> @@ -51,6 +51,8 @@
>      KDateTime mEnd;      // period ending date/time
>      bool mHasDuration;   // does period have a duration?
>      bool mDailyDuration; // duration is defined as number of days, not
> seconds +    QString mSummary;
> +    QString mLocation;
>  };
>  //@endcond
>
> @@ -136,3 +138,23 @@
>    d->mEnd = d->mEnd.toTimeSpec( oldSpec );
>    d->mEnd.setTimeSpec( newSpec );
>  }
> +
> +QString Period::summary() const
> +{
> +  return d->mSummary;
> +}
> +
> +void Period::setSummary(const QString & summary)
> +{
> +  d->mSummary = summary;
> +}
> +
> +QString Period::location() const
> +{
> +  return d->mLocation;
> +}
> +
> +void Period::setLocation(const QString & location)
> +{
> +  d->mLocation = location;
> +}
> --- trunk/KDE/kdepimlibs/kcal/period.h #777803:777804
> @@ -187,6 +187,11 @@
>      void shiftTimes( const KDateTime::Spec &oldSpec,
>                       const KDateTime::Spec &newSpec );
>
> +    QString summary() const;
> +    void setSummary( const QString &summary );
> +    QString location() const;
> +    void setLocation( const QString &location );
> +
>    private:
>      //@cond PRIVATE
>      class Private;

-- 
David Jarvie.
KAlarm author and maintainer.
http://www.astrojar.org.uk/kalarm
_______________________________________________
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/