kcalcore/event.cpp
145–154	I suppose the the second part of the or could be translated into "d->mHasEndDate && !dtEnd.isValid()" ? It's rather hard to understand the intent as it is. Also, perhaps add a comment why that is necessary in the first place, one could assume that simply checking for the validity of mDtEnd should be enough.
kcalcore/incidencebase.cpp
314–320	same as above
kcalcore/recurrence.cpp
479	But the ending has likely changed and isn't the same, right? In that case remove that block.
706	The old code does also setDuration(-1), not sure if it changes anything.
718	noop ?
833	should this be done already, and if not, why should we ever do it?
988	extra whitespace
kcalcore/recurrence.h
433–503	All the new calls should probably be private API

This revision now requires changes to proceed.Jun 5 2015, 3:02 PM

updated the commets, will update the patch if we are clear with the way to go.

kcalcore/event.cpp
145–154	The second part is to be feautre complete to the old behaviour (see line 149). The problem is that you can set d->mHasEndDate via setHasEndDate, so we have no control about this. From a user perspective i expect, that the mHasEndDate is set if i call setDtEnd with a vaild date. For setting an invalid date i do not expect to change anything, if the enddate is invalid already. So the other way round loks better, if we really wanna change the behaviour: d->mDtEnd != dtEnd \|\| (dtEnd.isValid() && !d->mHasEndDate )
kcalcore/incidencebase.cpp
314–320	d->mAllDay != dtStart.isDateOnly() is the expression to have no behaviour change (we execute l 317). And in that case, i see no atvantage to change the behaviour or any better expression.
kcalcore/recurrence.cpp
479	When we have duration mode, the enddate is calculated, by the duration. So rrule->endDt() returns a valid datetime in anycase but points to the end of the duration. That's why we can skip the update with an invalid datetime.
706	well this is really a behaviour change: rrule->setDuration(-1); set recurrence type to never ( infinit recurrence). But i would say, that you normally change the recurrencence type and do not expect, that the recurrence end changes as well.
833	because the position inside the list has no meaning, we should sort the list before we test them. But RecurrenceRule::WDayPos is a type, not easy to sort. For our usecase it does not matter because the sorting is done my the GUI.

kcalcore/event.cpp
145–154	We don't want any change of behaviour. It is just not immediately obvious what d->mHasEndDate != dtEnd.isValid() is supposed to do which makes the code hard to read. What I proposed indeed only covers one case instead of both, so perhaps use a named boolean instead above the if: const bool addOrRemoveDtEnd = d->mHasEndDate != dtEnd.isValid; ...or just leave it as it is.
kcalcore/recurrence.cpp
479	That does sound like an optimization. If it isn't necessary please remove. It only makes the code even more complex.
706	If we want to merge this upstream we can't change the behaviour.

the rest of comments i'll include in the next update

kcalcore/recurrence.cpp
479	No this no optimization, if we do not add this, than an update of an invalid endTime will trigger endDate in dirtyFields. The point is that there are different "end modes" (never, date, duration). And end via duration, returns for endDate the endDate create out of the duration: that means that rrule->endDt() is not invalid. So an update with an invalid endDate will trigger the setEndDt.

update with comments from christan.

mollekopf edited edge metadata.

mollekopf added inline comments.

kcalcore/recurrence.cpp
479	Ok, then at least explain this better in the comment: "If the currence rule has a duration, and we're trying to set an invalid end date, we have to skip setting it to avoid setting the field dirty. We can do that because we assume that the end date is already invalid since a duration is set (they are mutually exclusive). We cannot leave it to the inequality check below because endDt() also returns a valid date for a duration."