Skip to content
This repository has been archived by the owner on Nov 3, 2021. It is now read-only.

Bug 851032 - [SMS] Receiving a new message it's not properly threaded r=julienw #8716

Closed
wants to merge 7 commits into from
Closed

Bug 851032 - [SMS] Receiving a new message it's not properly threaded r=julienw #8716

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7e6f7d4
Bug 851032 - [SMS] Receiving a new message it's not properly threaded…
borjasalguero Mar 19, 2013
a9d3907
Julien suggestion about condition in IF clause
borjasalguero Mar 27, 2013
86a0ac7
Comment fixed
borjasalguero Mar 27, 2013
7f05a32
strict mode, linting and condition fixed
borjasalguero Mar 27, 2013
83c4bec
Removing self after rebase
borjasalguero Mar 27, 2013
91b893b
Moving to updateHeaderData
borjasalguero Mar 27, 2013
440d99d
Title fixed
borjasalguero Apr 2, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 1 addition & 2 deletions apps/sms/js/activity_handler.js
View file
Open in desktop
Expand Up @@ -104,9 +104,8 @@ if (!window.location.hash.length) {

// The SMS app is already displayed
if (!document.mozHidden) {
var currentThread = MessageManager.currentNum;
// If we are in the same thread, only we need to vibrate
if (number == currentThread) {
if (message.threadId === MessageManager.currentThread) {
navigator.vibrate([200, 200, 200]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say we could do the vibration in our MessageManager.onMessageReceived instead.

but since this is a tef+ bug let's keep that here now.

return;
}
Expand Down
31 changes: 16 additions & 15 deletions apps/sms/js/message_manager.js
View file
Open in desktop
Expand Up @@ -32,6 +32,10 @@ var MessageManager = {
var message = e.message;
var num = message.receiver;
if (window.location.hash == '#new') {
// We update the currentThread
if (!MessageManager.currentThread) {
MessageManager.currentThread = message.threadId;
}
// If we are in 'new' we go to the right thread
// 'num' has been internationalized by Gecko
window.location.hash = '#num=' + num;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we still use this hash ? #thread=<id> would be better now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently there is no getMessagesById() in Gecko. Our navigation paradigm it's based on the number (will change once Gecko let us to get all messages by thread_id), so we have to keep it (at least for v1.0.1)

Expand All @@ -56,6 +60,7 @@ var MessageManager = {
// thread without requesting Gecko, so we increase the performance and we
// reduce Gecko requests.
return {
id: message.threadId,
senderOrReceiver: message.sender,
body: message.body,
timestamp: message.timestamp,
Expand All @@ -66,14 +71,10 @@ var MessageManager = {
onMessageReceived: function mm_onMessageReceived(e) {
var message = e.message;

var num;
if (this.currentNum) {
num = this.currentNum;
}

var sender = message.sender;
if (num && num === sender) {
//Append message and mark as unread
var currentThread = this.currentThread;
var threadId = message.threadId;
if (currentThread != null && currentThread === threadId) {
// Append message and mark as read
this.markMessagesRead([message.id], true, function() {
MessageManager.getThreads(ThreadListUI.renderThreads);
});
Expand Down Expand Up @@ -120,8 +121,8 @@ var MessageManager = {
onVisibilityChange: function mm_onVisibilityChange(e) {
LinkActionHandler.resetActivityInProgress();
ThreadListUI.updateContactsInfo();
ThreadUI.updateHeaderData();
Utils.updateTimeHeaders();
ThreadUI.updateHeaderData();
},

slide: function mm_slide(callback) {
Expand Down Expand Up @@ -164,7 +165,7 @@ var MessageManager = {
contactButton.parentNode.appendChild(contactButton);
document.getElementById('messages-container').innerHTML = '';
ThreadUI.cleanFields();
MessageManager.currentNum = null;
MessageManager.currentThread = null;
threadMessages.classList.add('new');
MessageManager.slide(function() {
receiverInput.focus();
Expand All @@ -174,7 +175,7 @@ var MessageManager = {
//Keep the visible button the :last-child
var editButton = document.getElementById('icon-edit');
editButton.parentNode.appendChild(editButton);
MessageManager.currentNum = null;
MessageManager.currentThread = null;
if (mainWrapper.classList.contains('edit')) {
mainWrapper.classList.remove('edit');
if (ThreadListUI.editDone) {
Expand Down Expand Up @@ -211,14 +212,13 @@ var MessageManager = {
var num = this.getNumFromHash();
if (num) {
var filter = this.createFilter(num);
var input = document.getElementById('messages-input');
MessageManager.currentNum = num;

if (mainWrapper.classList.contains('edit')) {
mainWrapper.classList.remove('edit');
} else if (threadMessages.classList.contains('new')) {
ThreadUI.updateHeaderData();
ThreadUI.renderMessages(filter);
threadMessages.classList.remove('new');
ThreadUI.updateHeaderData();
} else {
// As soon as we click in the thread, we visually mark it
// as read.
Expand All @@ -227,7 +227,8 @@ var MessageManager = {
threadRead.getElementsByTagName('a')[0].classList
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I hate github for this: I'd like to comment the context that is before that)

Why not using the thread id instead of the number as the thread HTML id ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as before. Currently the whole model and API it's based on the number. This is changing, but currently we have to keep it because it's more relevant the phone number than ID by now.

.remove('unread');
}

// Update the currentThread
MessageManager.currentThread = threadRead.dataset.threadId;
var self = this;
// Update Header
ThreadUI.updateHeaderData(function headerReady() {
Expand Down
19 changes: 19 additions & 0 deletions apps/sms/js/sms_mock.js
View file
Open in desktop
Expand Up @@ -3,6 +3,8 @@
Code below is for desktop testing!

*********************************************************** */
'use strict';

(function(window) {

var MockNavigatormozSms = window.MockNavigatormozSms = {};
Expand All @@ -12,6 +14,7 @@
id: 0,
messages: [
{
threadId: 1,
sender: null,
receiver: '1977',
body: 'Alo, how are you today, my friend? :)',
Expand All @@ -20,6 +23,7 @@
timestamp: new Date(Date.now())
},
{
threadId: 1,
sender: null,
receiver: '1977',
body: 'arr :)',
Expand All @@ -28,20 +32,23 @@
timestamp: new Date(Date.now() - 8400000000)
},
{
threadId: 2,
sender: null,
receiver: '436797',
body: 'Sending :)',
delivery: 'sending',
timestamp: new Date(Date.now() - 172800000)
},
{
threadId: 3,
sender: null,
receiver: '197743697',
body: 'Nothing :)',
delivery: 'sent',
timestamp: new Date(Date.now() - 652800000)
},
{
threadId: 4,
sender: null,
receiver: '197746797',
body: 'Error message:)',
Expand All @@ -50,20 +57,23 @@
timestamp: new Date(Date.now() - 822800000)
},
{
threadId: 4,
sender: null,
receiver: '197746797',
body: 'Nothing :)',
delivery: 'sent',
timestamp: new Date(Date.now() - 1002800000)
},
{
threadId: 4,
sender: null,
receiver: '197746797',
body: 'Nothing :)',
delivery: 'error',
timestamp: new Date(Date.now() - 1002800000)
},
{
threadId: 4,
sender: '197746797',
body: 'Recibido!',
delivery: 'received',
Expand All @@ -72,30 +82,35 @@
],
threads: [
{
id: 1,
senderOrReceiver: '1977',
body: 'Alo, how are you today, my friend? :)',
timestamp: new Date(Date.now()),
unreadCount: 0
},
{
id: 2,
senderOrReceiver: '436797',
body: 'Sending :)',
timestamp: new Date(Date.now() - 172800000),
unreadCount: 0
},
{
id: 3,
senderOrReceiver: '197743697',
body: 'Nothing :)',
timestamp: new Date(Date.now() - 652800000),
unreadCount: 0
},
{
id: 4,
senderOrReceiver: '197746797',
body: 'Recibido!',
timestamp: new Date(Date.now() - 50000000),
unreadCount: 0
},
{
id: 5,
senderOrReceiver: '14886783487',
body: 'Hello world!',
timestamp: new Date(Date.now() - 60000000),
Expand All @@ -112,6 +127,7 @@
// Procedurally generate a large amount of messages for a single thread
for (var i = 0; i < 150; i++) {
messagesDb.messages.push({
threadId: 5,
sender: '14886783487',
body: 'Hello world!',
delivery: 'received',
Expand Down Expand Up @@ -169,13 +185,15 @@
};

MockNavigatormozSms.send = function(number, text, success, error) {
var threadId = MessageManager.currentThread;
var sendId = messagesDb.id++;
var request = {
error: null
};
var sendInfo = {
type: 'sent',
message: {
threadId: threadId,
sender: null,
receiver: number,
delivery: 'sending',
Expand Down Expand Up @@ -220,6 +238,7 @@
var receivedInfo = {
type: 'received',
message: {
threadId: threadId,
sender: number,
receiver: null,
delivery: 'received',
Expand Down
1 change: 1 addition & 0 deletions apps/sms/js/thread_list_ui.js
View file
Open in desktop
Expand Up @@ -269,6 +269,7 @@ var ThreadListUI = {
var threadDOM = document.createElement('li');
threadDOM.id = 'thread_' + num;
threadDOM.dataset.time = timestamp;
threadDOM.dataset.threadId = thread.id;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's put the thread_id in threadDOM.id, right ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and you can put the phoneNumber in the dataset instead. Which really makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change will come when the paradigm will be complete, and we have getMessages (by THREAD ID) or markThreadAsRead (by Thread ID). THis bug it's for fixing a v1.0.1 issue (the change will be in v1-train & master as well), but we could create bugs for changing the behaviour in master in the future (actually there are some bugs already created, related with adding cursor to getThreads...etc).


// Retrieving params from thread
var bodyText = (thread.body || '').split('\n')[0];
Expand Down
26 changes: 12 additions & 14 deletions apps/sms/js/thread_ui.js
View file
Open in desktop
Expand Up @@ -372,6 +372,15 @@ var ThreadUI = {
},
// Method for updating the header with the info retrieved from Contacts API
updateHeaderData: function thui_updateHeaderData(callback) {
if (MessageManager.currentThread === null) {
return;
}
// Retrieve phone number from Hash
var number = MessageManager.getNumFromHash();
// Add data to contact activity interaction
this.headerText.dataset.phoneNumber = number;
this.headerText.textContent = number;

// For Desktop Testing, mozContacts it's mockuped but it's not working
// completely. So in the case of Desktop testing we are going to execute
// the callback directly in order to make it works!
Expand All @@ -381,13 +390,6 @@ var ThreadUI = {
return;
}

var number = MessageManager.currentNum;
if (!number) {
return;
}

// Add data to contact activity interaction
this.headerText.dataset.phoneNumber = number;

Contacts.findByString(number, function gotContact(contacts) {
var carrierTag = document.getElementById('contact-carrier');
Expand Down Expand Up @@ -778,8 +780,8 @@ var ThreadUI = {
this.noResults.classList.add('hide');
this.container.classList.remove('hide');

if (resendText && typeof resendText === 'string') {
num = MessageManager.currentNum;
if (resendText && (typeof(resendText) === 'string') && resendText !== '') {
num = MessageManager.getNumFromHash();
text = resendText;
} else {
// Retrieve num depending on hash
Expand All @@ -791,7 +793,7 @@ var ThreadUI = {
return;
}
} else {
num = MessageManager.currentNum;
num = MessageManager.getNumFromHash();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idem

}

// Retrieve text
Expand All @@ -802,10 +804,6 @@ var ThreadUI = {
}
// Clean fields (this lock any repeated click in 'send' button)
this.cleanFields(true);
// Remove when
// https://bugzilla.mozilla.org/show_bug.cgi?id=825604 landed
MessageManager.currentNum = num;
this.updateHeaderData();
// Send the SMS
MessageManager.send(num, text);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you sure this is not necessary ? bug 825604 hasn't landed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed MessageManager.currentThread & updateHeaderData consequently for getting the same behaviour. Remember that ThreadID it's not available when clicking 'sendMessage' from 'new'.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes but we do have the number here (because we're sending to it) so maybe we could pass it directly to updateHeaderData as a parameter ?

I'll see how it feels as soon as I'll have a working build, maybe it is good enough like that

},
Expand Down