[SMS / MMS] behavior when character limit of SMS is reached V1.1

RESOLVED FIXED

Status

Firefox OS
Gaia::SMS
P1
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: maat, Assigned: gnarf)

Tracking

unspecified
x86
Mac OS X
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:leo+, b2g18 fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

5 years ago
1) When the user is adding characters and the character limit of an SMS is passed the message format switches from SMS to MMS

-- scenario in which generated -- 
If the max number of characters an SMS can hold is 300 and the user has typed 300 characters, when they add the 301st character the message format is switched from SMS to MMS


2) When the message is already over the character limit for an SMS and so the message is in MMS format and whilst the user removes characters the character limit of an SMS is passed the message format switches from MMS to SMS

-- scenario in which generated -- 
If the max number of characters an SMS can hold is 300 and the user has typed 301 characters the message is in MMS format. When the user removes the 301st character the message format is switched from MMS to SMS
(Reporter)

Updated

5 years ago
Assignee: nobody → gnarf37
Priority: -- → P1
(Reporter)

Updated

5 years ago
Blocks: 842229
(Reporter)

Comment 1

5 years ago
Created attachment 747522 [details]
MMS : Switching between SMS and MMS due to character count

attaching HTML5_SMS-MMSUserStorySpecifications_char_count_20130509_V0.1 to clarify requirements for this bug. 

wireframe will be included in HTML5_SMS-MMSUserStorySpecifications V9.0 when pack is released.
Quote from my replies in private, "I remember we have a limit for 10 segments, which make 1530 characters with 8-bit segment counter."
(Assignee)

Comment 3

5 years ago
Created attachment 749467 [details] [diff] [review]
conversion to and from sms/mms banners

Though this switches the composer mode around between sms and mms, the conversion banner will now display properly, however the send method still doesn't properly know about MMS yet.
Attachment #749467 - Flags: review?(felash)
(Assignee)

Updated

5 years ago
Blocks: 840035
(Assignee)

Comment 4

5 years ago
Created attachment 749578 [details] [diff] [review]
conversion to and from sms/mms banners
Attachment #749467 - Attachment is obsolete: true
Attachment #749467 - Flags: review?(felash)
Attachment #749578 - Flags: review?(felash)
(Assignee)

Comment 5

5 years ago
Created attachment 749612 [details] [diff] [review]
38af647b01f53da09465649886f862582028dd60.patch

rebased on master (with attachment refactor)
Attachment #749578 - Attachment is obsolete: true
Attachment #749578 - Flags: review?(felash)
Attachment #749612 - Flags: review?(felash)
(Assignee)

Updated

5 years ago
Blocks: 872369
Comment on attachment 749612 [details] [diff] [review]
38af647b01f53da09465649886f862582028dd60.patch

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

I like sinon's fake timers, this could help in problems we have in other tests too.

globally a good patch, these are more lots of small comments. Note that I didn't try it on the device too.

::: apps/sms/js/compose.js
@@ +41,4 @@
>  
>      var textLength = dom.message.textContent.length;
>      var empty = !textLength;
> +    var iframes = dom.message.querySelector('iframe');

nit: you're getting only one, so the `s` at the end is wrong.

I'd rather convert this to `var hasIframe = !!dom.message.querySelector('iframe')`.

@@ +339,5 @@
> +        trigger('type', event);
> +        if (event.defaultPrevented) {
> +          state.type = oldValue;
> +        }
> +        dom.form.dataset.messageType = state.type;

nit: we don't need to do this if the default is prevented, right ?

