This is Bug 833010 once again. STR: * open Sms app, open a thread * trigger "edit mode" * receive a SMS Expected: * the new SMS is displayed in the thread Actual: * the new SMS is not displayed. It's not even displayed when the user quits the edit mode This is a regression, adding qawanted to know if this happens in v1.0.1 and v1-train.

Note that there is a notification added too, whereas it should not.

I was not able to reproduce this issue on the latest v1 and v1.0.1 builds. All SMS arrived while I was in Edit mode, and Message threads were updated with sms received. Unagi, Build ID: 20130520070207 Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/321d5b4db55a Gaia: b161f42c5647b37e35ba5a20f394ba40bf674d9c Unagi, Build ID: 20130520070208 Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/dbecb0c45504 Gaia: ee70d7517689116622c5125ce33e56d46dd3c948

Thanks ! Therefore this is probably a regression from the MMS patches.

I've diagnosed the problem but I still have to find the change that caused it: when in edit mode it seems that the |Threads.currentId| property is set to |null| this causes this test to fail: https://github.com/mozilla-b2g/gaia/blob/37d895322819acff82f131452d5d0950d1cdb001/apps/sms/js/activity_handler.js#L253 In turn a regular notification ends up being dispatched instead of updating the visible thread.

OK, turns out the threadId patch caused this: https://github.com/mozilla-b2g/gaia/commit/2f444eefd99078cb782e227e8d16c4964c99b1f1 This encodes the current thread in the hash part of the URL but the different panels within the app are implemented by using the hash part too (e.g. #edit for the edit mode). So when in edit mode Threads.currentId() returns |null| because the URL ends in #edit instead of #thread<n> as it would expect.

I knew it was too clever ;) Gabriele, do you feel like you could fix this or should it be handled to someone else ?

I'm studying the code and I'll try to come up with a fix; it shouldn't be too hard. My only gripe is what I have in mind is not going to look pretty (hint: #editthread23) ;)

Pointer to Github pull-request

Comment on attachment 753805 [details] Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9989 This patch encodes the thread ID in the URL when in thread edit mode in a way that follows the existing logic as closely as possible. The end result is that in list edit mode the location hash will be '#edit', in the thread view '#thread=<n>' and in thread edit mode '#editthread=<n>'. This required a few small changes to the application logic: code that dealt with edit mode now checks if the beginning of the location hash matches '#edit' instead of the whole string and reacts accordingly; similarly thread-id logic now extracts the id from the trailing thread=<n> string instead of the full #thread=<n> form. Finally this patch fixes another issue: when leaving the app in thread-edit mode and receiving a message, tapping on the notification would drop the edit mode and not show the message even if it was in the same thread. With this patch applied the new message will show up correctly in the view.

Added a comment on the PR. Maybe we can wait that Bug 870069 lands, because the patch for that bug brings in something we could use here.

Dietrich, this is a regression due to MMS work, could you please block on this ?

Pointer to Github pull-request

Comment on attachment 755375 [details] Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/10078 Here's an alternate fix for the bug that does not use the location hash, instead the edit mode is handled internally by the ThreadUI object.

Comment on attachment 755375 [details] Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/10078 feedback+ let's go !

Comment on attachment 753805 [details] Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9989 Obsoleting, we'll go for the other solution.

Comment on attachment 755375 [details] Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/10078 If you're OK with this version r+ it and merge at will :)

Comment on attachment 755375 [details] Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/10078 ah no, sorry, it's feedback+ because I like the approach, but r- because it doesn't work ;) I've commented on github, please have a look and we can discuss about this tomorrow !

Comment on attachment 755375 [details] Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/10078 Updated pull-request as per our discussion on GitHub: - the #edit location hash has been removed entirely - both the thread and list UIs now handle the edit mode internally - related parts of the code have been cleaned up to leverage the new edit mode

Comment on attachment 755375 [details] Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/10078 r- for now, please r? me again when you have made the follow up changes. There are comments on the pull request https://github.com/mozilla-b2g/gaia/pull/10078

Just a quick update on the timeline, I'm at a workshop doing presentations today but I'll work on this tonight so I should have an updated patch ready by tomorrow morning.

Thanks for the update!

Comment on attachment 755375 [details] Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/10078 Here's the updated pull request. I've addressed the issues with the previous one, factorized all the code for entering/exiting the edit mode and fixed the unit tests. Unfortunately I had little time to test it on my device so I'll do that tomorrow morning.

Comment on attachment 755375 [details] Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/10078 r=me - thanks!

master: https://github.com/mozilla-b2g/gaia/commit/bd9c7ed5e3d07c36312e8f7d0d076d28261a8d28

Unfortunately my patch broke certain out-of-edit-mode transitions (like when we tap a message in the notification panel). This is actually what I feared when I prepared the alternate patch for this problem. I'll try to develop a fix today but going forward we should address the issue of state changes in a more consistent manner to prevent this kind of problems from creeping in once more.

Pointer to Github pull-request

Comment on attachment 757907 [details] Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/10183 Here's another PR that fixes the regression introduced by the previous one. I'm not exactly a fan of this approach which is the reason why I had come up with attachment 753805 [details] first; however at this stage there's no better fix available. Going forward I think we should move the state transition logic into some centralized place to avoid this kind of issues were we have to duplicate state-transitioning code across all paths (and risk breaking it somewhere and making the state inconsistent).

gsvelto - can you open a new bug for this regression/pull, just to make uplifting easier? I'd hate to land a bug via two commits and have one of them get lost in the uplift? I've tagged this with no uplift until that bug is fixed. I know julien asked me to do the same for another bug earlier, which is why I'm asking now. I left some comments on the pull request also.

(In reply to Corey Frang [:gnarf] [:gnarf37] from comment #29) > gsvelto - can you open a new bug for this regression/pull, just to make > uplifting easier? I'd hate to land a bug via two commits and have one of > them get lost in the uplift? I've tagged this with no uplift until that bug > is fixed. I know julien asked me to do the same for another bug earlier, > which is why I'm asking now. I've created bug 879291 as a clone of this bug, I'll move all the activity there. > I left some comments on the pull request also. Thanks, I'll address them ASAP.

@Gabriele Svelto, Hi, I have tested the patch that you have provided for Notification related Bug.Even with the patch i am able to reproduce the issue. STR 1.Receive 2-3 messages. 2.Click on the message from the SMS notification 3.When we click on the SMS notification of the received message,the New message screen is displayed with the received body in the Message field and the To field is empty. Note: When we click on the Notification message which is already deleted,we are getting a notification "The message has already deleted"(This scenario works fine).

uplifted master: bd9c7ed5e3d07c36312e8f7d0d076d28261a8d28 v1-train: d0f383776b7a1aeb13057bcf9fb9ed025d14b4c6

Created test case in MozTrap: https://moztrap.mozilla.org/manage/case/8593/ Also executed existing test case: https://moztrap.mozilla.org/manage/case/4979/ Verified fixed on: Device: Leo phone Build Identifier: 20130611074722 Update channel: leo/1.1.0/nightly Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/8d0562d20324 Gaia: 8c64e19b82d4e0490a7780232d3d6bd07d0ba9ec1370937291 Git commit info: 2013-06-11 07:54:51 OS version: 1.1.0.0-prerelease and Device: Unagi phone Build Identifier: 20130611074722 Update channel: unagi/1.1.0/beta Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/8d0562d20324 Gaia: 8c64e19b82d4e0490a7780232d3d6bd07d0ba9ec1370937291 Git commit info: 2013-06-11 07:54:51 OS version: 1.1.0.0-prerelease