Performance observation

[lishid]

Reported by @kometenstaub

There might be a performance issue where this line is repeatedly setting up a requestAnimationFrame and consuming CPU when nothing is happening:
https://github.com/Simonwep/pickr/blob/411cb1d214f961e0238ad72fecf6abf3e99113ca/src/js/pickr.js#L147

Seems to come from a dependency and we traced the issue to this plugin. Not sure if there are any specific repro steps but it doesn't always happen.

[lishid]

Update: Seems to be related to https://github.com/valentine195/obsidian-settings-search which sets up each setting's tab in the background to index the search.

[nothingislost]

So whose issue are we considering this? :)

[lishid]

I'd pin it on the picker because it's written poorly.

In the case where it's never put into DOM, it would infinite loop requestAnimationFrame which is a really bad way of detecting when an element is inserted into the DOM.

Given that you're including the library in this plugin (and I'm also seeing it in Highlightr) I think it's best if you either patch it up here, submit a patch upstream, or use a different library, unless you have a better suggestion?

[nothingislost]

I chose Pickr because Style Settings is using it. That's probably the most popular plugin using it currently.

I haven't really looked at the internals of Pickr but I'll take a look at that requestAnimationFrame behavior and see if it makes sense to patch it or move to a different library.

[lishid]

Ok fair enough. Maybe we should also tag @mgmeyers and @chetachiezikeuzor for a collaboration.

[nothingislost]

Looks like Pickr is no longer maintained so patching it is less of an option unless we fork it which I'm not super interested in doing. I did a quick look and https://github.com/mdbassit/Coloris seems interesting. I did a quick search and there's no reference to requestAnimationFrame in their codebase at least ;) I'll try it out and follow up with some porting instructions if it works out.

[mgmeyers]

Blerg. I recently saw that pickr is no longer being maintained and wanted to swap it out for something else. This just gives me another reason!

[mgmeyers]

@nothingislost Let me know how Coloris works out for you. Cecilia May recommended iro.js as another option: mgmeyers/obsidian-style-settings#50

[ebullient]

As an interesting offshoot ... @lishid, it would be nice if we could have a color picker as one of the controls provided by obsidian. You need a picker, too, for graph node colors. I would LOVE to just have one that handles cut/paste and alphas and things such that I don't have to care. ;) Then snippetor and style settings and dynamic highlights and admonitions and (however many more plugins use colors that I haven't thought of) can all have a consistent experience.

[lishid]

Oh yes. I would probably look into adding a color picker component for plugins to use and share. Probably when we are ready to integrate Style Settings into core.

[nothingislost]

I officially hate Pickr now :) I decided to go down the route of just fixing the Pickr problem after running into maturity issues with Coloris.

The plan is to unload Pickr in the PluginSettingTab.hide() method. I patched Settings Search to make a call to hide() after reading all of the tab settings. I then patched Dynamic Highlights to call Pickr.destroyAndRemove() in the hide() method. Everything is looking good so far but only because my Pickr instance is never hidden from view.

There is still an issue with Style Settings though. Style Settings loads many Pickr instances and all of them are hidden by default, which is what triggers the animationFrame silliness. Even after patching Style Settings to unload all Pickr instances on hide(), the issue still persisted.

Pickr cancels the animation frame on destroy() by using the ID of the animation frame it receives initially... but here https://github.com/Simonwep/pickr/blob/411cb1d214f961e0238ad72fecf6abf3e99113ca/src/js/pickr.js#L151 they keep requesting new animation frames and getting new IDs... yet they don't update their internal animation frame ID tracker with the new ID... so when it goes to destroy the element, it's requesting to cancel an animation frame ID that doesn't exist.

This means I have to fork Pickr now but at least it's a fairly easy fix.

tl;dr booo Pickr

[nothingislost]

Pickr fork: https://github.com/nothingislost/pickr/tree/a17739f7aa1871b44da778cbb79ae76dae77d839

Settings Search patch: javalent/settings-search#7
Dynamic Highlights patch: b7248e1
Style Settings patch: mgmeyers/obsidian-style-settings#60

before:

after: