From e1c1b1aa7283a5004336a8b951e0f4ad7cc74085 Mon Sep 17 00:00:00 2001 From: Daniel Gasienica Date: Mon, 19 Mar 2018 17:33:14 -0400 Subject: [PATCH] Load attachment data before rendering Prevent double rendering of attachments by multiple entries into `MessageView::render` using promises. --- js/views/message_view.js | 129 +++++++++++++++++++++++---------------- 1 file changed, 75 insertions(+), 54 deletions(-) diff --git a/js/views/message_view.js b/js/views/message_view.js index 89b4865e1..cc619e436 100644 --- a/js/views/message_view.js +++ b/js/views/message_view.js @@ -6,6 +6,9 @@ 'use strict'; window.Whisper = window.Whisper || {}; + const { Attachment } = window.Signal.Types; + const { context: migrationContext } = window.Signal.Migrations; + var URL_REGEX = /(^|[\s\n]|)((?:https?|ftp):\/\/[\-A-Z0-9\u00A0-\uD7FF\uE000-\uFDCF\uFDF0-\uFFFD+\u0026\u2019@#\/%?=()~_|!:,.;]*[\-A-Z0-9+\u0026@#\/%=~()_|])/gi; var ErrorIconView = Whisper.View.extend({ @@ -179,6 +182,9 @@ return this.model.id; }, initialize: function() { + // loadedAttachmentViews :: Promise (Array AttachmentView) | null + this.loadedAttachmentViews = null; + this.listenTo(this.model, 'change:errors', this.onErrorsChanged); this.listenTo(this.model, 'change:body', this.render); this.listenTo(this.model, 'change:delivered', this.renderDelivered); @@ -224,7 +230,8 @@ // Failsafe: if in the background, animation events don't fire setTimeout(this.remove.bind(this), 1000); }, - onUnload: function() { + /* jshint ignore:start */ + onUnload: async function() { if (this.avatarView) { this.avatarView.remove(); } @@ -240,18 +247,16 @@ if (this.timeStampView) { this.timeStampView.remove(); } - if (this.loadedAttachments && this.loadedAttachments.length) { - for (var i = 0, max = this.loadedAttachments.length; i < max; i += 1) { - var view = this.loadedAttachments[i]; - view.unload(); - } - } + + const views = await this.loadedAttachmentViews; + views.forEach(view => view.unload()); // No need to handle this one, since it listens to 'unload' itself: // this.timerView this.remove(); }, + /* jshint ignore:end */ onDestroy: function() { if (this.$el.hasClass('expired')) { return; @@ -376,7 +381,12 @@ this.renderErrors(); this.renderExpiring(); - this.loadAttachments(); + + // NOTE: We have to do this in the background (`then` instead of `await`) + // as our code / Backbone seems to rely on `render` synchronously returning + // `this` instead of `Promise MessageView` (this): + // eslint-disable-next-line more/no-then + this.loadAttachmentViews().then(views => this.renderAttachmentViews(views)); return this; }, @@ -395,51 +405,62 @@ }))(); this.$('.avatar').replaceWith(this.avatarView.render().$('.avatar')); }, - appendAttachmentView: function(view) { - // We check for a truthy 'updated' here to ensure that a race condition in a - // multi-fetch() scenario doesn't add an AttachmentView to the DOM before - // its 'update' event is triggered. - var parent = this.$('.attachments')[0]; - if (view.updated && parent !== view.el.parentNode) { - if (view.el.parentNode) { - view.el.parentNode.removeChild(view.el); - } - - this.trigger('beforeChangeHeight'); - this.$('.attachments').append(view.el); - view.setElement(view.el); - this.trigger('afterChangeHeight'); - } - }, - loadAttachments: function() { - this.loadedAttachments = this.loadedAttachments || []; - - // If we're called a second time, render() has replaced the DOM out from under - // us with $el.html(). We'll need to reattach our AttachmentViews to the new - // parent DOM nodes if the 'update' event has already fired. - if (this.loadedAttachments.length) { - for (var i = 0, max = this.loadedAttachments.length; i < max; i += 1) { - var view = this.loadedAttachments[i]; - this.appendAttachmentView(view); - } - return; - } - - this.model.get('attachments').forEach(function(attachment) { - var view = new Whisper.AttachmentView({ - model: attachment, - timestamp: this.model.get('sent_at') - }); - this.loadedAttachments.push(view); - - this.listenTo(view, 'update', function() { - view.updated = true; - this.appendAttachmentView(view); - }); - - view.render(); - }.bind(this)); - } + /* eslint-enable */ + /* jshint ignore:start */ + loadAttachmentViews() { + if (this.loadedAttachmentViews !== null) { + return this.loadedAttachmentViews; + } + + const loadData = Attachment.loadData(migrationContext.readAttachmentData); + const attachments = this.model.get('attachments'); + const loadedAttachmentViews = Promise.all(attachments.map(attachment => + new Promise(async (resolve) => { + const attachmentWithData = await loadData(attachment); + const view = new Whisper.AttachmentView({ + model: attachmentWithData, + timestamp: this.model.get('sent_at'), + }); + + this.listenTo(view, 'update', () => { + // NOTE: Can we do without `updated` flag now that we use promises? + view.updated = true; + resolve(view); + }); + + view.render(); + }))); + + // Memoize attachment views to avoid double loading: + this.loadedAttachmentViews = loadedAttachmentViews; + + return loadedAttachmentViews; + }, + renderAttachmentViews(views) { + views.forEach(view => this.renderAttachmentView(view)); + }, + renderAttachmentView(view) { + if (!view.updated) { + throw new Error('Invariant violation:' + + ' Cannot render an attachment view that isn’t ready'); + } + + const parent = this.$('.attachments')[0]; + const isViewAlreadyChild = parent === view.el.parentNode; + if (isViewAlreadyChild) { + return; + } + + if (view.el.parentNode) { + view.el.parentNode.removeChild(view.el); + } + + this.trigger('beforeChangeHeight'); + this.$('.attachments').append(view.el); + view.setElement(view.el); + this.trigger('afterChangeHeight'); + }, + /* jshint ignore:end */ + /* eslint-disable */ }); - })();