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

[commhistoryd] Fix sending MMS to multiple contacts. Contributes to JB#24418 #2

Closed
wants to merge 1 commit into from

Conversation

aroditelev
Copy link

No description provided.

@@ -683,18 +683,22 @@ int MmsHandler::sendMessage(const QString &imsi, const QStringList &to, const QS
event.setStatus(Event::SendingStatus);
event.setIsRead(true);

// XXX Group conversations not yet supported, so we will send one message at a time
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from that it will break the possibility (for others) to reply to the group on this message, and how it shows up there, can't this risk becoming very expensive if the user is charged per message?
Seems some charge per-recipient anyway, most being fixed-rate monthly of course, but hard to rule out conclusively.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, many tariffs have mms and sms packages from 300 to 1000 pieces, for which there is no charge for sending. I agree that these changes do not solve the multicast issue, but this functionality is not currently available.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as the user is made aware, it is certainly better than nothing. 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, many tariffs have mms and sms packages from 300 to 1000 pieces, for which there is no charge for sending. I agree that these changes do not solve the multicast issue, but this functionality is not currently available.

mms-engine does support multiple recipients

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the current reality, ShareMms has the ability to send mms to several contacts, but without this fix, no messages will be sent.

@@ -683,18 +683,22 @@ int MmsHandler::sendMessage(const QString &imsi, const QStringList &to, const QS
event.setStatus(Event::SendingStatus);
event.setIsRead(true);

// XXX Group conversations not yet supported, so we will send one message at a time
if (to.size() + cc.size() + bcc.size() > 1) {
QStringList recepients = to + cc + bcc;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong. To, cc and bcc lists are separate arguments and separate fields in the MMS message.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@monich What's your idea of ​​not using cc and bcc in recepients?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite understand the question. org.nemomobile.MmsHandler.sendMessage (commhistoryd) receives them as separate lists so that they can be passed to org.nemomobile.MmsEngine.sendMessage (mms-engine) as separate lists, so that mms-engine can build a PDU where to, cc and bcc lists are separate lists.

The UI may or may not support building separate to, cc and bcc lists, but it's entirely a UX decision, for middleware it's a feature of the protocol, which middleware implements and provides to the UI.

Note that UI could call org.nemomobile.MmsEngine.sendMessage (mms-engine) directly and the message would be sent all right but it wouldn't be recorded in the commhistory database. The message goes through commhistoryd in order to record the message (and its attachments) in the database. IIRC multiple recipients were left unsupported because it wasn't clear how to represent multiple messages in the database in such a way that drafts, delivery reports, error handling etc. worked correctly.

Now...

Since after so many years it hasn't been implemented the right way (and MMS as a technology is slowly fading away), sending multiple copies of the same message doesn't look as such a bad idea anymore. It's definitely better than nothing, and anything better than that is unlikely to materialize. Two things though:

  1. Add a new D-Bus call (org.nemomobile.MmsHandler.sendMessage2 or another variant of org.nemomobile.MmsHandler.sendMessage, whatever you like better) with a single list of recipients, attachments as file descriptors and a return value which allows the UI to tell what happened (e.g. all messages were sent, some or none, or whatever UI needs to handle send errors in a sane and useful way). Leave this D-Bus call as is, don't change its semantics.
  2. Make sure that attachment files copied to commhistory storage are hard-linked, to avoid wasting storage space.

@aroditelev aroditelev closed this Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants