From 95d15b78632bdf770928729fb3f468295cd74925 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Thu, 6 Dec 2018 22:26:51 -0700 Subject: [PATCH] Fix tinting of notification icon and use a more reliable notification source The js-sdk's placement of the notification change was unreliable and could cause stuck notifications. The new location (piggybacking the Notifier) is a lot more reliable. The tinting has been changed fairly invasively in order to support the changing of the `fill` attribute. What was happening before was the `fill` property would happily get set to the forced color value, but when it came time to reset it it wouldn't be part of the colors array and fail the check, therefore never being changed back. By using a second field we can ensure we are checking the not-forced value where possible, falling back to the potentially forced value if needed. In addition to fixing which color the Tinter was checking against, something noticed during development is that `this.colors` might not always be a set of hex color codes. This is problematic when the attribute we're looking to replace is a rgb color code but we're only looking at `keyHex` - the value won't be reset. It appears as though this happens when people use custom tinting in places as `this.colors` often gets set to the rgb values throughout the file. To fix it, we just check against `keyHex` and `keyRgb`. --- src/Notifier.js | 5 +++++ src/Tinter.js | 13 ++++++++++--- src/components/structures/RightPanel.js | 10 ++++------ src/components/views/elements/TintableSvg.js | 16 ++++++++++++++-- 4 files changed, 33 insertions(+), 11 deletions(-) diff --git a/src/Notifier.js b/src/Notifier.js index 80e8be1084..8550f3bf95 100644 --- a/src/Notifier.js +++ b/src/Notifier.js @@ -289,6 +289,11 @@ const Notifier = { const room = MatrixClientPeg.get().getRoom(ev.getRoomId()); const actions = MatrixClientPeg.get().getPushActionsForEvent(ev); if (actions && actions.notify) { + dis.dispatch({ + action: "event_notification", + event: ev, + room: room, + }); if (this.isEnabled()) { this._displayPopupNotification(ev, room); } diff --git a/src/Tinter.js b/src/Tinter.js index 1b1ebbcccd..9c2afd4fab 100644 --- a/src/Tinter.js +++ b/src/Tinter.js @@ -419,11 +419,18 @@ class Tinter { for (let k = 0; k < this.svgAttrs.length; k++) { const attr = this.svgAttrs[k]; for (let m = 0; m < this.keyHex.length; m++) { // dev note: don't use L please. - if (tag.getAttribute(attr) && - tag.getAttribute(attr).toUpperCase() === this.keyHex[m]) { + // We use a different attribute from the one we're setting + // because we may also be using forceColors. If we were to + // check the keyHex against a forceColors value, it may not + // match and therefore not change when we need it to. + const valAttrName = "mx-val-" + attr; + let attribute = tag.getAttribute(valAttrName); + if (!attribute) attribute = tag.getAttribute(attr); // fall back to the original + if (attribute && (attribute.toUpperCase() === this.keyHex[m] || attribute.toLowerCase() === this.keyRgb[m])) { fixups.push({ node: tag, attr: attr, + refAttr: valAttrName, index: m, forceColors: forceColors, }); @@ -442,8 +449,8 @@ class Tinter { for (let i = 0; i < fixups.length; i++) { const svgFixup = fixups[i]; const forcedColor = svgFixup.forceColors ? svgFixup.forceColors[svgFixup.index] : null; - if (forcedColor) console.log(forcedColor); svgFixup.node.setAttribute(svgFixup.attr, forcedColor ? forcedColor : this.colors[svgFixup.index]); + svgFixup.node.setAttribute(svgFixup.refAttr, this.colors[svgFixup.index]); } if (DEBUG) console.log("applySvgFixups end"); } diff --git a/src/components/structures/RightPanel.js b/src/components/structures/RightPanel.js index c21c5f459f..0870f085a5 100644 --- a/src/components/structures/RightPanel.js +++ b/src/components/structures/RightPanel.js @@ -124,7 +124,6 @@ module.exports = React.createClass({ this.dispatcherRef = dis.register(this.onAction); const cli = this.context.matrixClient; cli.on("RoomState.members", this.onRoomStateMember); - cli.on("Room.notificationCounts", this.onRoomNotifications); this._initGroupStore(this.props.groupId); }, @@ -212,16 +211,15 @@ module.exports = React.createClass({ } }, - onRoomNotifications: function(room, type, count) { - if (type === "highlight") this.forceUpdate(); - }, - _delayedUpdate: new RateLimitedFunc(function() { this.forceUpdate(); // eslint-disable-line babel/no-invalid-this }, 500), onAction: function(payload) { - if (payload.action === "view_user") { + if (payload.action === "event_notification") { + // Try and re-caclulate any badge counts we might have + this.forceUpdate(); + } else if (payload.action === "view_user") { dis.dispatch({ action: 'show_right_panel', }); diff --git a/src/components/views/elements/TintableSvg.js b/src/components/views/elements/TintableSvg.js index af9e56377b..08628c8ca9 100644 --- a/src/components/views/elements/TintableSvg.js +++ b/src/components/views/elements/TintableSvg.js @@ -51,6 +51,12 @@ var TintableSvg = React.createClass({ delete TintableSvg.mounts[this.id]; }, + componentDidUpdate: function(prevProps, prevState) { + if (prevProps.forceColors !== this.props.forceColors) { + this.calcAndApplyFixups(this.refs.svgContainer); + } + }, + tint: function() { // TODO: only bother running this if the global tint settings have changed // since we loaded! @@ -58,8 +64,13 @@ var TintableSvg = React.createClass({ }, onLoad: function(event) { - // console.log("TintableSvg.onLoad for " + this.props.src); - this.fixups = Tinter.calcSvgFixups([event.target], this.props.forceColors); + this.calcAndApplyFixups(event.target); + }, + + calcAndApplyFixups: function(target) { + if (!target) return; + // console.log("TintableSvg.calcAndApplyFixups for " + this.props.src); + this.fixups = Tinter.calcSvgFixups([target], this.props.forceColors); Tinter.applySvgFixups(this.fixups); }, @@ -72,6 +83,7 @@ var TintableSvg = React.createClass({ height={this.props.height} onLoad={this.onLoad} tabIndex="-1" + ref="svgContainer" /> ); },