Closed Bug 958105 Opened 6 years ago Closed 4 years ago
[Messages][Drafts] Creating a draft should not change the thread's timestamp
46 bytes, text/x-github-pull-request
|Details | Review|
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)
Attachment #8357895 - Flags: feedback?(evelyn) → feedback+
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 on attachment 8357895 [details] [review] https://github.com/mozilla-b2g/gaia/pull/15165 updates per review
Comment on attachment 8357895 [details] [review] https://github.com/mozilla-b2g/gaia/pull/15165 More comments and questions on github
Comment on attachment 8357895 [details] [review] https://github.com/mozilla-b2g/gaia/pull/15165 Questions addressed, patch updated and rebased onto master
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 on attachment 8357895 [details] [review] https://github.com/mozilla-b2g/gaia/pull/15165 (removing the review flag for now)
(can you add the review flag back if it's ready to review again ?)
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 on attachment 8357895 [details] [review] https://github.com/mozilla-b2g/gaia/pull/15165 Updated
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 :)
Attachment #8357895 - Flags: review?(felash) → review+
Taking to see if the bug still happens, and if yes, to land this.
Assignee: waldron.rick → felash
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.
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 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 :)
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.
(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.
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?
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!
ok, so let's finish Steve's patch from bug 1084298, and completely change how the drafts timestamp handling :)
Summary: [Messages][Drafts] Restore thread timestamp with last message timestamp when draft is discarded → [Messages][Drafts] Creating a draft should not change the thread's timestamp
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.
Attachment #8630391 - Flags: review?(felash)
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 :)
Attachment #8630391 - Flags: review?(felash) → review+
Thanks! In master: https://github.com/mozilla-b2g/gaia/commit/a07e8311628463178cf6f14d45206d3ada9b2015
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Whiteboard: [sms-most-wanted] → [sms-most-wanted][sms-sprint-FxOS-S3]
You need to log in before you can comment on or make changes to this bug.