From 173669b375aa86fda3303a757db14f4407a5854a Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Thu, 6 Dec 2018 16:18:02 -0700 Subject: [PATCH 1/2] Show the number of unread notifications above the bell on the right Fixes https://github.com/vector-im/riot-web/issues/3383 This achieves the result by counting up the number of highlights across all rooms and setting that as the badge above the icon. If there are no highlights, nothing is displayed. The red highlight on the bell is done by abusing how the Tinter works: because it has access to the properties of the SVG that we'd need to override it, we give it a collection of colors it should use instead of the theme/tint it is trying to apply. This results in the Tinter using our warning color instead of whatever it was going to apply. The RightPanel now listens for events to update the count too, otherwise when the user receives a ping they'd have to switch rooms to see the change. --- res/css/structures/_RightPanel.scss | 4 +++ src/Tinter.js | 13 +++++---- src/components/structures/RightPanel.js | 28 ++++++++++++++++++-- src/components/views/elements/TintableSvg.js | 3 ++- 4 files changed, 40 insertions(+), 8 deletions(-) diff --git a/res/css/structures/_RightPanel.scss b/res/css/structures/_RightPanel.scss index b4dff612ed..554aabfcd1 100644 --- a/res/css/structures/_RightPanel.scss +++ b/res/css/structures/_RightPanel.scss @@ -55,6 +55,10 @@ limitations under the License. padding-bottom: 3px; } +.mx_RightPanel_headerButton_badgeHighlight .mx_RightPanel_headerButton_badge { + color: $warning-color; +} + .mx_RightPanel_headerButton_highlight { width: 25px; height: 5px; diff --git a/src/Tinter.js b/src/Tinter.js index d24a4c3e74..1b1ebbcccd 100644 --- a/src/Tinter.js +++ b/src/Tinter.js @@ -390,7 +390,7 @@ class Tinter { // XXX: we could just move this all into TintableSvg, but as it's so similar // to the CSS fixup stuff in Tinter (just that the fixups are stored in TintableSvg) // keeping it here for now. - calcSvgFixups(svgs) { + calcSvgFixups(svgs, forceColors) { // go through manually fixing up SVG colours. // we could do this by stylesheets, but keeping the stylesheets // updated would be a PITA, so just brute-force search for the @@ -418,13 +418,14 @@ class Tinter { const tag = tags[j]; for (let k = 0; k < this.svgAttrs.length; k++) { const attr = this.svgAttrs[k]; - for (let l = 0; l < this.keyHex.length; l++) { + 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[l]) { + tag.getAttribute(attr).toUpperCase() === this.keyHex[m]) { fixups.push({ node: tag, attr: attr, - index: l, + index: m, + forceColors: forceColors, }); } } @@ -440,7 +441,9 @@ class Tinter { if (DEBUG) console.log("applySvgFixups start for " + fixups); for (let i = 0; i < fixups.length; i++) { const svgFixup = fixups[i]; - svgFixup.node.setAttribute(svgFixup.attr, this.colors[svgFixup.index]); + 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]); } if (DEBUG) console.log("applySvgFixups end"); } diff --git a/src/components/structures/RightPanel.js b/src/components/structures/RightPanel.js index 9017447a34..c21c5f459f 100644 --- a/src/components/structures/RightPanel.js +++ b/src/components/structures/RightPanel.js @@ -30,6 +30,7 @@ import { showGroupInviteDialog, showGroupAddRoomDialog } from '../../GroupAddres import GroupStore from '../../stores/GroupStore'; import { formatCount } from '../../utils/FormattingUtils'; +import MatrixClientPeg from "../../MatrixClientPeg"; class HeaderButton extends React.Component { constructor() { @@ -49,17 +50,26 @@ class HeaderButton extends React.Component { const TintableSvg = sdk.getComponent("elements.TintableSvg"); const AccessibleButton = sdk.getComponent("elements.AccessibleButton"); + // XXX: We really shouldn't be hardcoding colors here, but the way TintableSvg + // works kinda prevents us from using normal CSS tactics. We use $warning-color + // here. + // Note: This array gets passed along to the Tinter's forceColors eventually. + const tintableColors = this.props.badgeHighlight ? ["#ff0064"] : null; + + const classNames = ["mx_RightPanel_headerButton"]; + if (this.props.badgeHighlight) classNames.push("mx_RightPanel_headerButton_badgeHighlight"); + return
{ this.props.badge ? this.props.badge :   }
- + { this.props.isHighlighted ?
:
} ; @@ -76,6 +86,7 @@ HeaderButton.propTypes = { // The badge to display above the icon badge: PropTypes.node, + badgeHighlight: PropTypes.bool, // The parameters to track the click event analytics: PropTypes.arrayOf(PropTypes.string).isRequired, @@ -113,6 +124,7 @@ 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); }, @@ -200,6 +212,10 @@ 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), @@ -308,6 +324,13 @@ module.exports = React.createClass({ let headerButtons = []; if (this.props.roomId) { + let notifCountBadge; + let notifCount = 0; + MatrixClientPeg.get().getRooms().forEach(r => notifCount += (r.getUnreadNotificationCount('highlight') || 0)); + if (notifCount > 0) { + notifCountBadge =
{ formatCount(notifCount) }
; + } + headerButtons = [ 0} analytics={['Right Panel', 'Notification List Button', 'click']} />, ]; diff --git a/src/components/views/elements/TintableSvg.js b/src/components/views/elements/TintableSvg.js index e04bf87793..af9e56377b 100644 --- a/src/components/views/elements/TintableSvg.js +++ b/src/components/views/elements/TintableSvg.js @@ -29,6 +29,7 @@ var TintableSvg = React.createClass({ width: PropTypes.string.isRequired, height: PropTypes.string.isRequired, className: PropTypes.string, + forceColors: PropTypes.arrayOf(PropTypes.string), }, statics: { @@ -58,7 +59,7 @@ var TintableSvg = React.createClass({ onLoad: function(event) { // console.log("TintableSvg.onLoad for " + this.props.src); - this.fixups = Tinter.calcSvgFixups([event.target]); + this.fixups = Tinter.calcSvgFixups([event.target], this.props.forceColors); Tinter.applySvgFixups(this.fixups); }, From 95d15b78632bdf770928729fb3f468295cd74925 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Thu, 6 Dec 2018 22:26:51 -0700 Subject: [PATCH 2/2] 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" /> ); },