[PATCHv1 0/5] mmsd: GetConversation support

[PATCHv1 0/5] mmsd: GetConversation support

[Ronald Tessier]

[-- Attachment #1: Type: text/plain, Size: 496 bytes --]

These patches concern mmsd and implement "GetConversation" D-Bus API.

Ronald Tessier (5):
  service: Add GetConversation method
  service: Retrieve get_conversation() args
  service: Fill conversation list with messages
  service: Sort conversation list by message date
  doc: Update service-api.txt

 doc/service-api.txt |    9 +++-
 src/service.c       |  171 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 179 insertions(+), 1 deletions(-)

--
1.7.4.1

[PATCHv1 1/5] service: Add GetConversation method

[Ronald Tessier]

[-- Attachment #1: Type: text/plain, Size: 1262 bytes --]

---
 src/service.c |   21 +++++++++++++++++++++
 1 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/src/service.c b/src/service.c
index f89fd3f..24c89b6 100644
--- a/src/service.c
+++ b/src/service.c
@@ -690,6 +690,26 @@ static DBusMessage *get_messages(DBusConnection *conn,
 	return reply;
 }

+static DBusMessage *get_conversation(DBusConnection *conn,
+					DBusMessage *dbus_msg, void *data)
+{
+	DBusMessage *reply;
+	DBusMessageIter iter, array;
+
+	reply = dbus_message_new_method_return(dbus_msg);
+	if (reply == NULL)
+		return __mms_error_trans_failure(dbus_msg);
+
+	dbus_message_iter_init_append(reply, &iter);
+
+	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
+							"(oa{sv})", &array);
+
+	dbus_message_iter_close_container(&iter, &array);
+
+	return reply;
+}
+
 static gboolean mms_attachment_is_smil(const struct mms_attachment *part)
 {
 	if (g_str_has_prefix(part->content_type, "application/smil"))
@@ -813,6 +833,7 @@ release_msg:
 static GDBusMethodTable service_methods[] = {
 	{ "SendMessage", "assa(sss)", "o", send_message },
 	{ "GetMessages", "", "a(oa{sv})", get_messages },
+	{ "GetConversation", "su", "a(oa{sv})", get_conversation },
 	{ }
 };

--
1.7.4.1

Re: [PATCHv1 1/5] service: Add GetConversation method

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 167 bytes --]

Hi Ronald,

>  src/service.c |   21 +++++++++++++++++++++
>  1 files changed, 21 insertions(+), 0 deletions(-)

patch has been applied.

Regards

Marcel

[PATCHv1 2/5] service: Retrieve get_conversation() args

[Ronald Tessier]

[-- Attachment #1: Type: text/plain, Size: 1525 bytes --]

---
 src/service.c |   37 +++++++++++++++++++++++++++++++++++++
 1 files changed, 37 insertions(+), 0 deletions(-)

diff --git a/src/service.c b/src/service.c
index 24c89b6..18c11f4 100644
--- a/src/service.c
+++ b/src/service.c
@@ -690,11 +690,48 @@ static DBusMessage *get_messages(DBusConnection *conn,
 	return reply;
 }

+static gboolean get_conversation_get_args(DBusMessage *dbus_msg,
+				const char **number, unsigned int *count)
+{
+	DBusMessageIter top_iter;
+
+	if (dbus_message_iter_init(dbus_msg, &top_iter) == FALSE)
+		return FALSE;
+
+	if (dbus_message_iter_get_arg_type(&top_iter) != DBUS_TYPE_STRING)
+		return FALSE;
+
+	dbus_message_iter_get_basic(&top_iter, number);
+	if (*number[0] == '\0')
+		return FALSE;
+
+	if (valid_number_format(*number) == FALSE)
+		return FALSE;
+
+	if (!dbus_message_iter_next(&top_iter))
+		return FALSE;
+
+	if (dbus_message_iter_get_arg_type(&top_iter) != DBUS_TYPE_UINT32)
+		return FALSE;
+
+	dbus_message_iter_get_basic(&top_iter, count);
+
+	return TRUE;
+}
+
 static DBusMessage *get_conversation(DBusConnection *conn,
 					DBusMessage *dbus_msg, void *data)
 {
 	DBusMessage *reply;
 	DBusMessageIter iter, array;
+	const char *number;
+	unsigned int count;
+
+	if (get_conversation_get_args(dbus_msg, &number, &count) == FALSE) {
+		mms_debug("Invalid arguments");
+
+		return __mms_error_invalid_args(dbus_msg);
+	}

 	reply = dbus_message_new_method_return(dbus_msg);
 	if (reply == NULL)
--
1.7.4.1

Re: [PATCHv1 2/5] service: Retrieve get_conversation() args

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 1825 bytes --]

Hi Ronald,

> ---
>  src/service.c |   37 +++++++++++++++++++++++++++++++++++++
>  1 files changed, 37 insertions(+), 0 deletions(-)
> 
> diff --git a/src/service.c b/src/service.c
> index 24c89b6..18c11f4 100644
> --- a/src/service.c
> +++ b/src/service.c
> @@ -690,11 +690,48 @@ static DBusMessage *get_messages(DBusConnection *conn,
>  	return reply;
>  }
> 
> +static gboolean get_conversation_get_args(DBusMessage *dbus_msg,
> +				const char **number, unsigned int *count)
> +{
> +	DBusMessageIter top_iter;
> +
> +	if (dbus_message_iter_init(dbus_msg, &top_iter) == FALSE)
> +		return FALSE;
> +
> +	if (dbus_message_iter_get_arg_type(&top_iter) != DBUS_TYPE_STRING)
> +		return FALSE;
> +
> +	dbus_message_iter_get_basic(&top_iter, number);
> +	if (*number[0] == '\0')
> +		return FALSE;
> +
> +	if (valid_number_format(*number) == FALSE)
> +		return FALSE;
> +
> +	if (!dbus_message_iter_next(&top_iter))
> +		return FALSE;
> +
> +	if (dbus_message_iter_get_arg_type(&top_iter) != DBUS_TYPE_UINT32)
> +		return FALSE;
> +
> +	dbus_message_iter_get_basic(&top_iter, count);
> +
> +	return TRUE;
> +}
> +
>  static DBusMessage *get_conversation(DBusConnection *conn,
>  					DBusMessage *dbus_msg, void *data)
>  {
>  	DBusMessage *reply;
>  	DBusMessageIter iter, array;
> +	const char *number;
> +	unsigned int count;
> +
> +	if (get_conversation_get_args(dbus_msg, &number, &count) == FALSE) {
> +		mms_debug("Invalid arguments");
> +
> +		return __mms_error_invalid_args(dbus_msg);
> +	}

in this case I don't really see that it is useful to split this into a
separate function. And the debug print is actually useless. Please
remove that one.

> 
>  	reply = dbus_message_new_method_return(dbus_msg);
>  	if (reply == NULL)

Regards

Marcel

[PATCHv1 3/5] service: Fill conversation list with messages

[Ronald Tessier]

[-- Attachment #1: Type: text/plain, Size: 3043 bytes --]

---
 src/service.c |   88 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 87 insertions(+), 1 deletions(-)

diff --git a/src/service.c b/src/service.c
index 18c11f4..2787c3a 100644
--- a/src/service.c
+++ b/src/service.c
@@ -719,13 +719,85 @@ static gboolean get_conversation_get_args(DBusMessage *dbus_msg,
 	return TRUE;
 }

+static gboolean is_recipient(const char *recipients, const char *number)
+{
+	const char *rec, *num;
+
+	while (*recipients != '\0') {
+		rec = recipients;
+		num = number;
+
+		while (*rec != '\0' && *num != '\0') {
+			if (*rec == *num) {
+				rec++;
+				num++;
+			} else {
+				if (*rec == '-' || *rec == '.') {
+					rec++;
+					continue;
+				}
+
+				if (*num == '-' || *num == '.') {
+					num++;
+					continue;
+				}
+
+				rec++;
+				break;
+			}
+		}
+
+		/*
+		 * Phone numbers in recipients end with /TYPE=PLMN, so the
+		 * wanted number is found if the whole string number is found
+		 * and if the matched phone number ends with the wanted number
+		 * (i.e.: "/TYPE=PLMN" must follow).
+		 */
+		if (*num == '\0' && *rec == '/')
+			return TRUE;
+
+		recipients = rec;
+	}
+
+	return FALSE;
+}
+
+static GList *fill_conversation(const struct mms_service *service,
+				GList *conversation, const char *number)
+{
+	GHashTableIter table_iter;
+	gpointer key, value;
+
+	g_hash_table_iter_init(&table_iter, service->messages);
+	while (g_hash_table_iter_next(&table_iter, &key, &value)) {
+		struct mms_message *msg = value;
+		char *recipients;
+
+		if (msg->type == MMS_MESSAGE_TYPE_SEND_REQ)
+			recipients = msg->sr.to;
+		else if (msg->type == MMS_MESSAGE_TYPE_RETRIEVE_CONF)
+			recipients = msg->rc.from;
+		else
+			continue;
+
+		if (is_recipient(recipients, number) == TRUE)
+			conversation = g_list_prepend(conversation, value);
+	}
+
+	return conversation;
+}
+
 static DBusMessage *get_conversation(DBusConnection *conn,
 					DBusMessage *dbus_msg, void *data)
 {
 	DBusMessage *reply;
 	DBusMessageIter iter, array;
+	const struct mms_service *service = data;
+	struct mms_message *msg;
+	GList *msg_elt = NULL;
+	GList *conversation = NULL;
 	const char *number;
-	unsigned int count;
+	unsigned int count, i;

 	if (get_conversation_get_args(dbus_msg, &number, &count) == FALSE) {
 		mms_debug("Invalid arguments");
@@ -742,6 +814,20 @@ static DBusMessage *get_conversation(DBusConnection *conn,
 	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
 							"(oa{sv})", &array);

+	conversation = fill_conversation(service, conversation, number);
+	if (conversation == NULL)
+		goto out;
+
+	i = 0;
+
+	for (msg_elt = g_list_first(conversation); msg_elt != NULL;
+					msg_elt = g_list_next(msg_elt), i++) {
+		if (count != 0 && i == count)
+			break;
+		msg = msg_elt->data;
+		append_message_entry(msg->path, service, msg, &array);
+	}
+out:
 	dbus_message_iter_close_container(&iter, &array);

 	return reply;
--
1.7.4.1

Re: [PATCHv1 3/5] service: Fill conversation list with messages

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 1476 bytes --]

Hi Ronald,

> ---
>  src/service.c |   88 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 87 insertions(+), 1 deletions(-)
> 
> diff --git a/src/service.c b/src/service.c
> index 18c11f4..2787c3a 100644
> --- a/src/service.c
> +++ b/src/service.c
> @@ -719,13 +719,85 @@ static gboolean get_conversation_get_args(DBusMessage *dbus_msg,
>  	return TRUE;
>  }
> 
> +static gboolean is_recipient(const char *recipients, const char *number)
> +{
> +	const char *rec, *num;
> +
> +	while (*recipients != '\0') {
> +		rec = recipients;
> +		num = number;
> +
> +		while (*rec != '\0' && *num != '\0') {
> +			if (*rec == *num) {
> +				rec++;
> +				num++;
> +			} else {
> +				if (*rec == '-' || *rec == '.') {
> +					rec++;
> +					continue;
> +				}
> +
> +				if (*num == '-' || *num == '.') {
> +					num++;
> +					continue;
> +				}
> +
> +				rec++;
> +				break;
> +			}
> +		}
> +
> +		/*
> +		 * Phone numbers in recipients end with /TYPE=PLMN, so the
> +		 * wanted number is found if the whole string number is found
> +		 * and if the matched phone number ends with the wanted number
> +		 * (i.e.: "/TYPE=PLMN" must follow).
> +		 */
> +		if (*num == '\0' && *rec == '/')
> +			return TRUE;
> +
> +		recipients = rec;
> +	}
> +
> +	return FALSE;
> +}

please explain what this function is actually doing? Adding a comment
seems like a good idea here.

Regards

Marcel

[PATCHv1 4/5] service: Sort conversation list by message date

[Ronald Tessier]

[-- Attachment #1: Type: text/plain, Size: 1215 bytes --]

---
 src/service.c |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/src/service.c b/src/service.c
index 2787c3a..1bd4671 100644
--- a/src/service.c
+++ b/src/service.c
@@ -719,6 +719,31 @@ static gboolean get_conversation_get_args(DBusMessage *dbus_msg,
 	return TRUE;
 }

+static gint fill_conversation_sort(gconstpointer a, gconstpointer b)
+{
+	const struct mms_message *msg1 = a;
+	const struct mms_message *msg2 = b;
+	time_t date1, date2;
+
+	if (msg1->type == MMS_MESSAGE_TYPE_SEND_REQ)
+		date1 = msg1->sr.date;
+	else
+		date1 = msg1->rc.date;
+
+	if (msg2->type == MMS_MESSAGE_TYPE_SEND_REQ)
+		date2 = msg2->sr.date;
+	else
+		date2 = msg2->rc.date;
+
+	if (date1 > date2)
+		return 1;
+
+	if (date1 < date2)
+		return -1;
+
+	return 0;
+}
+
 static gboolean is_recipient(const char *recipients, const char *number)
 {
 	const char *rec, *num;
@@ -784,6 +809,8 @@ static GList *fill_conversation(const struct mms_service *service,
 			conversation = g_list_prepend(conversation, value);
 	}

+	conversation = g_list_sort(conversation, fill_conversation_sort);
+
 	return conversation;
 }

--
1.7.4.1

Re: [PATCHv1 4/5] service: Sort conversation list by message date

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 1418 bytes --]

Hi Ronald,

>  src/service.c |   27 +++++++++++++++++++++++++++
>  1 files changed, 27 insertions(+), 0 deletions(-)
> 
> diff --git a/src/service.c b/src/service.c
> index 2787c3a..1bd4671 100644
> --- a/src/service.c
> +++ b/src/service.c
> @@ -719,6 +719,31 @@ static gboolean get_conversation_get_args(DBusMessage *dbus_msg,
>  	return TRUE;
>  }
> 
> +static gint fill_conversation_sort(gconstpointer a, gconstpointer b)
> +{
> +	const struct mms_message *msg1 = a;
> +	const struct mms_message *msg2 = b;
> +	time_t date1, date2;
> +
> +	if (msg1->type == MMS_MESSAGE_TYPE_SEND_REQ)
> +		date1 = msg1->sr.date;
> +	else
> +		date1 = msg1->rc.date;
> +
> +	if (msg2->type == MMS_MESSAGE_TYPE_SEND_REQ)
> +		date2 = msg2->sr.date;
> +	else
> +		date2 = msg2->rc.date;
> +
> +	if (date1 > date2)
> +		return 1;
> +
> +	if (date1 < date2)
> +		return -1;
> +
> +	return 0;
> +}
> +
>  static gboolean is_recipient(const char *recipients, const char *number)
>  {
>  	const char *rec, *num;
> @@ -784,6 +809,8 @@ static GList *fill_conversation(const struct mms_service *service,
>  			conversation = g_list_prepend(conversation, value);
>  	}
> 
> +	conversation = g_list_sort(conversation, fill_conversation_sort);
> +

is it really more efficient to prepend and then later sort the list? Can
we just not insert items sorted to begin with?

Regards

Marcel

[PATCHv1 5/5] doc: Update service-api.txt

[Ronald Tessier]

[-- Attachment #1: Type: text/plain, Size: 1062 bytes --]

---
 doc/service-api.txt |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/doc/service-api.txt b/doc/service-api.txt
index 46989f4..99e80be 100644
--- a/doc/service-api.txt
+++ b/doc/service-api.txt
@@ -24,11 +24,18 @@ Methods		array{object,dict} GetMessages()
 			that are part of a conversation between the service
 			entity and the number provided.

+			The number parameter must only contains	significant digits
+			to look for (i.e.: n last digits of the phone number), only
+			messages with a recipient containing the given number will
+			be part of the GetConversation result.
+
 			The count parameter can either be 0 for unlimited
 			messages in the conversation or limit the conversation
 			to count last messages.

-			Possible Errors: [service].Error.InvalidArguments
+			Possible Errors:
+				[service].Error.InvalidArguments
+				[service].Error.TransientFailure

 		object SendMessage(array{string} recipients, string smil,
 					array{string id, string content-type,
--
1.7.4.1

Re: [PATCHv1 5/5] doc: Update service-api.txt

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 1481 bytes --]

Hi Ronald,

>  doc/service-api.txt |    9 ++++++++-
>  1 files changed, 8 insertions(+), 1 deletions(-)
> 
> diff --git a/doc/service-api.txt b/doc/service-api.txt
> index 46989f4..99e80be 100644
> --- a/doc/service-api.txt
> +++ b/doc/service-api.txt
> @@ -24,11 +24,18 @@ Methods		array{object,dict} GetMessages()
>  			that are part of a conversation between the service
>  			entity and the number provided.
> 
> +			The number parameter must only contains	significant digits

actually using "must" is a wrong description.
> +			to look for (i.e.: n last digits of the phone number), only
> +			messages with a recipient containing the given number will
> +			be part of the GetConversation result.

And I am not sure we do wanna make some sort of assumption here anyway.
These are not CLIP numbers where the operators don't seem to get this
right. For SMS and MMS is seems the international numbering plan is
used.

Or do you have counter examples. If so, please post them here first.

> +
>  			The count parameter can either be 0 for unlimited
>  			messages in the conversation or limit the conversation
>  			to count last messages.
> 
> -			Possible Errors: [service].Error.InvalidArguments
> +			Possible Errors:
> +				[service].Error.InvalidArguments
> +				[service].Error.TransientFailure

Please don't invent your own indentation here. Keep it aligned as all
the other documents in oFono and mmsd do.

Regards

Marcel

Re: [PATCHv1 5/5] doc: Update service-api.txt

[Ronald Tessier]

[-- Attachment #1: Type: text/plain, Size: 2432 bytes --]

Hi Marcel,

On 04/27/2012 02:20 PM, Marcel Holtmann wrote:
> Hi Ronald,
>
>>   doc/service-api.txt |    9 ++++++++-
>>   1 files changed, 8 insertions(+), 1 deletions(-)
>>
>> diff --git a/doc/service-api.txt b/doc/service-api.txt
>> index 46989f4..99e80be 100644
>> --- a/doc/service-api.txt
>> +++ b/doc/service-api.txt
>> @@ -24,11 +24,18 @@ Methods		array{object,dict} GetMessages()
>>   			that are part of a conversation between the service
>>   			entity and the number provided.
>>
>> +			The number parameter must only contains	significant digits
>
> actually using "must" is a wrong description.
>> +			to look for (i.e.: n last digits of the phone number), only
>> +			messages with a recipient containing the given number will
>> +			be part of the GetConversation result.
>
> And I am not sure we do wanna make some sort of assumption here anyway.
> These are not CLIP numbers where the operators don't seem to get this
> right. For SMS and MMS is seems the international numbering plan is
> used.
>
> Or do you have counter examples. If so, please post them here first.

I fully agree with you, for received MMS the international format is 
used. But for sent messages, the number is given by the user, the number 
can be international or local format, it can even contain '-' or '.'. 
For example, in my storage, received message used +33612345678 and sent 
message used the local version such as 0612345678, to get the 
conversation, we need to look for 612345678.
That's why I proposed to limit the number parameter (the one we're 
looking for in the messages) to its significant digits in 
GetConversation method.

Otherwise (if we want to use international format for number parameter) 
we have to force the user to only use international numbering plan when 
sending message, is it right ?

>
>> +
>>   			The count parameter can either be 0 for unlimited
>>   			messages in the conversation or limit the conversation
>>   			to count last messages.
>>
>> -			Possible Errors: [service].Error.InvalidArguments
>> +			Possible Errors:
>> +				[service].Error.InvalidArguments
>> +				[service].Error.TransientFailure
>
> Please don't invent your own indentation here. Keep it aligned as all
> the other documents in oFono and mmsd do.
No problem, I'll also change 'SendMessage' documentation to  indent as 
it is done in oFono ;-)

Regards,

Ronald