Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Commit

Permalink
Fix tinting of notification icon and use a more reliable notification…
Browse files Browse the repository at this point in the history
… 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`.
  • Loading branch information
turt2live committed Dec 7, 2018
1 parent 173669b commit 95d15b7
Show file tree
Hide file tree
Showing 4 changed files with 33 additions and 11 deletions.
5 changes: 5 additions & 0 deletions src/Notifier.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
13 changes: 10 additions & 3 deletions src/Tinter.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
});
Expand All @@ -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");
}
Expand Down
10 changes: 4 additions & 6 deletions src/components/structures/RightPanel.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
},

Expand Down Expand Up @@ -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',
});
Expand Down
16 changes: 14 additions & 2 deletions src/components/views/elements/TintableSvg.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,15 +51,26 @@ 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!
Tinter.applySvgFixups(this.fixups);
},

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

Expand All @@ -72,6 +83,7 @@ var TintableSvg = React.createClass({
height={this.props.height}
onLoad={this.onLoad}
tabIndex="-1"
ref="svgContainer"
/>
);
},
Expand Down

0 comments on commit 95d15b7

Please sign in to comment.