Skip to content

Calendar: Edit: Recurrence: Move changeRemainder() from UI to logic - try 2 #763

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

benbucksch
Copy link
Collaborator

No description provided.

@benbucksch benbucksch requested a review from NeilRashbrook July 16, 2025 00:44
@benbucksch benbucksch self-assigned this Jul 16, 2025
@benbucksch
Copy link
Collaborator Author

Alternative to #750

This code still doesn't work. When "changing remainder", it deletes all instances from here on, including this one.

@benbucksch benbucksch changed the title Calendar: Edit: Recurrence: Move changeRemainer() from UI to logic Calendar: Edit: Recurrence: Move changeRemainer() from UI to logic - try 2 Jul 16, 2025
@benbucksch benbucksch changed the title Calendar: Edit: Recurrence: Move changeRemainer() from UI to logic - try 2 Calendar: Edit: Recurrence: Move changeRemainder() from UI to logic - try 2 Jul 16, 2025
Copy link
Collaborator

@NeilRashbrook NeilRashbrook left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When "changing remainder", it deletes all instances from here on, including this one.

Isn't that the desired behaviour? The new series starts from this instance. (Well, it does if you remember to add it to the calendar...)

Comment on lines -246 to +239
await saveEvent(master);
master.finishEditing();
let newMaster = event.cloneSeriesFromThisInstance();
await newMaster.save();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

saveEvent used to add the event to the calendar.

Suggested change
await saveEvent(master);
master.finishEditing();
let newMaster = event.cloneSeriesFromThisInstance();
await newMaster.save();
let newMaster = event.cloneSeriesFromThisInstance();
await newMaster.save();
newMaster.calendar.events.add(newMaster);

Comment on lines +275 to +278
let end = new Date(event.startTime);
event.parentEvent.cancelEditing();
event.cancelEditing();
await event.truncateRecurrence();
await event.setRecurrenceEndTime(end);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

event isn't the master here, and you probably want to delete the current instance too:

Suggested change
let end = new Date(event.startTime);
event.parentEvent.cancelEditing();
event.cancelEditing();
await event.truncateRecurrence();
await event.setRecurrenceEndTime(end);
event.parentEvent.cancelEditing();
event.cancelEditing();
await event.truncateRecurrencesFromHere();

@@ -729,6 +729,7 @@ export class Event extends Observable {
* Used when a master recurrence rule is removed or changed incompatibly.
*/
protected clearExceptions() {
assert(this.recurrenceCase == RecurrenceCase.Master, "Must be a recurring master");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this.recurrenceCase can't be relied upon at this point, because we might have just cleared the recurrence rule.

Comment on lines +135 to +136
event.truncateRecurrencesFromHere();
await event.save();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't want to save the event here as that tries to turn it from an instance into an exception. (The old truncateRecurrence had await master.save(), which is why it was async.)

Suggested change
event.truncateRecurrencesFromHere();
await event.save();
event.truncateRecurrencesFromHere();
await event.parentEvent.save();

Comment on lines +242 to +243
event.truncateRecurrencesFromHere();
await event.save();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

Suggested change
event.truncateRecurrencesFromHere();
await event.save();
event.truncateRecurrencesFromHere();
await event.parentEvent.save();

/** Creates a new series with the same properties and recurrence,
* but starting at this instance.
*
* It will also truncate the old series at that start time.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not any more it doesn't.

Comment on lines +802 to +803
let { frequency, interval, week, weekdays } = oldMaster.recurrenceRule;
newMaster.newRecurrenceRule(frequency, interval, week, weekdays);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oldMaster.recurrenceRule could be null if you changed the frequency in the UI first. (This would replace the remainder of the series with a single event.)

@benbucksch
Copy link
Collaborator Author

benbucksch commented Jul 16, 2025

When "changing remainder", it deletes all instances from here on, including this one.

Isn't that the desired behaviour? The new series starts from this instance.

Problem is that the new series is not created

(Well, it does if you remember to add it to the calendar...)

Oh, duh! Is that the reason why I work on this since 2 days and nothing works? Is it that silly? sigh

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants