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`.
pull/21833/head
Travis Ralston 2018-12-06 22:26:51 -07:00
parent 173669b375
commit 95d15b7863
4 changed files with 33 additions and 11 deletions

View File

@ -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);
}

View File

@ -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");
}

View File

@ -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',
});

View File

@ -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"
/>
);
},