958105 - [Messages][Drafts] Creating a draft should not change the thread's timestamp

Status: RESOLVED FIXED

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

Description

[Rick Waldron [:rwaldron]]

If a draft is discarded, the in-memory thread object and associated DOM node  timestamp should be restored to the last message's timestamp. This will return the thread to the correct chronological place in the thread list (by message)

Comment 1

[Rick Waldron [:rwaldron]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/15165

Comment 3

[Julien Wajsberg [:julienw]]

Comment on attachment 8357895 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/15165

comments on github

please request review again once it's ready :)

Comment 4

[Rick Waldron [:rwaldron]]

Comment on attachment 8357895 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/15165

updates per review

Comment 5

[Julien Wajsberg [:julienw]]

Comment on attachment 8357895 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/15165

More comments and questions on github

Comment 6

[Rick Waldron [:rwaldron]]

Comment on attachment 8357895 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/15165

Questions addressed, patch updated and rebased onto master

Comment 7

[Julien Wajsberg [:julienw]]

Comment on attachment 8357895 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/15165

Some more questions.

I really think we need to compare the timestamps to get the greatest one.

Comment 8

[Julien Wajsberg [:julienw]]

Comment on attachment 8357895 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/15165

(removing the review flag for now)

Comment 9

[Julien Wajsberg [:julienw]]

(can you add the review flag back if it's ready to review again ?)

Comment 10

[Julien Wajsberg [:julienw]]

Comment on attachment 8357895 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/15165

some more comments on github.

I think there is no way to cleanly find the last message timestamp when removing a message without refactoring how messages are displayed. This is something I'd eventually want to do, but since this is not the goal for this bug, I think we should move on using an uglier solution.

Comment 11

[Rick Waldron [:rwaldron]]

Comment on attachment 8357895 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/15165

Updated

Comment 12

[Julien Wajsberg [:julienw]]

Comment on attachment 8357895 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/15165

r=me with the last comment

we need to fix the timestamp handling, and to do this, we need to keep the messages array in the thread object always sorted. Please file a bug for this and add the bug number in a comment.

Thanks, let's land this now :)

Comment 13

[Julien Wajsberg [:julienw]]

Taking to see if the bug still happens, and if yes, to land this.

Comment 14

[Julien Wajsberg [:julienw]]

The bug is still here, but the patch does not apply cleanly, which is quite expected.

I'll see if we can do comment 12 before landing this.

Comment 15

[Julien Wajsberg [:julienw]]

Attachment: github PR

unbitrotted the patch.

I did some basic checks and it seems to work correctly, and fix the initial issue.

Steve, I think this implemented part of the "model in threads.js" I outlined in bug 1084298. So it would be nice that you have a look at it. We discussed it a lot with Rick when he worked on it last January, but it never landed.

One part was not very nice: the part where we try to find the latest timestamp. I think it can break in some situations and I asked Rick to file a separate bug, but he left before doing it. If you still agree with this approach, I can file this separate bug, add a comment in the patch about it, and we can continue from here.

So what do you think?

Comment 16

[Steve Chung [:steveck]]

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

I left some thought in github, I think the basic idea is the same as in bug 1084298, maybe it's good chance to brainstorm for better message caching mechanism :)

Comment 17

[Julien Wajsberg [:julienw]]

Hey Oleg,

I'll be glad if you can help finishing this work with Steve. We mainly need to merge this patch with the patch in bug 1084298, see the differences and decide of the best approach.

In the end, one of the bugs will have a refactoring patch, and the other will have a simple patch.

Comment 18

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] from comment #17)
> Hey Oleg,
> 
> I'll be glad if you can help finishing this work with Steve. We mainly need
> to merge this patch with the patch in bug 1084298, see the differences and
> decide of the best approach.
> 
> In the end, one of the bugs will have a refactoring patch, and the other
> will have a simple patch.

Sure, I've briefly scanned through the patch in this bug and think that we can proceed first with the Steve's patch in bug 1084298 and then use this one to actually fix timestamp issue + some required refactoring.

Keeping ni=me for now.

Comment 19

[Julien Wajsberg [:julienw]]

Hey Jenny,

the more I think of this, the more I think we should not change the thread timestamp, which would fix this bug :)

Let me explain what happens now so that you have some context.

1. you open a thread that is not the most recent thread
2. you enter some text, press back, save the draft
3. now the thread is moved to the top. In the code, we change the thread's timestamp because we sort by timestamp.
4. we enter the thread again, change some text, press back, discard the draft
5. the thread is not moved until we restart the application
=> this is this bug.

I think that at step 3 we should not move the thread to the top because it feels weird. I think the original logic was that it was easier to find it back. Now I argue that I don't really care to find it back.

What do you think?

Comment 20

[Jenny Lee]

Hi Julien,

I agree with you. If user creates a draft and leaves it long enough to forget where is the thread roughly located in inbox, it probably means the user doesn't really care about it anyway; if the draft is important enough, it shouldn't be too hard to find since we have draft indicator, hence no point moving it up. This is also how iOS handle the particular situation you described. So let's do as you proposed here!
Thanks!

Comment 21

[Julien Wajsberg [:julienw]]

ok, so let's finish Steve's patch from bug 1084298, and completely change how the drafts timestamp handling :)

Comment 22

[GitHub Autolander Bot]

Attachment: [gaia] steveck-chung:message-ignore-draft-timestamp > mozilla-b2g:master

Comment 23

[Steve Chung [:steveck]]

Comment on attachment 8630391 [details] [review]
[gaia] steveck-chung:message-ignore-draft-timestamp > mozilla-b2g:master

Hi Julien, I made a small change that remove the draft timestamp in inbox view. But I'm not sure whether we should assert that "we didn't access the draft timestamp" because it's just not exist in the code.

Comment 24

[Julien Wajsberg [:julienw]]

Comment on attachment 8630391 [details] [review]
[gaia] steveck-chung:message-ignore-draft-timestamp > mozilla-b2g:master

Just one nit, but otherwise it's fine :)

r=me, please take care gaia try is green :)

Comment 25

[Steve Chung [:steveck]]

Thanks!
In master: https://github.com/mozilla-b2g/gaia/commit/a07e8311628463178cf6f14d45206d3ada9b2015