Make SVGs and CSS dynamically recolourable #77

ara4n · 2016-01-05T03:43:26Z

I'm not proud of this.

Turns any colour-themed SVGs from img tags into object tags to make them dynamically editable from JS, and implements a simple /tint command to let you clobber the CSS and appropriate SVG attributes with a custom colour scheme.

Requires facebook/react#5781 to work, as otherwise react doesn't understand onLoad on object tags.

ara4n · 2016-01-05T03:44:20Z

Could particularly do with review on Tinter.js - is it safe to store state in plain old vars like this, or should it be all bundled up into an exported object? Or is that just hygiene?

richvdh · 2016-01-05T13:57:49Z

src/SlashCommands.js

@@ -42,6 +43,11 @@ var commands = {
        return reject("Usage: /nick <display_name>");
    },

+    tint: function(room_id, args) {


(most of) the other commands have a comment saying what they do. wouldn't hurt here.

kegsay · 2016-01-05T14:45:18Z

is it safe to store state in plain old vars like this, or should it be all bundled up into an exported object? Or is that just hygiene?

It should be safe but in our case it isn't. It's usually safe because require() calls cache the first time they are called then return the same instance of module.exports again (along with closures containing those vars). It isn't safe for us because we tend to use the same file in multiple projects and we want that to refer to the same variables cross-project. This isn't how CommonJS modules work. You only get singleton-like behaviour in files in the same project. Cross-project references to the same file returns multiple instances (one for each project).

So what does this actually mean? It means that as it stands, if vector-web were to require("matrix-react-sdk/lib/Tinter") it would get an entirely new set of vars to the Tinter being required in matrix-react-sdk. You can refer to the same instance by doing a hack:

module.exports = {
  ...
};

if (!global.mxTinter) { // this attaches to 'window' (!)
    global.mxTinter = module.exports;
}
module.exports = global.mxTinter;

You would need to move all your var declarations to be attached to module.exports. This is all 'orrible - and is filed as element-hq/element-web#464

richvdh · 2016-01-05T14:51:34Z

src/Tinter.js

+            for (var k = 0; k < cssAttrs.length; k++) {
+                var attr = cssAttrs[k];
+                for (var l = 0; l < keyRgb.length; l++) {
+                    if (rule.style && rule.style[attr] === keyRgb[l]) {


can you add if (!rule.style) continue; at line 88, and simplify this?

yes, although it's very rare for a CSS rule to not have a style (it happens with weird rules like animation data which don't actually define styles), so it doesn't buy us much. have done it anyway.

…onload rather than react synthetic events

ara4n · 2016-01-06T02:42:59Z

I've addressed all the feedback other than:

the big point about putting Tinter's state into modules.exports rather than plain old vars (given we're only ever going to be importing Tinter from matrix-react-sdk, I'm not sure it's worth the effort right now)
the big point about not leaking bits of SVG DOM into Tinter's state, but instead making Tinter functional and apply to the DOM components themselves. Agreed this would be an improvement, but not sure it's worth the effort right now versus fixing other stuff.

Assuming we're agreed, I'll shift these to be maintenance bugs to address when we're out of crunch.

ara4n · 2016-01-06T02:43:06Z

@kegsay PTAL

ara4n · 2016-01-06T02:47:26Z

It's also worth noting that I was deliberately caching the required fixups for SVGs in the singleton Tinter rather than recalculating them every time onLoad (given fixups are still valid from previous loads). Given SVGs could theoretically be quite big, wasting time re-walking them everytime they reload to generate fixups would chew CPU and time for no reason at all.

...oh, except I ended up backing out that caching. ignore me. yup, it leaks, as the XXX comment says.

kegsay · 2016-01-06T12:01:40Z

putting Tinter's state into modules.exports rather than plain old vars (given we're only ever going to be importing Tinter from matrix-react-sdk, I'm not sure it's worth the effort right now)

WFM. You've put a big warning up so I'm happy as is.

not leaking bits of SVG DOM into Tinter's state, but instead making Tinter functional and apply to the DOM components themselves. Agreed this would be an improvement, but not sure it's worth the effort right now versus fixing other stuff.

So I had a look at how badly it leaks. My test was as follows:

Refresh page and swap to a room.
Take Heap Snapshot.
Change tint (if applicable)
Swap to different rooms ~10 times.
Take Heap Snapshot.

On develop - The heap snapshot delta was <1MB (21.6MB to 22.4MB).
On matthew/dynamic-svg - The heap snapshot delta was 34MB (24MB to 58MB).

This equates to about 3MB per room swap. This is too much for a long-lived web app. I don't mind fixing the leak for you if you're stretched for time, but I would be unhappy leaving it as-is.

ara4n · 2016-01-06T15:43:57Z

i'm very surprised the leak is this bad. will fix it. suggestions on how to refer to the set of tintablesvgs when the user runs /tint?

On 6 Jan 2016, at 12:01, Kegsay notifications@github.com wrote:

putting Tinter's state into modules.exports rather than plain old vars (given we're only ever going to be importing Tinter from matrix-react-sdk, I'm not sure it's worth the effort right now)

