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

[mkcal] Don't emit storageModified on notebook change. #64

Merged
merged 1 commit into from Jun 13, 2023

Conversation

dcaliste
Copy link
Contributor

@dcaliste dcaliste commented Jun 8, 2023

Like for events, there is no need to ask for
a complete calendar reset on a notebook change
since the modifications are known by the caller.

storageModified should be restricted for out
of process changes.

@pvuorela, since we're at cleaning mKCal API and behaviour, I would like to update the emitted signals on notebook changes. Currently a storageModified() signal is emitted, which means for clients "unknown database changes, please reset everything and reload". At first I wanted to create a new signal like the one existing for events (storageUpdated()) to notify for the notebook change, but I think there is no need. Clients making notebook changes know the change done since the API is synchronous in contrary to the event one which piles up changes done to the calendar and commit with the save() call.

In practice, the only client that requires to update something on a notebook change is the QML binding one. As far as I know, the Buteo sync plugins don't wait for a calendar wipe out on a notebook change. So I'm creating sailfishos/nemo-qml-plugin-calendar#58 to handle this.

Like for events, there is no need to ask for
a complete calendar reset on a notebook change
since the modifications are known by the caller.

storageModified should be restricted for out
of process changes.
@dcaliste
Copy link
Contributor Author

dcaliste commented Jun 8, 2023

I've just added a test to check that the storageModified signal is not emitted.

@pvuorela
Copy link
Contributor

pvuorela commented Jun 9, 2023

Wondering why this was done in the first place when the commit f3e123a made the signal defined from external changes and also avoided saving the transaction id. So now it works as specified? :)

@dcaliste
Copy link
Contributor Author

dcaliste commented Jun 9, 2023

I kept the storageModified signal for notebooks at that moment because I was convinced that I needed to create a signal (like the `storageUpadted() for the events) for the notebooks. So that clients can react on notebook change coming from in-process calls. I was undecided at that time how to create such a signal, that could handle notebook addition, update or deletion. Or should I have created three signals… Moreover, I was a bit dubious that it was right way to avoid complete reset because of in-process modifications. I was afraid of missing something and such modification was going to create regressions, even for events.

After one year of usage, I'm very happy with the solution and I believe it was the right way to differentiate for client code in-process modifications and external modifications. Only recently, I notice that for notebooks, with such API than the one we have in ExtendedStorage (immediate change is done in DB), there is actually no need to create such signals for notebooks. Which finally allow to complete f3e123a with very few changes.

@pvuorela pvuorela merged commit 4bbdcaf into sailfishos:master Jun 13, 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