Conversation
There is no need to have different names for the same props in chat and chatListItem. So let's rename them: - is_self_in_group → self_in_group - is_archived → archived - is_pinned → pinned - avatar_path → profile_image - is_device_chat → is_device_talk
|
clippy recommends to box the enum to avoid the linting error. So would ChatListItem(Box) with a pub struct ChatListItemData holding all the fields would be the way to go here? Or just add #[allow(clippy::large_enum_variant)] like it is done in lot.rs ? |
|
|
Why is it a temporary problem? The struct is only going to get larger, not smaller. But the good thing is that the vast majority of The problem with boxing is that the serialized JSON could change. Maybe not, maybe it's fine. But, as I said, just adding |
| is_self_in_group: chat.is_self_in_chat(ctx).await?, | ||
| self_in_group: chat.is_self_in_chat(ctx).await?, |
There was a problem hiding this comment.
chat.is_self_in_chat(ctx).await? is quite an expensive call, as it does a database request (just as most async functions). So, you should put let self_in_group = chat.is_self_in_chat(ctx).await?; above, and then use the variable here.
There was a problem hiding this comment.
i am wondering if [is]_self_in_group field is needed at all, esp. when it comes at costs.
[is]_self_in_group is currently only used to check whether to show the "N members" subtitle or "...", but that is probably not the ChalistItem then. and even if, it could be done by checking the anyway loaded contact list for CONTACT_ID_SELF (android code, desktop code)
(as a rule of thumb, we should try to avoid database calls needed to set a property that is not always needed - otherwise we have for the chatlist lots of never needed database calls, cffi is pretty strict on that, jsonrpc less in the past, resulting in hard to get slownesses, we're getting better there, but we should avoid that in the future)
There was a problem hiding this comment.
Based on that property we show the "leave group" context menu item, In chatlist where don't have the contact IDs of each listed group so we can't check if CONTACT_ID_SELF is in that list.
This precalculated property should avoid that we have to load the full chat when opening the context menu.
But in the end an extra DB call for each group item in the chat list (which is shown all the time) might be more expensive than the less frequent case, when the context menu item is opened. I would say it's fine to load the full chat when the context menu is opened (which I just introduced in the last refactoring) and then check in the contact list.
So let's get rid of self_in_group
When the deprected fields are removed eventually, clippy will stop to complain (but we should recheck). So i think suppressing the clippy error is fine for now, let's not optimize too early. |
allow clippy::large_enum_variant
Co-authored-by: iequidoo <117991069+iequidoo@users.noreply.github.com>
| is_self_talk: bool, | ||
| is_device_talk: bool, | ||
| is_sending_location: bool, | ||
| /// deprecated 2026-01 |
There was a problem hiding this comment.
| /// deprecated 2026-01 | |
| /// deprecated 2026-01, load the chat and check `self_in_group` instead |
| is_device_chat: bool, | ||
| /// Note that this is different from | ||
| /// [`ChatListItem::is_self_in_group`](`crate::api::types::chat_list::ChatListItemFetchResult::ChatListItem::is_self_in_group`). | ||
| /// [`ChatListItem::self_in_group`](`crate::api::types::chat_list::ChatListItemFetchResult::ChatListItem::self_in_group`). |
There was a problem hiding this comment.
This comment is wrong, then (apparently cargo doctest doesn't run in CI for deltachat-jsonrpc? Otherwise, it would have caught that).
Can just revert to the previous comment
4 participants
There is no need to have different names for the same props in chat and chat_list_item. So let's rename them: