From 16fda4dde0dc7bb509c12347448f6cbca7de7aa2 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Thu, 15 Apr 2021 14:59:01 +0200 Subject: [PATCH 1/5] white background for transparent avatar images --- src/platform/web/ui/css/themes/element/theme.css | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/platform/web/ui/css/themes/element/theme.css b/src/platform/web/ui/css/themes/element/theme.css index 4c41ad97..c5667b53 100644 --- a/src/platform/web/ui/css/themes/element/theme.css +++ b/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; } From 357ce21678dabfdc599bb613cb1f6394b6322605 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Thu, 15 Apr 2021 15:09:45 +0200 Subject: [PATCH 2/5] extract base class from Template view to select update mechanism --- src/platform/web/ui/general/BaseUpdateView.js | 58 +++++++++++++++++++ src/platform/web/ui/general/TemplateView.js | 39 ++----------- 2 files changed, 64 insertions(+), 33 deletions(-) create mode 100644 src/platform/web/ui/general/BaseUpdateView.js diff --git a/src/platform/web/ui/general/BaseUpdateView.js b/src/platform/web/ui/general/BaseUpdateView.js new file mode 100644 index 00000000..4f346499 --- /dev/null +++ b/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; + } + } +} diff --git a/src/platform/web/ui/general/TemplateView.js b/src/platform/web/ui/general/TemplateView.js index 85345c53..8b0da9df 100644 --- a/src/platform/web/ui/general/TemplateView.js +++ b/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() { @@ -96,17 +75,15 @@ export class TemplateView { } else { throw new Error("no render function passed in, or overriden in subclass"); } - const parentProvidesUpdates = options && options.parentProvidesUpdates; - if (!parentProvidesUpdates) { - 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 +95,6 @@ export class TemplateView { return this._root; } - _updateFromValue(changedProps) { - this.update(this._value, changedProps); - } - update(value) { this._value = value; if (this._bindings) { From c85b2ca3c9a415bf2697f86ff4955a3ec31b6abb Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Thu, 15 Apr 2021 15:11:05 +0200 Subject: [PATCH 3/5] allow manually updating subviews in templates w/ parentProvidesUpdates --- src/platform/web/ui/general/TemplateView.js | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/platform/web/ui/general/TemplateView.js b/src/platform/web/ui/general/TemplateView.js index 8b0da9df..f72e3383 100644 --- a/src/platform/web/ui/general/TemplateView.js +++ b/src/platform/web/ui/general/TemplateView.js @@ -131,6 +131,14 @@ export class TemplateView extends BaseUpdateView { 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 @@ -260,10 +268,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); } From 766ce4e21701c34109e3cde787aec975b9caeda4 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Thu, 15 Apr 2021 15:12:14 +0200 Subject: [PATCH 4/5] create AvatarView and renderStaticAvatar (for timeline) and use it in RoomTileView, we make some efforts to only have one update listener for the entire list, because by default a subview would listen on the view model --- src/platform/web/ui/avatar.js | 119 ++++++++++++++++++ src/platform/web/ui/common.js | 19 --- .../web/ui/session/leftpanel/RoomTileView.js | 12 +- src/platform/web/ui/session/room/RoomView.js | 4 +- .../web/ui/session/room/timeline/common.js | 4 +- 5 files changed, 132 insertions(+), 26 deletions(-) create mode 100644 src/platform/web/ui/avatar.js diff --git a/src/platform/web/ui/avatar.js b/src/platform/web/ui/avatar.js new file mode 100644 index 00000000..c68d5496 --- /dev/null +++ b/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}); +} diff --git a/src/platform/web/ui/common.js b/src/platform/web/ui/common.js index f5b71198..9fbafcdf 100644 --- a/src/platform/web/ui/common.js +++ b/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]); -} diff --git a/src/platform/web/ui/session/leftpanel/RoomTileView.js b/src/platform/web/ui/session/leftpanel/RoomTileView.js index fde02c25..84b38b62 100644 --- a/src/platform/web/ui/session/leftpanel/RoomTileView.js +++ b/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); + } } diff --git a/src/platform/web/ui/session/room/RoomView.js b/src/platform/web/ui/session/room/RoomView.js index 65f464d9..b445dcac 100644 --- a/src/platform/web/ui/session/room/RoomView.js +++ b/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), ]), diff --git a/src/platform/web/ui/session/room/timeline/common.js b/src/platform/web/ui/session/room/timeline/common.js index 0d1e30ee..22bcd6b1 100644 --- a/src/platform/web/ui/session/room/timeline/common.js +++ b/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); From 33f1ba686c41bd56e45f800e77957123e1692ceb Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Thu, 15 Apr 2021 15:14:02 +0200 Subject: [PATCH 5/5] add warning when rendering outside of render fn for templates --- src/platform/web/ui/general/TemplateView.js | 34 +++++++++++++++------ 1 file changed, 25 insertions(+), 9 deletions(-) diff --git a/src/platform/web/ui/general/TemplateView.js b/src/platform/web/ui/general/TemplateView.js index f72e3383..fd27eafd 100644 --- a/src/platform/web/ui/general/TemplateView.js +++ b/src/platform/web/ui/general/TemplateView.js @@ -68,12 +68,16 @@ export class TemplateView extends BaseUpdateView { mount(options) { const builder = new TemplateBuilder(this); - if (this._render) { - this._root = this._render(builder, this._value); - } else if (this.render) { // overriden in subclass - this._root = this.render(builder, this._value); - } else { - throw new Error("no render function passed in, or overriden in subclass"); + try { + if (this._render) { + this._root = this._render(builder, this._value); + } else if (this.render) { // overriden in subclass + this._root = this.render(builder, this._value); + } else { + throw new Error("no render function passed in, or overriden in subclass"); + } + } finally { + builder.close(); } // takes care of update being called when needed super.mount(options); @@ -145,6 +149,18 @@ export class TemplateView extends BaseUpdateView { 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() { @@ -164,7 +180,7 @@ class TemplateBuilder { setAttribute(node, name, newValue); } }; - this._templateView._addBinding(binding); + this._addBinding(binding); binding(); } @@ -184,7 +200,7 @@ class TemplateBuilder { } }; - this._templateView._addBinding(binding); + this._addBinding(binding); return node; } @@ -240,7 +256,7 @@ class TemplateBuilder { node = newNode; } }; - this._templateView._addBinding(binding); + this._addBinding(binding); return node; }