Skip to content

feat: Always allow non-DC reactions to create chats #6769

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

iequidoo
Copy link
Collaborator

@iequidoo iequidoo commented Apr 5, 2025

This also unifies conditions for creating chats for chatmail and non-chatmail with
ShowEmails::All. As for DC reaction messages, they don't contain extra parts, so they mustn't
create chats.

Trying to resolve #6767 (comment)

@iequidoo iequidoo enabled auto-merge (squash) April 5, 2025 02:57
@iequidoo iequidoo requested a review from r10s April 5, 2025 02:59
@r10s
Copy link
Contributor

r10s commented Apr 5, 2025

thanks for the idea, however, at a first glance, the existing code seemms easier to read and understand.

usually, on tries to avoid "mut" for that reason.

as it is about is_reaction in classic path - maybe just set the condition there. that would also make the logic change easier to understand and to judge if there maybe was a reason for current logic

@iequidoo iequidoo marked this pull request as draft April 5, 2025 07:38
auto-merge was automatically disabled April 5, 2025 07:38

Pull request was converted to draft

This also unifies conditions for creating chats for chatmail and non-chatmail with
`ShowEmails::All`. As for DC reaction messages, they don't contain extra parts, so they mustn't
create chats.
@iequidoo iequidoo force-pushed the iequidoo/receive_imf-allow_creation branch from 7284137 to 8e9c046 Compare April 6, 2025 05:58
@iequidoo iequidoo changed the title refactor: Unify conditions for creating a chat for chatmail and non-chatmail with ShowEmails::All feat: Always allow non-DC reactions to create chats Apr 6, 2025
@iequidoo
Copy link
Collaborator Author

iequidoo commented Apr 6, 2025

After re-reading the code, i think that the intention was initially to avoid creating chats for DC reactions, so i changed it in this direction. A little more code, but at least the motivation should be clear now, otherwise the code looks maybe not buggy, but inconsistent.

@iequidoo iequidoo marked this pull request as ready for review April 6, 2025 06:06
@iequidoo
Copy link
Collaborator Author

iequidoo commented Apr 6, 2025

as it is about is_reaction in classic path - maybe just set the condition there. that would also make the logic change easier to understand and to judge if there maybe was a reason for current logic

Yes, alternatively, we can prevent all reactions from creating chats, anyway it's a corner case. At least this also would make the code consistent.

@iequidoo iequidoo requested a review from link2xt April 6, 2025 18:30
@link2xt
Copy link
Collaborator

link2xt commented Apr 7, 2025

There are only DC reactions, so checking if the reaction is from DC or not is not useful.
Reactions should not create chats indeed, otherwise you get an empty chat.

@iequidoo
Copy link
Collaborator Author

iequidoo commented Apr 8, 2025

IMU of https://www.rfc-editor.org/rfc/rfc9078.html, there may be other message parts, not containing Content-Disposition: reaction, but smth else useful. DC never creates such messages, and if they never exist in practice, we indeed may just add && !is_reaction check in the other branch. Anyway, this corner case isn't that important, so the PR code is ok i think

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