-
-
Notifications
You must be signed in to change notification settings - Fork 117
api(rust and jsonrpc): marknoticed_all_chats method to mark all chats as notices, including muted ones. #7709
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
base: main
Are you sure you want to change the base?
Conversation
notices, including muted ones. made for solving deltachat/deltachat-desktop#5891 (comment)
closes #5891 depends on: - core with chatmail/core#7709 - should be merged after #5922
closes #5891 depends on: - core with chatmail/core#7709 - should be merged after #5922
WofWca
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks sensible overall, didn't test.
closes #5891 depends on: - core with chatmail/core#7709 - should be merged after #5922
Co-authored-by: WofWca <wofwca@protonmail.com>
| .await?; | ||
|
|
||
| for chat_id in list { | ||
| marknoticed_chat(context, chat_id).await?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider skipping messages arrived several seconds before, otherwise the user who wants to mark old messages as noticed may miss new messages, i.e. it's a user-app interaction race condition. Need to pass some flag to marknoticed_chat() for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a dedicated issue, it is already a problem / point for discussion. (problem not newly introduced by this pr)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this new feature it's worse because the probability of receiving a new message in any chat while interacting with the UI is much bigger than one for a particular chat. But as it's not a regression, i agree that it can be solved later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not a new feature, I'm just moving it to core. Desktop already has the same problem in its current implementation.
Anyway I'll make an issue or a PR for this later and will then mark this thread as resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, it's called "Mark all as read", but in fact it marks all messages as noticed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually, a user who marks a whole account as noticed doesn't care that much about the unseen messages. Otherwise, they would open the account, and look into all the chats. So, that's not a concern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually, a user who marks a whole account as noticed doesn't care that much about the unseen messages.
The timeout is about an edge case.
Another use case for the "mark all as read" feature is that you have an unread badge and the account open, but you can't easily find the chat with the unread message and just want to get rid of the badge.
Though you could still argue that the chat with the unread badge will probably be shown prominently enough at the top (unless it was archived).
So should we just ignore this for now in the name of saving work?
Disadvantage is that this kind of issue is not very likely to be reported because it will be rare, but it is about the user missing potentially important messages, so could be quite annoying for people who run into the issue.
| .await?; | ||
|
|
||
| for chat_id in list { | ||
| marknoticed_chat(context, chat_id).await?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't use marknoticed_chat(). E.g. marknoticed_chat() calls start_chat_ephemeral_timers(context, chat_id).await?;.
This function could probably just do this?
let noticed_msgs_count = context
.sql
.execute(
"UPDATE msgs
SET state=?
WHERE state=?
AND hidden=0",
(MessageState::InNoticed, MessageState::InFresh),
)
.await?;
if noticed_msgs_count > 0 {
// emit events
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why should it not all the ephemeral timers? Wouldn't it be a potential security/confidentiality issue if it would not call it, why does marknoticed_chat call the ephemeral timers?
| .await?; | ||
|
|
||
| for chat_id in list { | ||
| marknoticed_chat(context, chat_id).await?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually, a user who marks a whole account as noticed doesn't care that much about the unseen messages. Otherwise, they would open the account, and look into all the chats. So, that's not a concern.
method also mark chats with blocked contacts as noticed.
made for solving
deltachat/deltachat-desktop#5891 (comment)
will also be more efficient, because desktop currently loads all fresh messages to find out which chats to mark as noticed. https://github.com/deltachat/deltachat-desktop/blob/76d32bfc9364d53e1ffea40fcf4a60ad9a8482ee/packages/frontend/src/components/AccountListSidebar/AccountItem.tsx#L334
progress