rianjs / ical.net

ical.NET - an open source iCal library for .NET
MIT License
781 stars 230 forks source link

Event.Copy should do a deep copy #149

Open posics opened 7 years ago

posics commented 7 years ago

If I do var tmpEvent = permanentEvent.Copy(); var newStart = new CalDateTime(blabla); tmpEvent.Start = newStart;

then permanentEvent.Start has also changed to newStart. Shouldn't IEvent.Copy() do a deep copy and return a new instance with all things copied over, not just references?

I am doing all this because permanentEvent.getOccurrences(startTime, endTime) is returning occurences with just the start time set correctly and duration 0 (I see no end time anyway) and I need to calculate end and duration for each occurrence manually by creating a new event and copying stuff from permanentEvent. If getOccurrences() worked fine then I wouldn't have spotted this oddity. Or maybe it's the lack of documentation that is confusing me. Can we have that documentation please?

rianjs commented 7 years ago

Yep, sounds like a bug. I may have introduced it when I was evaluating all the usages of Copy in ical a few months ago. (Or maybe I didn't, and it's always been there.)

KotM commented 6 years ago

Hi @rianjs ,

could you please explain why this bug was closed? I tried to do an event copy (want to create an Exception from a recurrent event), but this is not actual copy, because changing the event field (e.g. Summary) also changes the parent fields value as well.

        var parentEvent = new Event();
        parentEvent.Start = new CalDateTime(2015, 1, 1);
        parentEvent.Summary = "parent summary";

        var copy = parentEvent.Copy<Event>();

        copy.Start = new CalDateTime(2016, 1, 1);
        copy.Summary = "not parent summary";

        Assert.AreNotEqual(parentEvent.Start, copy.Start); // fails!
        Assert.AreNotEqual(parentEvent.Summary, copy.Summary); // fails!

I have no other possibility to create copy of new event except serialize/deserialize, but it is poor performance.

rianjs commented 6 years ago

It isn’t closed.

KotM commented 6 years ago

Sorry for misunderstanding, very latest "Duplicate property DESCRIPTION" at the end of the list has confused me a bit. :)

So, do you have any plans and estimations for fixing this issue?

Or maybe you can propose another workaround except mentioned above serialize/deserialize approach?

Thank you in advance.

rianjs commented 6 years ago

So, do you have any plans and estimations for fixing this issue?

No.

Or maybe you can propose another workaround except mentioned above serialize/deserialize approach?

That's what we do today, hundreds of VCALENDARs and VEVENTs at a time, often as part of a click handler because it's fast enough, especially if you're working with collections AsParallel().