[MMS] should not show download-later UI as placeholder when auto-download is enabled

RESOLVED FIXED

Status

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

People

(Reporter: dietrich, Assigned: gnarf)

Tracking

unspecified
x86
Mac OS X
Dependency tree / graph
Bug Flags:
in-moztrap -

Firefox Tracking Flags

(blocking-b2g:leo+, b2g18 fixed)

Details

(Whiteboard: [uplift-blocker])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
if autodownload is enabled, we shouldn't be showing the download-later UI at all. instead we should just wait until entire message is downloaded and *then* show notifications and display message, etc
(Reporter)

Comment 1

5 years ago
even the current behavior is broken. right now, we show the download later UI, and then when download is complete, we don't remove it, so you end up with *both*.
(Reporter)

Updated

5 years ago
blocking-b2g: --- → leo+
Whiteboard: [uplift-blocker]
(Reporter)

Updated

5 years ago
Assignee: nobody → gnarf37
(Reporter)

Updated

5 years ago
Whiteboard: [uplift-blocker] → [uplift-blocker][NO_UPLIFT]
(Assignee)

Comment 2

5 years ago
Created attachment 753542 [details] [diff] [review]
patch v1

* Ignore 'pending' messages we receive in message manager to not render.
* Make 'pending' messages carry the status over (so it looks like its downloading) now that we have 'manual' status for manual download mode.
* Make appendMessage remove an old message of the same ID just in case
Attachment #753542 - Flags: review?(felash)
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
Comment on attachment 753542 [details] [diff] [review]
patch v1

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

