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

remove dead code #3459

Merged
merged 1 commit into from
Dec 2, 2024
Merged

remove dead code #3459

merged 1 commit into from
Dec 2, 2024

Conversation

r10s
Copy link
Member

@r10s r10s commented Dec 2, 2024

these RPC calls are probably a relict of first tries, they are not exhaustive in their area are nor were ever used, maybe not even working.
if we decide to go that way at some point anyways much more is needed :)

came over that when trying to get a little more grip on jsonrpc docs at deltachat/deltachat-core-rust#6303

these RPC calls are probably a relict of first tries,
they are not exhaustive in their area are nor were ever used,
maybe not even working.
if we decide to go that way at some point anyways much more is needed :)
@r10s r10s requested review from adbenitez and Hocuri December 2, 2024 10:52
Copy link

github-actions bot commented Dec 2, 2024

To test the changes in this pull request, install this apk:
📦 app-preview.apk

Copy link
Member

@Simon-Laux Simon-Laux left a comment

Choose a reason for hiding this comment

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

I'm not an android dev, but I agree that unused manual wrappers functions to the jsonrpc api should be removed - they are dead code.

In the future ideally we would have wrapper function auto-generation also for java, like we have for typescript. Manual wrapper functions are easy to become out of date with nobody noticing, especially if they are unused.

@r10s
Copy link
Member Author

r10s commented Dec 2, 2024

In the future ideally we would have wrapper function auto-generation also for java

that is not really an issue, there is already a PR in resurrection for auto-generation.

the real blockers are (a) the existing jsonrpc lacks documentation and has less structure (b) at least when i looked last time, basic things were missing or do much more than needed for android/iOS, resulting in additional, not needed database calls; the jsonrpc was not created with having android/iOS in mind. and (c) also performance, we never checked if lots of async calls are still fast enough, having in mind that they're mostly not needed for android/iOS and may result in some waiting cycles to get the return value

but that's better discussed in a sprint or so ... thanks for approval!

@r10s r10s merged commit 8728a0a into main Dec 2, 2024
2 checks passed
@r10s r10s deleted the r10s/remove-dead-code branch December 2, 2024 12:30
@Simon-Laux
Copy link
Member

Simon-Laux commented Dec 2, 2024

(a) I totally agree
(b) yes, it was primarily made for what was needed for desktop ui and is not as low-level
(c) jsonrpc will always have more steps than the cffi, as it needs to do more:

  • encode and decode json
  • find the right method to call based on method id
  • if using the async methods the ui also needs a way to match responses with callbacks/futures/promises that were returned back when the request was made.

So more work than the cffi.
It's not really much extra cost because of rust async that is also a performance cost in the cffi and core in general. In cffi all calls do block on, in jsonrpc api they spawn a tokio task.

But that async nature not really a real issue, because you can all jsonrpc function more directly with the blocking api.
Then there are less downsides compared to cffi:

downside upside
performance cost of json en/decoding and method id matching quicker/easier to develop/iterate thanks to auto generation & less layers of abstractions to write
inconsistent naming* better error handling/reporting*
methods missing (very easy to fix) easier to return structs through json
more fields in events that also labeled

* can be fixed in the oposing api

but that's better discussed in a sprint or so

I agree, though knowing the problems (even without directly fixing them) is important to me to paint a balanced picture in deltachat/deltachat-pages#984

@r10s
Copy link
Member Author

r10s commented Dec 2, 2024

i do not think that encoding/decoding json or looking up the method is an issue. if "blocking call" means it is a direct call in the same thread without additional waiting, so, in that part, similar to a cffi call, then that's probably fine

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.

2 participants