::: apps/sms/js/thread_ui.js
@@ +255,5 @@
>    },
>  
> +  // Message composer type changed:
> +  messageComposerTypeHandler: function thui_messageComposerTypeHandler(event) {
> +    // if we are chaning to sms type, we might want to cancel

nit: changing

@@ +266,5 @@
> +    var message = navigator.mozL10n.get('converted-to-' + Compose.type);
> +    this.convertNotice.querySelector('p').textContent = message;
> +    this.convertNotice.classList.remove('hide');
> +
> +    clearTimeout(this.convertNoticeTimeout);

we might want to prefix this "private" variable with an underscore

@@ +269,5 @@
> +
> +    clearTimeout(this.convertNoticeTimeout);
> +    this.convertNoticeTimeout = setTimeout(function hideConvertNotice() {
> +      this.convertNotice.classList.add('hide');
> +    }.bind(this), 3000);

put this 3000 in a constant (cleaner, and will allow to override in tests)

@@ +402,4 @@
>      this.container.scrollTop = this.container.scrollHeight;
>    },
>  
> +  checkSmsSegmentLimit: function() {

soooo `checkSmsSegmentLimit` is modifying the counter ? maybe rename it to `updateSmsCounter` ?

@@ -416,5 @@
>  
> -    if (hasMaxLength || exceededMaxLength) {
> -      Compose.setMaxLength(value.length);
> -      var key = hasMaxLength ?
> -          'messages-max-length-text' : 'messages-exceeded-length-text';

you remove this but haven't removed the l10n keys; is that because you intend to use them back later ?

::: apps/sms/style/sms.css
@@ +520,5 @@
>    padding-right: 0.5rem;
>  }
>  
> +[data-message-type="mms"] #messages-send-button.has-counter:before {
> +  content: 'MMS';

I think this should not be here, because this is not localizable.

::: apps/sms/test/unit/compose_test.js
@@ +275,5 @@
> +        Compose.clear();
> +        typeChange.called = 0;
> +        form = document.getElementById('messages-compose-form');
> +      });
> +      test('Message switches type when adding/removing attachment',

you can convert this test to a suite with multiple tests

@@ +290,5 @@
> +        Compose.clear();
> +        assert.equal(form.dataset.messageType, 'sms');
> +        assert.equal(typeChange.called, 2);
> +      });
> +      test('Message type switch is cancelable', function() {

same here

::: apps/sms/test/unit/thread_ui_test.js
@@ +443,5 @@
> +    });
> +    teardown(function() {
> +      fakeTime.restore();
> +    });
> +    test('sms to mms and back displays banner', function() {

same here
Attachment #749612 - Flags: review?(felash)
(Assignee)

Comment 7

5 years ago
(In reply to Julien Wajsberg [:julienw] from comment #6)
> nit: you're getting only one, so the `s` at the end is wrong.
> 
> I'd rather convert this to `var hasIframe =
> !!dom.message.querySelector('iframe')`.

Fixed in new patch

> nit: we don't need to do this if the default is prevented, right ?

Fixed in new patch

> nit: changing

Fixed in new patch

> we might want to prefix this "private" variable with an underscore

Fixed in new patch

> put this 3000 in a constant (cleaner, and will allow to override in tests)

Fixed in new patch

> soooo `checkSmsSegmentLimit` is modifying the counter ? maybe rename it to
> `updateSmsCounter` ?

Fixed in new patch


> you remove this but haven't removed the l10n keys; is that because you
> intend to use them back later ?

Yes, the next patch down the line uses these still

> I think this should not be here, because this is not localizable.

I have seen a visual design that had a banner in the corner, I'm assuming this will be replaced with an image during a visual design pass.


After a discussion in IRC - Regarding the tests, all three of these tests depend on chaining state modifications, I think the way the tests are written now is probably best.
(Assignee)

Comment 8

5 years ago
Created attachment 750092 [details] [diff] [review]
870177-convert-message-type.patch
Attachment #749612 - Attachment is obsolete: true
Attachment #750092 - Flags: review?(felash)
Comment on attachment 750092 [details] [diff] [review]
870177-convert-message-type.patch

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

I'm not familiar with the code base, and have not tested the code, but I've verified that you've made all the changes Julien requested in his review.

See my comment about the code in thread_ui.js. r+ to land it if it won't incorrectly add a counter to an MMS message, and if you at least comment the method.

::: apps/sms/js/compose.js
@@ +344,5 @@
> +        }
> +      }
> +      return state.type;
> +    }
> +  });

If you're going to all this trouble to define a setter, I'm suprised you don't verify that the value is legal

::: apps/sms/js/thread_ui.js
@@ +425,5 @@
>      if (segments && (segments > 1 || availableChars <= 10)) {
>        this.sendButton.classList.add('has-counter');
>      } else {
>        this.sendButton.classList.remove('has-counter');
>      }

I don't like this bit. If there is an attempt to convert from MMS to SMS but the message is too long, this code will update and display the counter before rejecting the conversion attempt and reverting to MMS. Won't that result in an incorrect display? Or perhaps a flicker?

Rather than changing the name of hte function from check to update as Julien suggested, I'd prefer a modification of the method so that it just checks the segment limit without updating anything.

But failing that, at least add a comment at the start of the method saying what it does and what the return values mean.
Attachment #750092 - Flags: review?(felash) → review+
(Assignee)

Comment 10

5 years ago
Thanks David! - I added a sanity check to the type setter, it now only allows 'sms' and 'mms'.

I added some extra commenting around the updateSmsSegmentLimit function to adress the potential problems, which aren't actually problems due to when and how this method is called. Uploading final patch and landing
(Assignee)

Comment 11

5 years ago
Created attachment 750155 [details] [diff] [review]
870177-convert-message-type.patch
Attachment #750092 - Attachment is obsolete: true
Attachment #750155 - Flags: review?(dflanagan)

Updated

5 years ago
Attachment #750155 - Flags: review?(dflanagan) → review+
(Assignee)

Comment 12

5 years ago
master: acf36cfaa2e439b29179286b84883638ca127d7f
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
thx David !
(Assignee)

Updated

5 years ago
blocking-b2g: --- → leo?
blocking-b2g: leo? → leo+
v1--train: d1fc336
status-b2g18: --- → fixed
You need to log in before you can comment on or make changes to this bug.