WFM. You've put a big warning up so I'm happy as is.

not leaking bits of SVG DOM into Tinter's state, but instead making Tinter functional and apply to the DOM components themselves. Agreed this would be an improvement, but not sure it's worth the effort right now versus fixing other stuff.

So I had a look at how badly it leaks. My test was as follows:

Refresh page and swap to a room.
Take Heap Snapshot.
Change tint (if applicable)
Swap to different rooms ~10 times.
Take Heap Snapshot.
On develop - The heap snapshot delta was <1MB (21.6MB to 22.4MB).
On matthew/dynamic-svg - The heap snapshot delta was 34MB (24MB to 58MB).

This equates to about 3MB per room swap. This is too much for a long-lived web app. I don't mind fixing the leak for you if you're stretched for time, but I would be unhappy leaving it as-is.

—
Reply to this email directly or view it on GitHub.

kegsay · 2016-01-06T16:01:54Z

You just need to invert the control flow a bit. When people run /tint you should emit an event on the dispatcher (e.g. tint_change). Then you just need to listen for this in your TintableSvg and then change the tint. You never maintain the full set of tintable svgs.

kegsay · 2016-01-06T16:14:28Z

So the full flow would look like:

// SlashCommands.js
tint: function(newColour) {
  Tinter.setTint(newColour);
  dispatcher.dispatch({
    action: "tint_change"
  });
}
// ============

// TintableSvg.js
componentWillMount: function() {
  dis.register(this._onAction);
},

componentDidMount: function() {
  Tinter.applyTint(ReactDOM.findDOMNode(this));
},

_onAction: function(payload) {
  if (payload.action !== "tint_change") { return; }
  Tinter.applyTint(ReactDOM.findDOMNode(this));
}

// ============

// Tinter.js
setTint: function(newColour) {
  currentColour = newColour;
},

applyTint: function(domNode) {
  // apply `currentColour` to `domNode`
}

And make sure that Tinter never persists the passed domNode. It should only persist the current colour.

ara4n · 2016-01-06T20:43:56Z

oh, sorry, yes. was being thick.

…storing SVG DOM fragments in Tinter to avoid leaking them

ara4n · 2016-01-07T03:44:35Z

0f52c0a fixes the leak by making TintableSvgs responsible for updating their own tints, and responding to tint_update dispatch events. However I've kept the existing functions in Tinter.js rather than refactoring as per Kegan's full suggestion, as it retains symmetry with the half which is still needed for fixing up CSS (rather than SVGs), and i don't want to burn yet more time on this.

@kegsay: PTAL

kegsay · 2016-01-07T09:53:41Z

src/components/views/elements/TintableSvg.js

+
+    onAction: function(payload) {
+        if (payload.action !== 'tint_update') return;
+        Tinter.applySvgFixups(this.fixups);


If this is called prior to onLoad then this will NPE on Tinter.js:192.

why? this.fixups is initialised to [], not null?

Oh, right you are. However, the way you're doing it isn't what you intend I think. You're defining fixups on :27 as a prototype property, which is shared across all instances. You can't define instance properties like that, you need to use:

componentWillMount: function() { this.fixups = []; }

Without this, all TintableSvg elements would share the same fixups array(!)

oops, fixed, thanks

kegsay · 2016-01-07T10:24:32Z

LGTM - Did more heap dumps and now we no longer leak :)

