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

Avoid rendering messages with duplicate IDs #387

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Avoid rendering messages with duplicate IDs #387

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 17 additions & 2 deletions src/view/pane/message.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,20 @@ Candy.View.Pane = (function(self, $) {
timestamp = Candy.Util.iso8601toDate(timestamp);
}

var messagePane = self.Room.getPane(roomJid, ".message-pane");
Copy link
Member

Choose a reason for hiding this comment

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

can we rely on the fact that each client behaves like it should & generates a unique id for the message, also that not two clients generate the same id for different messages?

Copy link
Member Author

Choose a reason for hiding this comment

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

We cleanly handle the case where a message does not have an ID, but there's no way to implement this for clients which generate non-unique IDs. I'll post some more cogent thoughts once I'm really awake.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the best we could do here is to make this behaviour optional and enable it when all clients are controlled or meet the requirements for globally unique IDs.

Copy link
Member

Choose a reason for hiding this comment

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

yes, I'd go for this option as well. Another solution would be to generate when receiving a unique id based on message body, from jid and date.. but this might generate false duplicates :/

var messageId;
if (stanza) {
messageId = $(stanza).attr('id');
if (messageId) {
var existingMessage = messagePane.find('[data-message-id="' + messageId + '"]');
if (messageId && existingMessage.length > 0) {
// This message ID has already been rendered
return false;
}
}
}

// Before we add the new message, check to see if we should be automatically scrolling or not.
var messagePane = self.Room.getPane(roomJid, '.message-pane');
var enableScroll = ((messagePane.scrollTop() + messagePane.outerHeight()) === messagePane.prop('scrollHeight')) || !$(messagePane).is(':visible');
Candy.View.Pane.Chat.rooms[roomJid].enableScroll = enableScroll;

Expand Down Expand Up @@ -158,7 +170,8 @@ Candy.View.Pane = (function(self, $) {
time: Candy.Util.localizedTime(timestamp),
timestamp: timestamp.toISOString(),
roomjid: roomJid,
from: from
from: from,
id: messageId
},
stanza: stanza
};
Expand Down Expand Up @@ -246,6 +259,8 @@ Candy.View.Pane = (function(self, $) {
* (String) message - Message text
*/
$(Candy).triggerHandler('candy:view.message.after-show', evtData);

return true;
}
};

Expand Down
2 changes: 1 addition & 1 deletion src/view/template.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ Candy.View.Template = (function(self){

self.Message = {
pane: '<div class="message-pane-wrapper"><ul class="message-pane"></ul></div>',
item: '<li><small data-timestamp="{{timestamp}}">{{time}}</small><div>' +
item: '<li data-message-id="{{id}}"><small data-timestamp="{{timestamp}}">{{time}}</small><div>' +
Copy link
Member

Choose a reason for hiding this comment

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

why not just id? might be used for other cases

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to avoid potential colision with any reserved name. Happy to change it if you're confident that data-id is ok. I'm not a front-end guy ;)

Copy link
Member

Choose a reason for hiding this comment

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

I meant just id, not data-id :D but then you'd probably need a message- prefix to avoid collisions..

'<a class="label" href="#" class="name">{{displayName}}</a>' +
'<span class="spacer">▸</span>{{{message}}}</div></li>'
};
Expand Down