Merge pull request #326 from vector-im/bwindels/fix-avatar-updates

Fix avatars not switching between an image and no image

src/platform/web/ui/avatar.js

@@ -0,0 +1,119 @@
+/*
+Copyright 2021 The Matrix.org Foundation C.I.C.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+import {tag, text, classNames} from "./general/html.js";
+import {BaseUpdateView} from "./general/BaseUpdateView.js";
+
+/*
+optimization to not use a sub view when changing between img and text
+because there can be many many instances of this view
+*/
+
+export class AvatarView extends BaseUpdateView {
+    /**
+     * @param  {ViewModel} value   view model with {avatarUrl, avatarColorNumber, avatarTitle, avatarLetter}
+     * @param  {Number} size
+     */
+    constructor(value, size) {
+        super(value);
+        this._root = null;
+        this._avatarUrl = null;
+        this._avatarTitle = null;
+        this._avatarLetter = null;
+        this._size = size;
+    }
+
+    _avatarUrlChanged() {
+        if (this.value.avatarUrl !== this._avatarUrl) {
+            this._avatarUrl = this.value.avatarUrl;
+            return true;
+        }
+        return false;
+    }
+
+    _avatarTitleChanged() {
+        if (this.value.avatarTitle !== this._avatarTitle) {
+            this._avatarTitle = this.value.avatarTitle;
+            return true;
+        }
+        return false;
+    }
+
+    _avatarLetterChanged() {
+        if (this.value.avatarLetter !== this._avatarLetter) {
+            this._avatarLetter = this.value.avatarLetter;
+            return true;
+        }
+        return false;
+    }
+
+    mount(options) {
+        this._avatarUrlChanged();
+        this._avatarLetterChanged();
+        this._avatarTitleChanged();
+        this._root = renderStaticAvatar(this.value, this._size);
+        // takes care of update being called when needed
+        super.mount(options);
+        return this._root;
+    }
+
+    root() {
+        return this._root;
+    }
+
+    update(vm) {
+        // important to always call _...changed for every prop 
+        if (this._avatarUrlChanged()) {
+            // avatarColorNumber won't change, it's based on room/user id
+            const bgColorClass = `usercolor${vm.avatarColorNumber}`;
+            if (vm.avatarUrl) {
+                this._root.replaceChild(renderImg(vm, this._size), this._root.firstChild);
+                this._root.classList.remove(bgColorClass);
+            } else {
+                this._root.replaceChild(text(vm.avatarLetter), this._root.firstChild);
+                this._root.classList.add(bgColorClass);
+            }
+        }
+        const hasAvatar = !!vm.avatarUrl;
+        if (this._avatarTitleChanged() && hasAvatar) {
+            const img = this._root.firstChild;
+            img.setAttribute("title", vm.avatarTitle);
+        }
+        if (this._avatarLetterChanged() && !hasAvatar) {
+            this._root.firstChild.textContent = vm.avatarLetter;
+        }
+    }
+}
+
+/**
+ * @param  {Object} vm   view model with {avatarUrl, avatarColorNumber, avatarTitle, avatarLetter}
+ * @param  {Number} size
+ * @return {Element}
+ */
+export function renderStaticAvatar(vm, size) {
+    const hasAvatar = !!vm.avatarUrl;
+    const avatarClasses = classNames({
+        avatar: true,
+        [`usercolor${vm.avatarColorNumber}`]: !hasAvatar,
+    });
+    const avatarContent = hasAvatar ? renderImg(vm, size) : text(vm.avatarLetter);
+    return tag.div({className: avatarClasses}, [avatarContent]);
+}
+
+function renderImg(vm, size) {
+    const sizeStr = size.toString();
+    return tag.img({src: vm.avatarUrl, width: sizeStr, height: sizeStr, title: vm.avatarTitle});
+}

src/platform/web/ui/common.js

@@ -31,22 +31,3 @@ export function spinner(t, extraClasses = undefined) {
     }
 }
 
-/**
- * @param  {TemplateBuilder} t
- * @param  {Object} vm   view model with {avatarUrl, avatarColorNumber, avatarTitle, avatarLetter}
- * @param  {Number} size
- * @return {Element}
- */
-export function renderAvatar(t, vm, size) {
-    const hasAvatar = !!vm.avatarUrl;
-    const avatarClasses = {
-        avatar: true,
-        [`usercolor${vm.avatarColorNumber}`]: !hasAvatar,
-    };
-    // TODO: handle updates from default to img or reverse
-    const sizeStr = size.toString();
-    const avatarContent = hasAvatar ?
-        t.img({src: vm => vm.avatarUrl, width: sizeStr, height: sizeStr, title: vm => vm.avatarTitle}) :
-        vm => vm.avatarLetter;
-    return t.div({className: avatarClasses}, [avatarContent]);
-}