Make SVGs and CSS dynamically recolourable

added missing strings

ara4n added 4 commits January 3, 2016 22:30

switch SVGs from imgs to objects

4f915d6

move title attributes to the div

3009da0

wire up Tinter.js

555abda

support fixing up dynamically loaded SVGs

001dc86

ara4n assigned dbkr Jan 5, 2016

ara4n mentioned this pull request Jan 5, 2016

Make SVGs and CSS dynamically recolourable element-hq/element-web#585

Merged

ara4n changed the title ~~Matthew/dynamic svg~~ Make SVG and CSS dynamically recolourable Jan 5, 2016

ara4n changed the title ~~Make SVG and CSS dynamically recolourable~~ Make SVGs and CSS dynamically recolourable Jan 5, 2016

ara4n added 3 commits January 5, 2016 03:51

oops, no double classNames

0e98764

oops, forgot this

9cf49eb

oops

aa1012b

richvdh assigned richvdh and unassigned dbkr Jan 5, 2016

richvdh reviewed Jan 5, 2016
View reviewed changes

kegsay assigned ara4n and unassigned richvdh Jan 5, 2016

ara4n added 5 commits January 6, 2016 01:11

Merge branch 'develop' into matthew/dynamic-svg

9e8daba

factor out tintable SVGs into their own component, and use plain DOM …

509ea7c

…onload rather than react synthetic events

comment /tint

44a0fa1

sundry PR feedback

f499c60

add fixme

8c1bb90

ara4n assigned kegsay and unassigned ara4n Jan 6, 2016

make TintableSvgs responsible for updating their own tints, and stop …

0f52c0a

…storing SVG DOM fragments in Tinter to avoid leaking them

oops, we actually need to cache the fixups in TintableSvg

296b626

kegsay reviewed Jan 7, 2016
View reviewed changes

oops, initialise fixups per-object, not per-prototype

fd32362

ara4n added a commit that referenced this pull request Jan 7, 2016

Merge pull request #77 from matrix-org/matthew/dynamic-svg

8170288

Make SVGs and CSS dynamically recolourable

ara4n merged commit 8170288 into develop Jan 7, 2016

dtygel pushed a commit to coletivoEITA/matrix-react-sdk that referenced this pull request May 15, 2017

Merge pull request matrix-org#77 from coletivoEITA/translations

b4c0d8d

added missing strings

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make SVGs and CSS dynamically recolourable #77

Make SVGs and CSS dynamically recolourable #77

ara4n commented Jan 5, 2016

ara4n commented Jan 5, 2016

richvdh Jan 5, 2016

ara4n Jan 6, 2016

kegsay commented Jan 5, 2016

richvdh Jan 5, 2016

ara4n Jan 6, 2016

ara4n commented Jan 6, 2016

ara4n commented Jan 6, 2016

ara4n commented Jan 6, 2016

kegsay commented Jan 6, 2016

ara4n commented Jan 6, 2016

kegsay commented Jan 6, 2016

kegsay commented Jan 6, 2016

ara4n commented Jan 6, 2016

ara4n commented Jan 7, 2016

kegsay Jan 7, 2016

ara4n Jan 7, 2016

kegsay Jan 7, 2016

ara4n Jan 7, 2016

kegsay commented Jan 7, 2016

Make SVGs and CSS dynamically recolourable #77

Make SVGs and CSS dynamically recolourable #77

Conversation

ara4n commented Jan 5, 2016

ara4n commented Jan 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kegsay commented Jan 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ara4n commented Jan 6, 2016

ara4n commented Jan 6, 2016

ara4n commented Jan 6, 2016

kegsay commented Jan 6, 2016

ara4n commented Jan 6, 2016

kegsay commented Jan 6, 2016

kegsay commented Jan 6, 2016

ara4n commented Jan 6, 2016

ara4n commented Jan 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kegsay commented Jan 7, 2016