::: apps/sms/js/thread_ui.js
@@ +870,5 @@
>          Utils.Message.format(message.body || '')
>        );
>      }
>  
>      if (notDownloaded) {

In message notDownloaded scenario, it looks we will have download button by default. It's true for manual/error status (and change 'downloading' for pending), but we could not download message while 'rejected' status(download option 'never'). Since we don't have 'never download' option for v1.1, I think it would be great to have a TODO comment here(and maybe move notDownloaded block into another function?)

@@ +897,5 @@
> +      if (status === 'pending') {
> +        downloadString = 'downloading';
> +        classNames.push('pending');
> +      }
> +

What if user click the button while downloading? I think we don't not disable the button here, but just want to make sure the message won't be downloaded twice(although it seems impossible...)
(Assignee)

Comment 4

5 years ago
Comment on attachment 753542 [details] [diff] [review]
patch v1

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

::: apps/sms/js/thread_ui.js
@@ +870,5 @@
>          Utils.Message.format(message.body || '')
>        );
>      }
>  
>      if (notDownloaded) {

I don't have a bug # for the TODO, but it should be pretty obvious we need to change rendering when that time happens.

I did abstract this into _createNotDownloadedHTML though

@@ +897,5 @@
> +      if (status === 'pending') {
> +        downloadString = 'downloading';
> +        classNames.push('pending');
> +      }
> +

When I allowed the user to start a second retrieve request, things got very strange because the error condition happened immediately.
Attachment #753542 - Flags: review?(felash)
(Assignee)

Comment 5

5 years ago
Created attachment 753607 [details] [diff] [review]
patch v2

Refactored the method to createNotDownloadedHTML
Attachment #753542 - Attachment is obsolete: true
Attachment #753607 - Flags: review?(schung)
Attachment #753607 - Flags: review?(felash)
(In reply to Corey Frang [:gnarf] [:gnarf37] from comment #4)
> Comment on attachment 753542 [details] [diff] [review]
> patch v1
> 
> Review of attachment 753542 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: apps/sms/js/thread_ui.js
> @@ +870,5 @@
> >          Utils.Message.format(message.body || '')
> >        );
> >      }
> >  
> >      if (notDownloaded) {
> 
> I don't have a bug # for the TODO, but it should be pretty obvious we need
> to change rendering when that time happens.

We don't have the bug # for disable mms download right now, but I think it's possible including this option in the future.

> 
> I did abstract this into _createNotDownloadedHTML though
> 
> @@ +897,5 @@
> > +      if (status === 'pending') {
> > +        downloadString = 'downloading';
> > +        classNames.push('pending');
> > +      }
> > +
> 
> When I allowed the user to start a second retrieve request, things got very
> strange because the error condition happened immediately.

Hi Chia-hong, could you please confirm that it's a bug or normal behavior that we don't have to handle it?
Flags: needinfo?(ctai)
(Assignee)

Comment 7

5 years ago
(In reply to Steve Chung from comment #6)
> We don't have the bug # for disable mms download right now, but I think it's
> possible including this option in the future.

I just mean that I don't really see the need to leave a comment here, it's a pretty obvious place to put this future code - without a bug # to reference it won't be much help anyway
Comment on attachment 753607 [details] [diff] [review]
patch v2

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

feedback+ for now, I feel we have questions to answer first.

::: apps/sms/js/message_manager.js
@@ +98,5 @@
> +      // Here we can only have one sender, so deliveryStatus[0] => message
> +      // status from sender. Ignore 'pending' messages that are received
> +      // this means we are in automatic download mode
> +      if (message.deliveryStatus[0] === 'pending')
> +        return;

I think this should be done in gecko instead: we should not receive the event for a pending message !

Or maybe I don't exactly get how this works. Do we receive an event on mozMobileMessage with the same message for each of its lifecycle events ?

::: apps/sms/js/thread_ui.js
@@ +856,5 @@
> +    var messageString = 'not-downloaded-mms';
> +    var downloadString = 'download';
> +
> +    // assuming that incoming message only has one deliveryStatus
> +    var status = message.deliveryStatus[0];

I know you did not write this, but shouldn't we check message.delivery ? Or maybe message.delivery does not have the same informations ?

@@ +871,5 @@
> +    }
> +
> +    if (status === 'error') {
> +      classNames.push('error');
> +    }

I think we already have this in buildMessageDOM (the `classNames = [XXX, delivery]` line). If deliveryStatus === "error" I believe "delivery" will be "error" too.

@@ +944,5 @@
> +    var messageDOM = this.container.querySelector(
> +      '[data-message-id="' + message.id + '"]');
> +    if (messageDOM) {
> +      this.removeMessageDOM(messageDOM);
> +    }

I feel like we do this a lot, can't we have a removeMessage that would take an id ? But feel free to ignore this suggestion now.

also, if we already have it, maybe we should better change the data instead of removing and creating again. But probably this is an optimization for an edge case that we could do later.
Attachment #753607 - Flags: review?(felash) → feedback+
(Assignee)

Comment 9

5 years ago
(In reply to Julien Wajsberg [:julienw] from comment #8)
> Comment on attachment 753607 [details] [diff] [review]
> patch v2
> 
> Review of attachment 753607 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> feedback+ for now, I feel we have questions to answer first.
> 
> I think this should be done in gecko instead: we should not receive the
> event for a pending message !
> 
> Or maybe I don't exactly get how this works. Do we receive an event on
> mozMobileMessage with the same message for each of its lifecycle events ?

Yes, we get multiple events, if we get a received message who is in deliveryStatus[0] === 'pending' it means we are in automatic download, its the only scenario this happens.
 
> ::: apps/sms/js/thread_ui.js
> @@ +856,5 @@
> > +    var messageString = 'not-downloaded-mms';
> > +    var downloadString = 'download';
> > +
> > +    // assuming that incoming message only has one deliveryStatus
> > +    var status = message.deliveryStatus[0];
> 
> I know you did not write this, but shouldn't we check message.delivery ? Or
> maybe message.delivery does not have the same informations ?
>

message.delivery === 'not-downloaded'; message.deliveryStatys[0] === 'error'; etc.

> @@ +871,5 @@
> > +    }
> > +
> > +    if (status === 'error') {
> > +      classNames.push('error');
> > +    }
> 
> I think we already have this in buildMessageDOM (the `classNames = [XXX,
> delivery]` line). If deliveryStatus === "error" I believe "delivery" will be
> "error" too.

See last comment

> @@ +944,5 @@
> > +    var messageDOM = this.container.querySelector(
> > +      '[data-message-id="' + message.id + '"]');
> > +    if (messageDOM) {
> > +      this.removeMessageDOM(messageDOM);
> > +    }
> 
> I feel like we do this a lot, can't we have a removeMessage that would take
> an id ? But feel free to ignore this suggestion now.
> 
> also, if we already have it, maybe we should better change the data instead
> of removing and creating again. But probably this is an optimization for an
> edge case that we could do later.

We could do this, it's part of me "refactor message rendering" :)
(Reporter)

Updated

5 years ago
Whiteboard: [uplift-blocker][NO_UPLIFT] → [uplift-blocker]
Comment on attachment 753607 [details] [diff] [review]
patch v2

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

r=me with the last change

::: apps/sms/js/message_manager.js
@@ +97,5 @@
>      if (threadId === Threads.currentId) {
> +      // Here we can only have one sender, so deliveryStatus[0] => message
> +      // status from sender. Ignore 'pending' messages that are received
> +      // this means we are in automatic download mode
> +      if (message.deliveryStatus[0] === 'pending')

move this at the very start of the function
Attachment #753607 - Flags: review?(schung) → review+
(Assignee)

Comment 11

5 years ago
https://github.com/mozilla-b2g/gaia/commit/09140a0a93dbdce2b1b72b57c361f17da34ebd19
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
After a good night, I think we need to be more clever here: we should update whatever existing message if we receive a new event for that existing message.

This goes with the fact that (I think) we need a special notice when the MMS is donwloading.

Shoul dwe file a new bug or do we have one already ?
(Assignee)

Comment 13

5 years ago
Julien - see bug 873508 - I started on this already but then ran into some problems and was trying really hard to fix a different problem anyway.
v1-train: e8b3666
status-b2g18: --- → fixed
Flags: needinfo?(ctai)

Updated

5 years ago
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.