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] Drop Extended* object from email API #39

Merged
merged 1 commit into from May 22, 2023

Conversation

dcaliste
Copy link
Contributor

@dcaliste dcaliste commented Dec 1, 2022

This is an API break, but there is no sense to use ::Ptr in this API. Implementation should not keep any pointer on the passed notebook and incidences. More over, having the ::Ptr avoids the use of the service in context where only plain notebook and incidences are available.

Rationalise the ServiceHandler API, not passing the ExtendedStorage pointer when it's not used in the actual interface and only there to check notebook validity with respect to the storage.

@pvuorela, this is not in a hurry, but I would like your opinion on the opportunity to update this plugin API. Having it in ::Ptr everywhere and with useless additional arguments makes it difficult to use it in an environment different from the one now : a ExtendedStorage in the same thread as the service. While I would like to use it also in a different thread because none of the function are actually related to an ExtendedStorage object but they are much more local, either using a Notebook or an Incidence.

It will require to update the caldavinvitation plugin and maybe one in EAS (while actually it seems to me that the defaultInvitationPlugin is actually playing this role).

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.

Partly again pondering how much we gain by avoiding the extra Ptr, but at least the service handler usage is lot more limited.

src/servicehandler.cpp Outdated Show resolved Hide resolved
src/servicehandler.cpp Outdated Show resolved Hide resolved
src/servicehandler.cpp Outdated Show resolved Hide resolved
src/servicehandler.cpp Outdated Show resolved Hide resolved
@dcaliste
Copy link
Contributor Author

I've rebased this PR also. Maybe dropping the ::Ptr stuff from the API is not worth the API breakage, but working now on the replacement, or improvement, of Extended{Calendar,Storage} from nemo-qml-plugin-calendar to something that would allow incidence sharing UID in multi notebooks, I think, it may be worth to remove the Extended{Calendar,Storage} from this service API, since it's not really needed and can be better replaced by the Notebook directly.

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.

Let's keep the Ptr for now. It's common convention on kf5-calendarcore and shouldn't matter on the calls too much. Less adjustments to make on depending plugins and keeps the other changes more visible if not changing the types.

@dcaliste
Copy link
Contributor Author

Ok, I've put back the ::Ptr where they were. It implies less changes to the downstream code. But it's a bit ugly to have it here where it's not necessary and sometime even harmfull since we need to pass a ::Ptr-ified pointer.

@dcaliste dcaliste changed the title [mkcal] Remove usage of ::Ptr in service handler. [mkcal] Drop Extended* object from email API May 17, 2023
src/notebook.h Outdated Show resolved Hide resolved
src/notebook.cpp Outdated Show resolved Hide resolved
src/servicehandler.cpp Outdated Show resolved Hide resolved
src/servicehandler.h Outdated Show resolved Hide resolved
src/servicehandler.cpp Outdated Show resolved Hide resolved
@pvuorela
Copy link
Contributor

Ok, I've put back the ::Ptr where they were. It implies less changes to the downstream code. But it's a bit ugly to have it here where it's not necessary and sometime even harmfull since we need to pass a ::Ptr-ified pointer.

Thanks. Easier now to go through the other changes. Yea, some extra lines for using that but didn't seem too bad.

@pvuorela
Copy link
Contributor

Also indeed the Extended... classes seem somewhat pointless

@dcaliste
Copy link
Contributor Author

Thank you for the review. Besides addressing your remarks, I've added tests in every routines with a ::Ptr argument to guard against null() values. I removed them when removing the ::Ptr types but I forgot to put them back...

Rationalise the ServiceHandler API, not passing the
ExtendedStorage pointer when it's not used in the actual
interface and only there to check notebook validity
with respect to the storage. Instead, always pass directly
the notebook the incidence is associated with. Let the
client retrieve this notebook, depending how the
incidence is stored.
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.

Looking good. Nicely cleans up the handler code too.

@pvuorela pvuorela merged commit 22292b4 into sailfishos:master May 22, 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