api!: Remove msg_id (last message ID) from Chatlist::get_summary2() (#3071)

[iequidoo]

See commit messages.

[iequidoo]

Let's give it another try :) Now only dc_chatlist_get_summary2() (renamed to dc_chat_get_summary()) is changed, all functions taking dc_chatlist_t use cached message IDs as before and do the same SQL requests, so FFI API looks consistent and Android/iOS are untouched. As for Desktop, now it actually calls Message::load_from_db_last_for_chat() which does one SQL request instead of two, so i expect here even better performance and no races involving message IDs

[r10s]

ftr, for previous discussions and reasonings, see merged https://github.com/deltachat/deltachat-core-rust/pull/3931

[iequidoo]

Why change get_msg_id into get_chat_id everywhere? Maybe add a comment to get_msg_id that it is not recommended to use as the message may have expire if you think it would be nice to remove eventually?

Mostly to test Message::load_from_db_last_for_chat(). There's still test_parse_ndn_group_msg() that tests get_msg_id(). Also tried to improve the comment on get_msg_id().

[iequidoo]

unless i missed something, i am still not sure about this pr because of the slowness added for android/ios, see above and cmp #3931 (review)

As i mentioned before, i tried not to touch any API used in Android/iOS:

Now only dc_chatlist_get_summary2() (renamed to dc_chat_get_summary()) is changed, all functions taking dc_chatlist_t use cached message IDs as before and do the same SQL requests, so FFI API looks consistent and Android/iOS are untouched.

It's dc_chatlist_get_summary() who uses chatlist objects and cached message ids, and is used in Android/iOS. Please correct me if i'm wrong.

the potential race on desktop seems to be solved at #3931 sufficiently, so what is the real-world issue that satisfies the disadvantages and lots of discussions?

It is solved sufficiently for draft messages, but not for disappearing messages f.e. If it's not so difficult to solve it radically, why not to do that. Sorry for the long discussion, and thanks for the comments which helped me to try to not slow down things for Android/iOS, i think i should check which API is used there on my own, maybe i missed smth again.

[iequidoo]

Checked DC Android, only dc_chatlist_get_summary() is used there, but this PR doesn't touch it.

[r10s]

glimpsing at this PR again, it still looks to me as if it is a call for troubles, API changes, not to speak about possible performance degradations

If it's not so difficult to solve it radically, why not to do that.

this reminds me very much the comment "there's no much work in the clients code" of https://github.com/deltachat/deltachat-core-rust/pull/4314 :) but we have troubles with that since months, including user-visible degrations

i lost track about the consequences of this PR, not deep in this topic currently, however, i have the feeling that we again exchange minor issues for larger ones. that the requested changes are stale since nearly a year and systems do work underlines this impression. among all issues we seem not to have many issues with the chatlist

it really suggest to call it a day with this chatist refactoring for android/ios and close this PR.

if needed, let's do on-point, precise fixes for desktop, that does not even use cffi but jsonrpc meanwhile.

[iequidoo]

It's the further work started in https://github.com/deltachat/deltachat-core-rust/pull/3931 and it's not related to Android/iOS, it only reduces the number of SQL queries in the jsonrpc API and fixes the remaining races involving message ids. Refactoring in the cffi is not really needed, will revert. Also a better commit message is needed explaining which behaviour is changed and which is not, i understand that it's hard to review currently...

[r10s]

yeah, this is a source of confusion and hard to review, esp. when read complicated stuff with gaps of several months :) the lack of a clear, easy to understand goal does not help motivating :)

maybe as a first step, in another PR, remove code to have less confusion:

Now only dc_chatlist_get_summary2() (renamed to dc_chat_get_summary())

i think, both were never used by android, ios nor ubuntu-touch (did a quick search) - and desktop should use jsonrpc

maybe this pr is older than the move to jsonrpc :)

[iequidoo]

Reverted changes in the CFFI and improved the commit message so that it's clear what this PR is for.

[iequidoo]

dc_chatlist_get_summary2() needs to be removed first, otherwise this doesn't compile.