src/platform/web/ui/css/themes/element/theme.css

@@ -39,7 +39,7 @@ limitations under the License.
 
 .avatar {
     border-radius: 100%;
-    background: #3D88FA;
+    background: #fff;
     color: white;
 }
 

src/platform/web/ui/general/BaseUpdateView.js

@@ -0,0 +1,58 @@
+/*
+Copyright 2021 The Matrix.org Foundation C.I.C.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+export class BaseUpdateView {
+    constructor(value) {
+        this._value = value;
+        // TODO: can avoid this if we adopt the handleEvent pattern in our EventListener
+        this._boundUpdateFromValue = null;
+    }
+
+    mount(options) {
+        const parentProvidesUpdates = options && options.parentProvidesUpdates;
+        if (!parentProvidesUpdates) {
+            this._subscribe();
+        }
+    }
+
+    unmount() {
+        this._unsubscribe();
+    }
+
+    get value() {
+        return this._value;
+    }
+
+    _updateFromValue(changedProps) {
+        this.update(this._value, changedProps);
+    }
+
+    _subscribe() {
+        if (typeof this._value?.on === "function") {
+            this._boundUpdateFromValue = this._updateFromValue.bind(this);
+            this._value.on("change", this._boundUpdateFromValue);
+        }
+    }
+
+    _unsubscribe() {
+        if (this._boundUpdateFromValue) {
+            if (typeof this._value.off === "function") {
+                this._value.off("change", this._boundUpdateFromValue);
+            }
+            this._boundUpdateFromValue = null;
+        }
+    }
+}

src/platform/web/ui/general/TemplateView.js

@@ -16,6 +16,7 @@ limitations under the License.
 
 import { setAttribute, text, isChildren, classNames, TAG_NAMES, HTML_NS } from "./html.js";
 import {errorToDOM} from "./error.js";
+import {BaseUpdateView} from "./BaseUpdateView.js";
 
 function objHasFns(obj) {
     for(const value of Object.values(obj)) {
@@ -38,37 +39,15 @@ function objHasFns(obj) {
         - add subviews inside the template
 */
 // TODO: should we rename this to BoundView or something? As opposed to StaticView ...
