Skip to content
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

Don't reload event by UID if already in memory. #55

Merged
merged 2 commits into from Mar 17, 2023

Conversation

dcaliste
Copy link
Contributor

@dcaliste dcaliste commented Mar 8, 2023

Don't reload an existing incidence from DB.
Either the calendar is already in sync with
the calendar or the database has been externally
modified and in that case, the calendar has been
emptied.

Testing the parent incidence in case of recurring
incidence should be enough to guarantee the
existence of all exceptions since the load
methods ensure loading of whole series.

@pvuorela, tests are passing and this PR follow the same spirit as the one introducing the caching for ranges, meaning that there is no point in reloading if we assume that the calendar is in sync with the DB. It is a minor optimisation that should not cost too much in term of maintainablity. It may not be so minor though for search cases where there could be large quantity of events to load by UIDs.

Don't reload an existing incidence from DB.
Either the calendar is already in sync with
the calendar or the database has been externally
modified and in that case, the calendar has been
emptied.

Testing the parent incidence in case of recurring
incidence should be enough to guarantee the
existence of all exceptions since the load
methods ensure loading of whole series.
Copy link
Contributor

@pvuorela pvuorela left a comment

Choose a reason for hiding this comment

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

Somewhere on the borderline here, but since the range is indeed already tracking the loaded range and omitting those. And since the check should indeed guarantee the event is loaded already, let's just have this.

@pvuorela pvuorela merged commit 138c04b into sailfishos:master Mar 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants