917639 - [SMS]Thread's time is different from Conversation's time, when timezone changes on settings during message apps background

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect)

Description

[kenzo Ohta]

User Agent: Mozilla/5.0 (Windows NT 5.1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/29.0.1547.66 Safari/537.36

Steps to reproduce:

1. SMS app is launched (check any thread's and the conversation's recent mail time)
2. settings app is launched (SMS app is turn background)
3. select Date & Time
4. change Region (ex)from Europe to America
5. change SMS app foreground


Actual results:

thread's time is not changed(conversation's time is changed)


Expected results:

thread's time is changed

Comment 1

[kenzo Ohta]

Change Region and change City is same result.

Comment 2

[at-kitamura]

This is available by handling a change of setting value "time.timezone", and drawing threads list again.

For reference...

>  Add to 'startup.js'
>  ------------------------
>  navigator.mozSettings.addObserver('time.timezone', function onTimeZoneChanged(evt){
>    MessageManager.getThreads(ThreadListUI.renderThreads);
>  });
>  ------------------------

Comment 3

[Julien Wajsberg [:julienw]]

Let's handle this after bug 901852 if this is still happening.

We don't want to rerender all threads when that happens, we can simply change the timeheaders, and we currently do this on visibility change, which should cover this case too.

So let's see if this still happens after bug 901852 lands.

Thanks !

Comment 4

[Julien Wajsberg [:julienw]]

bug 901852 landed today, can you please check if this still happens ?

If yes, I'm perfectly happy that you work on this :)
Otherwise, just close as duplicate to bug 901852 please!

Comment 5

[kenzo Ohta]

Hi Julien-san

I have re-checked this issue, but unhappy restult.
This bug still happen, because this issue point is not header.
So now kitamura-san tries modifying this issue.

Comment 6

[Julien Wajsberg [:julienw]]

I added kitamura-san as assignee.

Could you please attach images that show the problem ?

We're supposed to use exactly the same code for a conversation and for the thread list, so something must be missing somewhere. Maybe a data-attribute is missing.

Comment 7

[at-kitamura]

Attachment: Bugzilla917639.pdf

Comment 8

[at-kitamura]

Attachment: Bugzilla917639.patch

Hi Julien-san,

I attached image(Bugzilla917639.pdf) that show this problem. 
And because I have already created a patch(Bugzilla917639.patch), I attached it together.

This patch has the following correspondence. Please check it.

 - If a document is not hidden when application handled 'visibilitychange', I update time in 'li' elements(messages-thread).
 - I add test suite of [When visibility of document is changed] in thread_list_ui_test.js.

Comment 9

[Julien Wajsberg [:julienw]]

Comment on attachment 827722 [details] [diff] [review]
Bugzilla917639.patch

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

Hi,

thanks a lot for the patch!

However I'd like that you integrate it here instead: https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/time_headers.js#L35

That means:
* add the data-attribute where appropriate
* change the selector in that line, so that it can get these nodes as well

Thanks!

Comment 10

[at-kitamura]

Hi Julien-san,

Thanks for your review.

I think that I should not integrate it in 'time_headers.js' for the following reasons.
 - These 'time elements' are the time of the message received at finally and these are not necessary to update it regularly（once in 60 seconds）.
 - These 'time elements' are not headers.

Thearefore I added implementation to 'thread_list_ui.js'.

Should I integrate it in 'time_headers.js'?

Comment 11

[Julien Wajsberg [:julienw]]

I agree this should not be in the scheduler, but you can still integrate it in the "visibility change" handler in time_headers.js.

Maybe the "TimeHeaders" name is not good, and should be renamed to "TimeHandler".

Comment 12

[at-kitamura]

Attachment: Bugzilla917639.patch

Hi Julien-san,

I understood your suggestion.

I integrated these correspondences into function 'onvisibilityChange' of 'time_headers.js'.
(I did not change the file name.)

I reuploaded patch. Please check it.

Comment 13

[Julien Wajsberg [:julienw]]

Comment on attachment 830033 [details] [diff] [review]
Bugzilla917639.patch

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

I think we can try to be more clever.

Basically we have 2 types of elements:
* elements where we need to update the time every minute (the existing ones)
* elements where we need to update the time only on visibility change (the new ones)

Currently, the first set of elements use a `dataset.timeUpdate = true` marker. I think we should do the same for the second set of elements.

How about using:
* `dataset.timeUpdate = "repeat"` for the first set of elements
* `dataset.timeUpdate = true` for the second set of elements

You'll need to:
* change the existing use of timeUpdate in thread_ui.js and thread_list_ui.js to use "repeat" instead of "true".
* in time_headers.js/updateAll function:
  + pass a selector to this function, and use this selector in the "querySelectorAll" call
  + pass "header[data-time-update=repeat]" in all existing calls, and use "updateAll('time[data-time-update=true]')" instead of calling your "updateThreadList".
* change how the thread is created in thread_list_ui.js:
  + change the template messages-thread-tmpl in index.html to set the "time" (change the "formattedDate" parameter to a "timestamp" parameter), "timeUpdate" ("true") and "timeOnly" ("true") dataset
  + in thread_list_ui.js/createThread: pass the new "timestamp" parameter instead of "formattedDate", and call "TimeHeaders.update" on the "time" element (you'll need to querySelector it)

What do you think of this plan?

In the future, I think it would be better to use only <time> elements instead of using both <time> and <header> elements, would make our code more generic. You can work on this if you want, but in another patch and another bug, because this involves some dangerous changes in how threads and messages DOM nodes are created.

::: apps/sms/test/unit/time_headers_test.js
@@ +183,5 @@
> +        assert.equal(timeElements[i].textContent, existingTime[i]);
> +      }
> +    });
> +
> +    test('calling after one hour should update time elements', function() {

the test should be "changing timezone should update time elements"

@@ +187,5 @@
> +    test('calling after one hour should update time elements', function() {
> +      this.sinon.clock.tick(60 * 60 * 1000);
> +      var threadlist = document.getElementsByTagName('li');
> +      for (var i = 0, l = threadlist.length; i < l; i++) {
> +        threadlist[i].dataset.time = Date.now();

you can add a comment explaining "since we can't change timezones easily, we simulate this by changing the stored 'time' dataset"

@@ +192,5 @@
> +      }
> +
> +      TimeHeaders.updateThreadList();
> +      var timeElements = document.getElementsByTagName('time');
> +      for (var i = 0, l = timeElements.length; i < l; i++) {

nit: `i` and `l` are already defined

Comment 14

[at-kitamura]

(In reply to Julien Wajsberg [:julienw] from comment #13)

Thanks for your suggestion.

I almost understood your suggestion, but I have one question.

> * in time_headers.js/updateAll function:
>   + pass a selector to this function, and use this selector in the
> "querySelectorAll" call
>   + pass "header[data-time-update=repeat]" in all existing calls, and use
> "updateAll('time[data-time-update=true]')" instead of calling your
> "updateThreadList".

Is this a meaning to update both 'header[data-time-update=repeat]' and 'time[data-time-update=true]' in "updateAll()"?

In that case, it is necessary to change updateAll()
  updateAll() -> updateAll(repeat).
Because 'time[data-time-update=true]' should not be updated regularly, right?

Comment 15

[Julien Wajsberg [:julienw]]

Yes, that's exactly what I mean when I write "pass "header[data-time-update=repeat]" in all existing calls" :)

Thanks!

Comment 16

[at-kitamura]

Attachment: Bugzilla917639.patch

Hi Julien-san,

I uploaded a patch again. I think that all your suggestion is included in the patch...
Please check it.

This patch has the following correspondence.
 * add an argument('repeat' is 'true' or 'false') to 'updateAll()'.
  - when repeat is false, headers('header[data-time-update]') and time elements('time[data-time-update=true]') are updated.
  - when repeat is true, only headers('header[data-time-update]') is updated.
 * 'startScheduler()' is changed
  - call 'updateAll(true)' at the first time and call 'updateAll(false)' at after the 2nd time.
 * use 'updateAll(true)' instead of 'updateAll()'.

Comment 17

[Julien Wajsberg [:julienw]]

Comment on attachment 8335115 [details] [diff] [review]
Bugzilla917639.patch

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

Hey!
Some more comments, We're getting there :)

Tell me what you think!

::: apps/sms/js/thread_list_ui.js
@@ +390,5 @@
>  
> +    var timeElements = li.getElementsByTagName('time');
> +    if (timeElements) {
> +        TimeHeaders.update(timeElements[0]);
> +    }

You know for sure that you have this element, so you can simply do:

  TimeHeaders.update(li.querySelector('time'));

without any if.

::: apps/sms/js/time_headers.js
@@ +28,3 @@
>        }).bind(this);
>  
> +      updateFunction(false);

I don't find this very readable. Maybe instead we should call `updateAll()` (with the changes that I explain below) in `onVisibilityChange`, and change here like this:

    startScheduler: function th_startScheduler() {
      var updateFunction = (function() {
        var now = Date.now(),
            nextTimeout = new Date(now + 60000);
        nextTimeout.setSeconds(0);
        nextTimeout.setMilliseconds(0);

        // stop updateTimer first
        this.stopScheduler();

        // then register a new one
        updateTimer = setTimeout(function() {
          this.updateAllRepeat();
          updateFunction();
        }.bind(this), nextTimeout.getTime() - now);
      }).bind(this);

      updateFunction();
    },

To remove some of these .bind(this), maybe we can move the updateFunction as a TimeHeaders property?

@@ +31,5 @@
>      },
>      stopScheduler: function th_stopScheduler() {
>        clearTimeout(updateTimer);
>      },
> +    updateAll: function th_updateAll(repeat) {

So, in my idea, here is what you should do:

* instead of taking a boolean "repeat" argument, take a string "selector" argument
* in updateAll, do at the start: `selector = selector || "[data-time-update]"`. This way, the default 
* when you have now "updateAll(true)" use `updateAll("header[data-time-update=repeat]")`
* when you have now "updateAll(false)", use instead `updateAll()` (without argument)


If you prefer, you can make 1 simple method that will call updateAll: "updateAllRepeat", that will call `updateAll("header[data-time-update=repeat]")`. In the future, I think that all existing calls to updateAll should use a narrower selector, but this is in my opinion for another bug if this proves to be a problem.

@@ +50,5 @@
> +
> +        for (j = 0; j < threadlength; j++) {
> +          this.update(threadlist[j]);
> +        }
> +      }

and then you don't need this ;)

::: apps/sms/test/unit/thread_list_ui_test.js
@@ +375,5 @@
>  
>    suite('createThread', function() {
>      setup(function() {
>        this.sinon.spy(Template, 'escape');
> +      this.sinon.spy(MockTimeHeaders, 'update');

nit: for jshint, you need to add this object to the /*global line at the start of the file

Comment 18

[at-kitamura]

Attachment: Bugzilla917639.patch

Thanks for your comments!

Because I thought that I didn't have any problem at your suggestions, I modified with almost all.
I uploaded a patch again.

Please check it.

Comment 19

[Julien Wajsberg [:julienw]]

Comment on attachment 8338363 [details] [diff] [review]
Bugzilla917639.patch

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

Hey Kitamura-san,

Thanks for your patch!

Some small changes needed, and then we're ready.

Can you please open a github pull request so that all unit tests and checks are run ?

Thanks!

::: apps/sms/js/thread_list_ui.js
@@ +337,5 @@
>          delete thlui_renderThreads.abort;
>          // set the fixed header
>          FixedHeader.refresh();
>          // Boot update of headers
> +        TimeHeaders.updateAll('header[data-time-update=repeat]');

nit: use 'header[data-time-update]' as selector (shorter, and more logical here, as the update we want to do has nothing in common with the repeating condition)

::: apps/sms/js/thread_ui.js
@@ +484,5 @@
>  
>    onMessageReceived: function thui_onMessageReceived(message) {
>      this.appendMessage(message);
>      this.scrollViewToBottom();
> +    TimeHeaders.updateAll('header[data-time-update=repeat]');

nit: same here: use 'header[data-time-update]' as selector, it's enough!

@@ +1174,5 @@
>    showFirstChunk: function thui_showFirstChunk() {
>      // Show chunk of messages
>      ThreadUI.showChunkOfMessages(this.CHUNK_SIZE);
>      // Boot update of headers
> +    TimeHeaders.updateAll('header[data-time-update=repeat]');

nit: same here: use 'header[data-time-update]' as selector

::: apps/sms/js/time_headers.js
@@ +9,5 @@
>        onvisibilityChange();
>        document.addEventListener('visibilitychange', onvisibilityChange);
>      },
>      startScheduler: function th_startScheduler() {
> +      this.updateFunction();

thanks to this change, I now understand that we can just put what's inside `updateFunction` in `startScheduler`. We don't need a separate function after all. Sorry for the mess...

Comment 20

[at-kitamura]

Hi Julien-san,

I have done Pull Request. Please check it.

https://github.com/mozilla-b2g/gaia/pull/14169

Comment 21

[at-kitamura]

Hi Julien-san
 
> https://github.com/mozilla-b2g/gaia/pull/14169

Though the Travis is not carried out, do you know why it is not carried out?
(I think that it was automatically carried out before.)

Please tell me if you understand it.

Comment 22

[Julien Wajsberg [:julienw]]

Hey,

I have no clue why the PR has not started.

You'll need to rebase because some code checked in yesterday conflict with your code. So hopefully once you'll push the rebase it will trigger a Travis build.

Also, is your pull request up to date? The changes in startScheduler are not like I thought: we just need to take the updateFunction in your previous patch, and make it replace startScheduler (because startScheduler is just calling updateFunction), instead of what you did now: move it _inside_ startScheduler. Is it clear enough? Please tell me if this is not clear!

Thanks!

Comment 23

[at-kitamura]

Hi Julien-san

My apologies for the late reply.

I remade Pull Request(rebase and corrected the places that were pointed out).
Probably I think that I corrected that in accordance to your guidance.

In addition, I closed "https://github.com/mozilla-b2g/gaia/pull/14169" and reopened as "https://github.com/mozilla-b2g/gaia/pull/14255".

Please check "https://github.com/mozilla-b2g/gaia/pull/14255".

Comment 24

[Julien Wajsberg [:julienw]]

Attachment: github PR

Comment 25

[Julien Wajsberg [:julienw]]

Comment on attachment 8341600 [details] [review]
github PR

This is great work!

r=me, I'll fix the 2 jshint issues left before merging

Comment 26

[Julien Wajsberg [:julienw]]

(the jshint issues came from a previous patch)

master: 833ef17fb23f8f39d8217757410ad4e900a4cf8a

Comment 27

[Rick Waldron [:rwaldron]]

jshint fixes: https://github.com/mozilla-b2g/gaia/commit/aaacceb6453973e3a0e4868e95e605727ce9ba39