-export class TemplateView {
+export class TemplateView extends BaseUpdateView {
     constructor(value, render = undefined) {
-        this._value = value;
+        super(value);
         // TODO: can avoid this if we have a separate class for inline templates vs class template views
         this._render = render;
         this._eventListeners = null;
         this._bindings = null;
         this._subViews = null;
         this._root = null;
-        // TODO: can avoid this if we adopt the handleEvent pattern in our EventListener
-        this._boundUpdateFromValue = null;
-    }
-
-    get value() {
-        return this._value;
-    }
-
-    _subscribe() {
-        if (typeof this._value?.on === "function") {
-            this._boundUpdateFromValue = this._updateFromValue.bind(this);
-            this._value.on("change", this._boundUpdateFromValue);
-        }
-    }
-
-    _unsubscribe() {
-        if (this._boundUpdateFromValue) {
-            if (typeof this._value.off === "function") {
-                this._value.off("change", this._boundUpdateFromValue);
-            }
-            this._boundUpdateFromValue = null;
-        }
     }
 
     _attach() {
@@ -89,6 +68,7 @@ export class TemplateView {
 
     mount(options) {
         const builder = new TemplateBuilder(this);
+        try {
             if (this._render) {
                 this._root = this._render(builder, this._value);
             } else if (this.render) {   // overriden in subclass
@@ -96,17 +76,18 @@ export class TemplateView {
             } else {
                 throw new Error("no render function passed in, or overriden in subclass");
             }
-        const parentProvidesUpdates = options && options.parentProvidesUpdates;
+        } finally {
-        if (!parentProvidesUpdates) {
+            builder.close();
-            this._subscribe();
         }
+        // takes care of update being called when needed
+        super.mount(options);
         this._attach();
         return this._root;
     }
 
     unmount() {
         this._detach();
-        this._unsubscribe();
+        super.unmount();
         if (this._subViews) {
             for (const v of this._subViews) {
                 v.unmount();
@@ -118,10 +99,6 @@ export class TemplateView {
         return this._root;
     }
 
-    _updateFromValue(changedProps) {
-        this.update(this._value, changedProps);
-    }
-
     update(value) {
         this._value = value;
         if (this._bindings) {
@@ -158,12 +135,32 @@ export class TemplateView {
             this._subViews.splice(idx, 1);
         }
     }
+
+    updateSubViews(value, props) {
+        if (this._subViews) {
+            for (const v of this._subViews) {
+                v.update(value, props);
+            }
+        }
+    }
 }
 
 // what is passed to render
 class TemplateBuilder {
     constructor(templateView) {
         this._templateView = templateView;
+        this._closed = false;
+    }
+
+    close() {
+        this._closed = true;
+    }
+
+    _addBinding(fn) {
+        if (this._closed) {
+            console.trace("Adding a binding after render will likely cause memory leaks");
+        }
+        this._templateView._addBinding(fn);
     }
 
     get _value() {
@@ -183,7 +180,7 @@ class TemplateBuilder {
                 setAttribute(node, name, newValue);
             }
         };
-        this._templateView._addBinding(binding);
+        this._addBinding(binding);
         binding();
     }
 
@@ -203,7 +200,7 @@ class TemplateBuilder {
             }
         };
 
-        this._templateView._addBinding(binding);
+        this._addBinding(binding);
         return node;
     }
 
@@ -259,7 +256,7 @@ class TemplateBuilder {
                 node = newNode;
             }
         };
-        this._templateView._addBinding(binding);
+        this._addBinding(binding);
         return node;
     }
 
@@ -287,10 +284,10 @@ class TemplateBuilder {
 
     // this insert a view, and is not a view factory for `if`, so returns the root element to insert in the template
     // you should not call t.view() and not use the result (e.g. attach the result to the template DOM tree).
-    view(view) {
+    view(view, mountOptions = undefined) {
         let root;
         try {
-            root = view.mount();
+            root = view.mount(mountOptions);
         } catch (err) {
             return errorToDOM(err);
         }

src/platform/web/ui/session/leftpanel/RoomTileView.js

@@ -16,7 +16,7 @@ limitations under the License.
 */
 
 import {TemplateView} from "../../general/TemplateView.js";
-import {renderAvatar} from "../../common.js";
+import {AvatarView} from "../../avatar.js";
 
 export class RoomTileView extends TemplateView {
     render(t, vm) {
@@ -26,12 +26,12 @@ export class RoomTileView extends TemplateView {
         };
         return t.li({"className": classes}, [
             t.a({href: vm.url}, [
-                renderAvatar(t, vm, 32),
+                t.view(new AvatarView(vm, 32), {parentProvidesUpdates: true}),
                 t.div({className: "description"}, [
                     t.div({className: {"name": true, unread: vm => vm.isUnread}}, vm => vm.name),
                     t.div({
                         className: {
-                            "badge": true,
+                            badge: true,
                             highlighted: vm => vm.isHighlighted,
                             hidden: vm => !vm.badgeCount
                         }
@@ -40,4 +40,10 @@ export class RoomTileView extends TemplateView {
             ])
         ]);
     }
+
+    update(value, props) {
+        super.update(value);
+        // update the AvatarView as we told it to not subscribe itself with parentProvidesUpdates
+        this.updateSubViews(value, props);
+    }
 }

src/platform/web/ui/session/room/RoomView.js

@@ -19,7 +19,7 @@ import {TemplateView} from "../../general/TemplateView.js";
 import {TimelineList} from "./TimelineList.js";
 import {TimelineLoadingView} from "./TimelineLoadingView.js";
 import {MessageComposer} from "./MessageComposer.js";
-import {renderAvatar} from "../../common.js";
+import {AvatarView} from "../../avatar.js";
 
 export class RoomView extends TemplateView {
     render(t, vm) {
@@ -27,7 +27,7 @@ export class RoomView extends TemplateView {
             t.div({className: "TimelinePanel"}, [
                 t.div({className: "RoomHeader middle-header"}, [
                     t.a({className: "button-utility close-middle", href: vm.closeUrl, title: vm.i18n`Close room`}),
-                    renderAvatar(t, vm, 32),
+                    t.view(new AvatarView(vm, 32)),
                     t.div({className: "room-description"}, [
                         t.h2(vm => vm.name),
                     ]),

src/platform/web/ui/session/room/timeline/common.js

@@ -15,7 +15,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-import {renderAvatar} from "../../../common.js";
+import {renderStaticAvatar} from "../../../avatar.js";
 
 export function renderMessage(t, vm, children) {
     const classes = {
@@ -28,7 +28,7 @@ export function renderMessage(t, vm, children) {
     };
 
     const profile = t.div({className: "profile"}, [
-        renderAvatar(t, vm, 30),
+        renderStaticAvatar(vm, 30),
         t.div({className: `sender usercolor${vm.avatarColorNumber}`}, vm.displayName)
     ]);
     children = [profile].concat(children);