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

api: cffi: add dc_event_get_json which gives the jsonrpc representation of an event #6326

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Simon-Laux
Copy link
Member

@Simon-Laux Simon-Laux commented Dec 10, 2024

This has the advantage that fields are named/labled and there can be potentially more than 2 (data1 & data2) fields on the events.

This removes the need for the (potentialy confusing) overloading data1 with dc_event_get_data1_str to add more information to events.
And also allows gradual/partial moving to the jsonrpc api, when needed or wanted.

Context/intention: seems logical and was easy to add, no personal grand plan behind it, just wanted to make life easier for UI devs.

Disclaimer: haven't tested it yet, but should work as it is really simple.

…tion of an event

This has the advantage that fields are named/labled and there can be potentially more than 2 (data1 & data2) fields on the events.

This removes the need for the (potentialy confusing) overloading data1 with `dc_event_get_data1_str` to add more information to events.
And also allows gradual/partial moving to the jsonrpc api, when needed or wanted.
deltachat-ffi/deltachat.h Outdated Show resolved Hide resolved
@Simon-Laux Simon-Laux requested a review from link2xt December 12, 2024 02:10
@Simon-Laux
Copy link
Member Author

Simon-Laux commented Jan 8, 2025

needs to be conditional on the jsonrpc feature flag. But I also ask myself if we still need the jsonrpc feature flag at all?

edit: I made #6417

@hpk42
Copy link
Contributor

hpk42 commented Jan 13, 2025

are there no tests against the jsonrpc API in the core?
Adding such a method without a test that becomes part of CI feels brittle.

@link2xt
Copy link
Collaborator

link2xt commented Jan 13, 2025

We kind of want to deprecate legacy Python bindings at some point, and it is the only part that tests CFFI.

What we really need is some UI to use this API instead of the old API. I'm also fine with merging this as is if it makes experimenting with this easier, if it's broken it is not a problem, but we need iOS or Android or DeltaTouch to use it eventually.

@r10s
Copy link
Member

r10s commented Jan 13, 2025

but we need iOS or Android or DeltaTouch to use it eventually

for Android/iOS, there is no need nor plans just now. but on the next event added, it may become handy - so, not sure if it makes sense to merge this PR, usually, we do not do that without UI counterpart. i would be fine to put it to resurrection and revive it, it if gets needed

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.

5 participants