Skip to content

jsonrpc: add more functions, mostly message related #3590

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

Conversation

Simon-Laux
Copy link
Contributor

No description provided.

Copy link
Contributor

@r10s r10s left a comment

Choose a reason for hiding this comment

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

for the new BasicChat - where is the line that makes a difference to FullChat?

my advice would be, to make the object clearer, more useful and faster, to draw the line about what should be in and what not by the need of additional database callls:

so, add only add things where ...

  • one knows for sure they are always or most times needed whenever a BasicChat object is created
  • or things, that do not require additional database calls.

otherwise, things may easily end up in a mess, not much different from FullChat.

concrete, this means to think over adding can_send, profile_image and color - maybe remove them for now and use separate functions instead or re-add them if it turns out they are really needed for the majority of usages of BasicChat. or let the caller use FullChat. it is easier to add a field than to remove one :)

otherwise, we waste up to 3 database calls per load of a BasicChat structure.

also, i would document what makes the difference between BasicChat and FullChat - so future improvements are easier.

apart from these general thoughts, the pr lgtm.

@Simon-Laux Simon-Laux changed the title [WIP] jsonrpc: add more functions, mostly message related jsonrpc: add more functions, mostly message related Sep 9, 2022
@Simon-Laux Simon-Laux requested a review from r10s September 9, 2022 18:50
Copy link
Contributor

@r10s r10s left a comment

Choose a reason for hiding this comment

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

in case, we assume that chat image/color is needed most times BasicChat is created (not so unlikely), this lgtm.

@Jikstra
Copy link
Contributor

Jikstra commented Sep 10, 2022

for the new BasicChat - where is the line that makes a difference to FullChat?

my advice would be, to make the object clearer, more useful and faster, to draw the line about what should be in and what not by the need of additional database callls:

so, add only add things where ...

* one knows for _sure_ they are _always or most times_ needed whenever a `BasicChat` object is created

* or things, that do not require additional database calls.

otherwise, things may easily end up in a mess, not much different from FullChat.

concrete, this means to think over adding can_send, profile_image and color - maybe remove them for now and use separate functions instead or re-add them if it turns out they are really needed for the majority of usages of BasicChat. or let the caller use FullChat. it is easier to add a field than to remove one :)

otherwise, we waste up to 3 database calls per load of a BasicChat structure.

also, i would document what makes the difference between BasicChat and FullChat - so future improvements are easier.

apart from these general thoughts, the pr lgtm.

Agree with your thoughts, but at the same time it's about porting from js to rust and keeping the refactorings as sane as possible. Also weighting database accesses vs socket overhead I think won't make a big difference. Having the data structure properly modularized in jsonrpc will allow us to have a lot better and clearer overview over the types we use, and also evaluate better ones. But we can do this at any time, also removing properties will not be a problem as deltachat-desktop is the only consumer of this api for now.

@Simon-Laux Simon-Laux force-pushed the jsonrpc-add-missing-functions-for-making-message-list-work-in-desktop branch from 6471cf8 to 4ac4047 Compare September 11, 2022 17:19
@Simon-Laux Simon-Laux enabled auto-merge (squash) September 11, 2022 17:28
@Simon-Laux Simon-Laux merged commit d3f2db2 into master Sep 11, 2022
@Simon-Laux Simon-Laux deleted the jsonrpc-add-missing-functions-for-making-message-list-work-in-desktop branch September 11, 2022 17:48
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.

3 participants