965312 - Evaluate having an option for specifying the cursor direction when retrieving message threads

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: RIL, defect)

Description

[Gabriele Svelto [:gsvelto]]

+++ This bug was initially created as a clone of Bug #963475 +++

As discussed in bug 963475 we might want to add an option to specify in which order the threads retrieved via MozMobileMessageManager.getThreads() are returned. We have a similar option for MozMobileMessageManager.getMessages() and it's specified as an optional boolean parameter. As :rwaldron pointed out in bug 963475 comment 13 this is not very readable by someone who's not already familiar with the API. Instead an optional options object (no pun intended) can be passed, e.g.:

* Default order of retrieval

MozMobileMessageManager.getThreads()

or

MozMobileMessageManager.getThreads({ reverse: false })

* Reversed order of retrieval

MozMobileMessageManager.getThreads({ reverse: true })

For both better readability and consistency we might modify MozMobileMessageManager.getMessages's second parameter accordingly.

Comment 1

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] Add an options object to specify the order of retrieval of messages and threads

Comment 2

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] Add an options object to specify the order of retrieval of messages and threads

Comment 3

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH gaia] Adapt the SMS app to use the modified API

Comment 4

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8373365 [details] [diff] [review]
[PATCH] Add an options object to specify the order of retrieval of messages and threads

This is my proposed change to the MozMobileMessageManager API that uses an options object to pass optional parameters to the getThreads() and getMessages() calls. In this case I've implemented the |reverse| parameter that was already available in getMessages() and I've extended it to cover getThreads() too.

I had only a couple of answers on dev-webapi about this but both seemed positive so I think it should be OK to go ahead with it.

I've also attached a Gaia patch that modifies the SMS app to make use of the new parameters.

Comment 5

[Rick Waldron [:rwaldron]]

This looks really good, thank you for addressing the argument form :)

Comment 6

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8373365 [details] [diff] [review]
[PATCH] Add an options object to specify the order of retrieval of messages and threads

Review of attachment 8373365 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/mobilemessage/src/MobileMessageManager.cpp
@@ +402,5 @@
>    if (!filter) {
>      filter = new SmsFilter();
>    }
>  
> +  if (aArgc == 1) {

aArgc == 2

::: dom/mobilemessage/tests/marionette/test_getthreads.js
@@ +166,2 @@
>           "' should be found in bodies array.");
> +      messageCount++;

The problem here is, there might be two messages of the same timestamp.  SMS timstamps are of seconds.  It's quite possible that in our testing environment, especially running on your mighty desktop beast, to have two messages of identical timestamp.  For these messages, their order in the output of |getMessages| is undefined, and therefore we cannot assume an absolutely increasing order here.

Comment 7

[Gabriele Svelto [:gsvelto]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #6)
> ::: dom/mobilemessage/src/MobileMessageManager.cpp
> @@ +402,5 @@
> >    if (!filter) {
> >      filter = new SmsFilter();
> >    }
> >  
> > +  if (aArgc == 1) {
> 
> aArgc == 2

Strangely enough that doesn't seem to work. It seems that aArgc == 0 if I pass only one argument and aArgc == 1 if I pass two. Is it possible it's counting only the optional arguments and not the required ones?

> ::: dom/mobilemessage/tests/marionette/test_getthreads.js
> @@ +166,2 @@
> >           "' should be found in bodies array.");
> > +      messageCount++;
> 
> The problem here is, there might be two messages of the same timestamp.  SMS
> timstamps are of seconds.  It's quite possible that in our testing
> environment, especially running on your mighty desktop beast, to have two
> messages of identical timestamp.  For these messages, their order in the
> output of |getMessages| is undefined, and therefore we cannot assume an
> absolutely increasing order here.

I'm not exactly sure how this affects that particular bit of code; the tests seem to pass on my machine though. Do you mean this might introduce intermittent issues if we end up with two messages with the same timestamp?

Comment 8

[Gabriele Svelto [:gsvelto]]

We will most likely remove this API.