Skip to content
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

feat: Make broadcast lists create their own chat #4644

Merged
merged 8 commits into from
Oct 17, 2023

Conversation

Hocuri
Copy link
Collaborator

@Hocuri Hocuri commented Aug 30, 2023

feat: Make broadcast lists create their own chat - UIs need to ask for the name when creating broadcast lists now (see deltachat/deltachat-android#2653)

That's quite a minimal approach: Add a List-ID header to outgoing broadcast lists, so that the receiving Delta Chat shows them as a separate chat, as talked about with @r10s and @hpk42. I know that @adbenitez will love it :)

To be done in other PRs:

  • Right now the receiving side shows "Mailing list" in the subtitle of such a chat, it would be nicer if it showed "Broadcast list" (or alternatively, rename "Broadcast list" to "Mailing list", too)
  • The UIs should probably ask for a name before creating the broadcast list, since it will actually be sent over the wire. (Android PR: Ask for the name when creating a broadcast list deltachat-android#2653)
  • Fix an existing bug that the chat name isn't updated when the broadcast/mailing list name changes (I already started this locally)

Fixes #4597

BREAKING CHANGE: This means that UIs need to ask for the name when creating a broadcast list, similar to deltachat/deltachat-android#2653.

@Hocuri Hocuri marked this pull request as draft August 30, 2023 15:11
@adbenitez
Copy link
Member

adbenitez commented Aug 30, 2023

@Hocuri you are my personal hero 🤩 I am looking forward to add this to deltalab as soon as possible to test it!

some thoughts:

about the name, what name will it have by default on the receiver side? the list address I guess but isn't that a random id? I have a mailing list with my family and I am worried how that will affect them, I expect them to at least see the broadcast name I have set so it is not super confusing to them realized it reading the code

how do you unsubscribe from such mailing list? would be nice in a future to allow to unsubscribe in a future sending some automated message to the broadcast creator to remove us from the broadcast list,

can you really block the broadcast or will they reappear?

shouldn't they be renamed to "Channel" or something like that? also allowing to share an invitation QR in the future since now the receiver side is aware of the broadcast it makes sense to allow to invite to join!

what about the broadcast avatar? it makes sense to sync that as well

@adbenitez
Copy link
Member

adbenitez commented Aug 30, 2023

I am already using it!! awesome!!! works like a charm overall, the biggest issue I found is this:
if I rename the broadcast list, the next message I send doesn't updates the name in the receiver side, this is an old bug in mailing list support in general, and will be great to fix it now solving two issues at the same time

(also notice that in DeltaLab I already mark read-only mailing lists as "Channel" subtitle so it is perfect fit!!)

image_2023-08-30_16-08-11

src/mimefactory.rs Outdated Show resolved Hide resolved
@link2xt
Copy link
Collaborator

link2xt commented Sep 11, 2023

The only missing part blocking this PR is "The UIs should probably ask for a name before creating the broadcast list, since it will actually be sent over the wire.". This needs a new API, something like dc_create_broadcast_list2(context, name) and deprecation of the old API.

@Hocuri
Copy link
Collaborator Author

Hocuri commented Sep 12, 2023

Alternatively, we could just let the UIs set a name right after creation (i.e. call dc_create_broadcast_list() and then directly dc_set_chat_name()). While this API isn't perfect, I would prefer it over needing a deprecation process.

@iequidoo
Copy link
Collaborator

Alternatively, we could just let the UIs set a name right after creation (i.e. call dc_create_broadcast_list() and then directly dc_set_chat_name()). While this API isn't perfect, I would prefer it over needing a deprecation process.

It's not perfect only if it will do more db transactions, but i don't think it's very important thing to optimise here

@Hocuri Hocuri changed the title Make broadcast lists create their own chat feat: Make broadcast lists create their own chat Sep 18, 2023
@Hocuri Hocuri force-pushed the hoc/broadcast-as-mailing-list branch from ff0d5b2 to 1fd1f9e Compare September 29, 2023 15:19
@Hocuri Hocuri marked this pull request as ready for review September 29, 2023 15:20
@Hocuri Hocuri requested review from link2xt and iequidoo September 29, 2023 15:20
@iequidoo
Copy link
Collaborator

iequidoo commented Oct 4, 2023

@Hocuri, i guess the PR could be merged already, but i still suggest to try using protected headers for "List-ID"

@link2xt
Copy link
Collaborator

link2xt commented Oct 10, 2023

@Hocuri Is this intended for master branch? In this case it cannot get into the next release, but will be released together with verified 1:1 chats and deltachat/deltachat-android#2653 will wait until master is finally merged into stable.
Alternatively, this can be merged into stable, but then we need to prepare all UIs for the next release and not just Android.

Fix an existing bug that the chat name isn't updated when the broadcast/mailing list name changes (I already started this locally)

If this goes into stable, then we also need a bugfix for this.

I would rather merge this into stable, seems to be relatively easy to get into the next release and focus mostly on verified 1:1 chats later.

@adbenitez
Copy link
Member

  • Right now the receiving side shows "Mailing list" in the subtitle of such a chat, it would be nicer if it showed "Broadcast list" (or alternatively, rename "Broadcast list" to "Mailing list", too)

IMHO naming it broadcast list doesn't makes much sense anymore since it is no longer a broadcast list (what users know from WhatsApp) but more like a Telegram "Channel", in theory any read-only mailing list on the wild is also a "channel", and the other non-readonly mailing lists are commonly called "discussion" (mailman) or "super group" (telegram)

@Hocuri Hocuri changed the base branch from master to stable October 13, 2023 10:03
…rom create_or_lookup_mailinglist()

…so that we can use it later to calculate the updated name for mailing
lists
…return the identifying mailinglist header and rename it to
get_mailinglist_header(), because I will need this in
apply_mailinglist_changes().
…the receiving side.

Previously, if a mailing list name was changed, it wasn't updated by the receiving device.
@Hocuri Hocuri force-pushed the hoc/broadcast-as-mailing-list branch from 0d95ebc to 7385778 Compare October 13, 2023 12:01
@Hocuri
Copy link
Collaborator Author

Hocuri commented Oct 13, 2023

@link2xt OK, I made a fix for this and pushed it into this PR. Probably it makes sense to review the four new commits commit-by-commit, they also have a short explanation on it each.

@adbenitez IIRC @hpk42 and @r10s were against calling it "channel". I would personally prefer "mailing list", but I see that this also has disadvantages. In any case, I don't think we have the time for this discussion now, the renaming can go into one or two releases later, if we want to go for it - broadcast lists are still experimental and need to be explicitly enabled in the settings, so I don't think renaming them later would be a problem.

@iequidoo iequidoo self-requested a review October 14, 2023 06:30
src/receive_imf.rs Outdated Show resolved Hide resolved
@Hocuri Hocuri requested a review from iequidoo October 15, 2023 13:55
Copy link
Member

@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.

lgtm, to move forward, we can merge this, thanks a lot!

for the UI, however, as there are anyways some changes needed, it would be great if we can identify broadcast lists on the receiver side already now as such (or is that already possible by some trick? or shall we follow the example of deltalab and name mailinglists and broadcasts the same?). showing them as "mailing lists" is a bit weird and will open discussions.

thinking it over, even if it is called "mailing list" for now, things should be good enough. we can discuss afterwards if mailing lists and broadcast should be the same for the receiver and which name it should get

@r10s
Copy link
Member

r10s commented Oct 16, 2023

i just tested, there is still some special handling of broadcast avatars in core; this should be removed or adapted as we allow setting the avatar in UI now

(core currently always returns ./assets/icon-broadcast.png - for simplicity, i would remove that completely, not even use it as fallback (which we're also not doing for mailing lists or groups currrently))

as this pr is already nicely reviewed, we can do that remove in a subsequent pr, but before deltachat/deltachat-android#2653 gets merged (as otherwise we have UI elements that are not working)

@Hocuri Hocuri merged commit 8573649 into stable Oct 17, 2023
@Hocuri Hocuri deleted the hoc/broadcast-as-mailing-list branch October 17, 2023 08:40
Hocuri added a commit to deltachat/deltachat-android that referenced this pull request Oct 19, 2023
Before deltachat/deltachat-core-rust#4644, when creating a broadcast list, the UIs didn't ask for a name since the name wasn't shown to the recipients and therefore not that important.

As of deltachat/deltachat-core-rust#4644, broadcast lists will create their own chat for the recipients, showing the broadcast lists's name, so we need to ask the user for a name when creating a broadcast list.
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.

Broadcast messages trigger ProtectionStatus::ProtectionBroken state
5 participants