Bug 918970 - display existing thread when sending a sms from contact app #24886
Conversation
|
|
Augustin Trancart (autra)
|
|
|
|
| .catch(function() { | ||
| ActivityHandler.toView(viewInfo); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
I think you can rewrite this like this (I remove the "this" and object names so that it's shorter to write and read :) ):
return findThreadFromNumber(viewInfo.number).then(
function onresolve(threadId) {
viewInfo.threadId = threadId;
},
function onreject() {
// we return the result so that we wait that it's finished before going on
return findContactByNumber(viewInfo.number)
.then((contact) => viewInfo.contact = contact);
}
)
.catch(() => {}) // gobble errors
.then(() => {
return this.toView(viewInfo); // final action
});
|
|
Augustin Trancart (autra)
|
|
| } | ||
| deferred.reject('No contact found with number: ' + number); |
There was a problem hiding this comment.
I've read somewhere (at the end of the paragraph) that I should reject with a real error, not a string. What do you think about that? Do we care about that?
Related to this blog, does Promise.reject return in Firefox? The author is mentioning that some library does not return when you reject, which means that if you have a resolve later in the code, the promise could end up being fullfilled. For me this behaviour breaks the specs of Promise, but better ask confirmation :-)
There was a problem hiding this comment.
To answer your questions:
- yes, it's always better to reject with a real Error than with a string. This has the added useful property to have a stack :) so we know where the Error comes from when (for example) we display it
.rejectdoes not return; it's a normal function call. so you need to reject and then return. However you can never change a promise state nor value. Once it's been rejected, you can't resolve it, and vice versa.
I've made a jsbin to show all this: http://jsbin.com/qobacicijo/1/edit
The first case is interesting: I actually didn't really know what it meant to throw from inside the Promise constructor's callback :) Now I know it's the same than throwing.
There is something I dislike in Remy Sharp's example: if compare is asynchronous, throwing in the callback won't throw in the Promise constructor's callback. In the end, the promise will never be fulfilled, and there is no way to catch the exception in this case.
There was a problem hiding this comment.
so yeah, in the end, it's better to reject with an error here :)
|
|
|
|
| @@ -292,7 +323,7 @@ var ActivityHandler = { | |||
| return; | |||
| } | |||
|
|
|||
| Navigation.toPanel('thread', { id: threadId }); | |||
| Navigation.toPanel('thread', { id: threadId }).then(Compose.focus); | |||
There was a problem hiding this comment.
I've put this in another commit, but I can squash if you want.
I did not add a test. Is it necessary according to you?
There was a problem hiding this comment.
yep a test could be nice :)
|
I don't know why but gaia-try-hook didn't run on my last push. |
| }).then(done,done); | ||
| }); | ||
|
|
||
| test('when there is an existing thread, Composer should be focused', |
There was a problem hiding this comment.
I copy my comment on the commit. Don't know why it didn't happen here as well :
So for this test, I relied on stubbing focus (which is mocked anyway) because Navigation.toPanel is called before Compose.focus. So spying toPanel was not enough to wait for focus to be called.
The main drawback of this solution is that is case Compose.focus is not called, the test fails because of the timeout, which is not ideal. Do you have a better idea? I didn't see anything to spy on to make things better.
Sometimes it doesn't when some conditions are not met (http://ftp.mozilla.org/pub/mozilla.org/b2g/tinderbox-builds/mozilla-central-linux64_gecko/latest/ does not have a build, or gaia is closed) |
|
|
|
landed in bd4dcc8 |
old PR : #16216