Optimise recurrence rule editing #842 #849
Conversation
benbucksch
left a comment
benbucksch
left a comment
There was a problem hiding this comment.
Duh, I didn't send off the review.
| $: $event.startTime, $event.duration, updateDateUI(); | ||
| function updateDateUI() { | ||
| // This should close the repeat box, but this update might run first. |
There was a problem hiding this comment.
"This" "this"?
Unclear what the first and second "this" refer to.
There was a problem hiding this comment.
Sorry; for the first "this" I was referring to the frequency being None, while the second "this" refers to this function.
There was a problem hiding this comment.
Thanks. Can you please fix the comment?
| } | ||
| this.timezone ||= myTimezone(); | ||
| this.unedited = this.calendar.newEvent(); | ||
| // Use a raw event to avoid automatic instance generation. |
There was a problem hiding this comment.
Why does a raw Event not generate instances? It should!
Can you find another way which is more explicit?
There was a problem hiding this comment.
A raw event doesn't have a calendar, so we don't bother generating instances. The other way is to explicitly set the calendar to null.
There was a problem hiding this comment.
As mentioned, a raw Event should generate instances. We may need that for various circumstances in the future. At the very least, please assume that they do.
In general, never rely on assumptions about how other classes, components or other parts of the code are implemented. This leads to fragile code and is a big no-no in Mustang. Each class and component must depend only on the public API of other components, in form of functions and properties, and not on their specific and current implemention behavior. (And this here is nothing you can just document in a code comment, because it will likely change.)
This PR cannot land with this assumption.
There was a problem hiding this comment.
other classes
Then it's a good thing I'm making assumptions about how this class is implemented. Also, we already have other events that don't generate instances.
| // Use a raw event to avoid automatic instance generation. | ||
| this.unedited = new Event(); | ||
| this.unedited.copyFrom(this); | ||
| // Instead, just shallow clone our instances (needed for `seriesStatus`) |
There was a problem hiding this comment.
Unclear how that interacts with all our other recurrence editing and generating and saving code.
There was a problem hiding this comment.
Well, we shouldn't be saving the unedited version...
There was a problem hiding this comment.
Not sure what "Instead" refers to? What's the alternative that we intentionally don't do?
benbucksch
force-pushed
the
master
branch
3 times, most recently
from
d2ed3e0 to
3fc49e6
Compare
| } | ||
| this.timezone ||= myTimezone(); | ||
| this.unedited = this.calendar.newEvent(); | ||
| // Use a raw event to avoid automatic instance generation. |
There was a problem hiding this comment.
As mentioned, a raw Event should generate instances. We may need that for various circumstances in the future. At the very least, please assume that they do.
In general, never rely on assumptions about how other classes, components or other parts of the code are implemented. This leads to fragile code and is a big no-no in Mustang. Each class and component must depend only on the public API of other components, in form of functions and properties, and not on their specific and current implemention behavior. (And this here is nothing you can just document in a code comment, because it will likely change.)
This PR cannot land with this assumption.
| // Use a raw event to avoid automatic instance generation. | ||
| this.unedited = new Event(); | ||
| this.unedited.copyFrom(this); | ||
| // Instead, just shallow clone our instances (needed for `seriesStatus`) |
There was a problem hiding this comment.
Not sure what "Instead" refers to? What's the alternative that we intentionally don't do?
benbucksch
force-pushed
the
master
branch
2 times, most recently
from
7d2a658 to
3640c13
Compare
updateDateUIruns first; in this case we don't want the update, as the rule would be invalidupdateDateUIis supposed to run, the rule may already be valid, so don't update it, as that will regenerate the instances