Desktop: Resolves #7612 : Fixing Note Viewer theme reset issue on switching HTML to Md note or between 2 HTML notes.

[deepampriyadarshi]

Fixes #7612
The issue was caused due to the function addPluginAssets(assets) in ./packages/app-desktop/gui/note-viewer/index.html file. One of task of this function was to check loading multiple duplicate css/javascript files while switching between two notes. It works well when two notes share the same css styling (file) but it doesn't removes those css/javascript file links from the DOM tree which was loaded by some other notes and which shouldn't be applied to the current note being viewed. End result => the last appended link node for a css file overwrites the any previously loaded css file belonging to the current note being viewed.

[github-actions]

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

[deepampriyadarshi]

I have read the CLA Document and I hereby sign the CLA

[deepampriyadarshi]

Hi @tessus, @laurent22 in my first commit all of the checks passed except the BuildAndroidDebug one. I tried running commands (/gradlew assembleDebug), it worked fine. Also I was able to install it on my phone. And none of my code changes are related to android app. Can we run checks once again to see if it works now?

[laurent22]

I think recreating this on each setHtml call might be costly because it's called every time the note changes, so almost every keypress. Isn't it possible to either do this only when changing notes, or to remove the script/css that shouldn't be there?

[deepampriyadarshi]

I think recreating this on each setHtml call might be costly because it's called every time the note changes, so almost every keypress. Isn't it possible to either do this only when changing notes, or to remove the script/css that shouldn't be there?

True. I think the process of removing script/css will be equally, if not more, costly since then there would be 2 for loops. One which is already present which goes through assets list and second would be for parsing link nodes. So the former one seems to be a good solution. Also there's one more issue which I found now with the asset list which is passed as the parameter. It has duplicate entries for same CSS files. This only happens when switching between .md files and I think that is why the previous code was checking for any duplicate entries in the asset list. Although it served the purpose of not creating duplicate css/script link node but size of parameter passed keeps increasing on each note switch.

[laurent22]

removing script/css will be equally, if not more, costly since then there would be 2 for loops

Well, in most cases nothing needs to be added or removed, and looping a JS array costs pretty much nothing in terms of performance. It's adding and removing nodes that's more of a problem.

[deepampriyadarshi]

Well, in most cases nothing needs to be added or removed, and looping a JS array costs pretty much nothing in terms of performance. It's adding and removing nodes that's more of a problem.

The performance does gets affected if the loop is called on every setHtml (even on single key press) call and on each call the JS array size increases.. Below is the code snippet of the for loop that runs through pluginAssets array, from current dev branch :-

joplin/packages/app-desktop/gui/note-viewer/index.html

Lines 160 to 172 in 8842601

	function addPluginAssets(assets) {
	if (!assets) return;
	const pluginAssetsContainer = document.getElementById('joplin-container-pluginAssetsContainer');
	for (let i = 0; i < assets.length; i++) {
	const asset = assets[i];
	// # and ? can be used in valid paths and shouldn't be treated as the start of a query or fragment
	const encodedPath = asset.path
	.replaceAll('#','%23')
	.replaceAll('?','%3F')
	const assetId = asset.name ? asset.name : encodedPath;

---

Here a screen recording of what 's happening to the pluginAssests array with each setHtml call. Look at the zoomed in section at bottom centre. With just two lines in note, size of array became 90, out of which 73 are duplicate entries of the same file. You can verify from your end too. No doubt that this will stall the rendering as the above loop runs no matter what.

Demo.mp4

[deepampriyadarshi]

Hi @laurent22 can I get a review on my latest commit 66e8804. I believe that this will solve the above mentioned issues.

[laurent22]

That doesn't need to be defined outside the if block. We prefer to use const as much as possible and you can do that if you define the variable only where it's used.

[deepampriyadarshi]

Thanks for the guidance, this would fixed in my next commit.

[laurent22]

We avoid that kind of opaque "options" parameter. Please pass instead the noteId and assets as I think that's what you need

[deepampriyadarshi]

Thanks for the guidance, this would be fixed in my next commit.

[laurent22]

Re-assign pluginAssetsContainer to newPluginAssetsContainer then you don't need the awkward conditionals inside the loop (and yes it means that in that case pluginAssetsContainer can't be const)

[deepampriyadarshi]

Thanks for the guidance, this would be fixed in my next commit.

[laurent22]

I don't get what this does, but we really don't want this kind of things in the codebase. Also your message implies that the output of this function is modified by calling code, but that shouldn't happen. Something needs to be fixed properly somewhere

[deepampriyadarshi]

Thanks a lot @laurent22 for the review... Sorry for the long explanation. Kindly bear it to the end.

Explanation

So the function allProcessedAssets(Rules, theme, codeTheme) returns an object of the form given below, which it either computes from scratch or retrieves from this.allProcessedAssets_ class property (acting as a cache) -

Object Structure

{
      'cssString`: ["....", ".....", ...],
      'html': "",
      'pluginAssets': [ {...}, {...}, {...}, ....]
}

The above is a nested/complex object and this is the exact format in which it is stored in the cache.
Now as render() function is called on every note change/edit, the plugin assets values are taken from the cache to increase performance.

joplin/packages/renderer/MdToHtml.ts

Lines 332 to 339 in f5fc1f2

	// This return all the assets for all the plugins. Since it is called
	// on each render, the result is cached.
	private allProcessedAssets(rules: RendererRules, theme: any, codeTheme: string) {
	const cacheKey: string = theme.cacheKey + codeTheme;
	if (this.allProcessedAssets_[cacheKey]) return this.allProcessedAssets_[cacheKey];

Now in let output = { ...this.allProcessedAssets(allRules, options.theme, options.codeTheme) }; we have only a single spread ... operator which creates only a shallow copy (up to first level) of the object returned by allProcessedAssets(Rules, theme, codeTheme) and assigns it to the variable output as show below.

Shallow Copy

Further the render() function goes on to modify the output variable by calling private async outputAssetsToExternalAssets_(output: any) function that converts cssStrings to css files and pushes an object containing path and file type as properties, to the output.pluginAsset. But from the above image, since output.pluginAsset holds a reference to this.allProcessedAssets_.pluginAsset so it actually gets pushed over there. After that the function deletes output.cssStrings, again whose value is just a reference to this.allProcessedAssets_.cssStrings, and deleting it has no effect on this.allProcessedAssets_.cssStrings.

joplin/packages/renderer/MdToHtml.ts

Lines 380 to 387 in f5fc1f2

	private async outputAssetsToExternalAssets_(output: any) {
	for (const cssString of output.cssStrings) {
	const filePath = await this.fsDriver().cacheCssToFile(cssString);
	output.pluginAssets.push(filePath);
	}
	delete output.cssStrings;
	return output;
	}

The final state of the objects are shown below:-

Final States

Now on subsequent calls to render(), the render function is unaware of the modifications made in the entries of allProcessedAssets_ property and that the cache is in inconsistent state. Inconsistent because the structure of the object stored in cache allProcessedAssets_ (stated at the beginning of comment) signifies that the values in the allProcessedAssets_.cssStrings do not have their respective entry in the allProcessedAssets_.pluginAsset array, which is clearly not the case here as shown in above figure. It is the duty of render() to call outputAssetsToExternalAssets_() for making those entries. Thus the render function makes duplicate entries in the pluginAsset array on each of its call. This is evident form the video in my previous comment. Although for modern computer systems it may not be an issue to loop through a constantly increasing array but it causes cache inconsistencies which can be hard to detect since it doesn't raises any error

End Note

One final question still arises that, so what if the cache mechanism-allProcessedAssets_ is inconsistent, we still have a second cache, private cachedOutputs_: any = {}; as our savior, which caches the entire output returned by render() function.

Sadly it is also of no use at present since cache key used to store cache is generated using the body of the note, as one of its parameter. So editing a note or switching notes makes the render() run through its entirety.

joplin/packages/renderer/MdToHtml.ts

Lines 458 to 462 in f5fc1f2

	const cacheKey = md5(escape(body + this.customCss_ + JSON.stringify(options) + JSON.stringify(options.theme)));
	const cachedOutput = this.cachedOutputs_[cacheKey];
	if (cachedOutput) return cachedOutput;

[laurent22]

Thanks @deepampriyadarshi for the explanation. The problem is that outputAssetsToExternalAssets_ is modifying the input - and that should not happen because all functions should ideally be idempotent and return a new object.

So any chance you could tweak this function so that it doesn't change the input parameter? I think just newOutput = { ...output, pluginAssets: output.slice() } should get you there?

[deepampriyadarshi]

The problem is that outputAssetsToExternalAssets_ is modifying the input - and that should not happen because all functions should ideally be idempotent and return a new object.

Yes that's beacuse in general, object and arrays are passed by reference (more technically Call by sharing) and not by value, unlike primitive data types. Therefore we need to make separate Deep-Copy of objects which was not happening earlier in let output = { ...this.allProcessedAssets(allRules, options.theme, options.codeTheme) };

Clarifications

So do you want me to keep this => let output = JSON.parse(JSON.stringify(this.allProcessedAssets(allRules, options.theme, options.codeTheme))); as well as modify outputAssetsToExternalAssets_
                         OR
Just modify outputAssetsToExternalAssets_?

[laurent22]

Yes modify outputAssetsToExternalAssets_. If you do as I've shown above, by creating a copy of the relevant properties, it should work fine

[deepampriyadarshi]

Done.. All the expected functionalities are retained, nothing broke... Hopefully it resolves the issue

[laurent22]

Yes hopefully that should be fine. Duplicating objects using spread and slice is the preferred way because most likely faster than JSON parse/serialize and it feels more proper too. You'll see that a lot in the reducers for instance.

[deepampriyadarshi]

Duplicating objects using spread and slice is the preferred way because most likely faster than JSON parse/serialize and it feels more proper too. You'll see that a lot in the reducers for instance.

Totally agreed. I was least bothered about performance. Was more focused on bringing up this cache inconsistency to your notice.

[deepampriyadarshi]

Hi @laurent22 my recent commit a351969 has all the necessary refactoring that you asked. If you feel it's okay to merged then do notify me. I will squash up all the intermediate commits to clean up the commit tree.

[laurent22]

Thanks for the detailed @deepampriyadarshi, I appreciate although that might take me a while to review this!

[laurent22]

Thanks for updating this. Any change to add a test that would prove that modifying the output of this function doesn't modify the cached value?

[deepampriyadarshi]

Yeah sure. There's a test file packages/app-cli/tests/MdToHtml.ts, should I make the necessary changes in that? Other than this there's no other test file related to MdToHtml.ts. I guess this is the test file that runs on running yarn test from root directory.

[laurent22]

Yes this file sounds good

[deepampriyadarshi]

Thanks for updating this. Any change to add a test that would prove that modifying the output of this function doesn't modify the cached value?

Done 👍

[deepampriyadarshi]

Hi @laurent22 can this PR be merged?

[laurent22]

I've recently implemented this to fix an issue when the theme is automatically switched between dark and light: d1e545a And as far as I can see it also fixed the issue in this PR, isn't it?

[deepampriyadarshi]

Yes except the cache inconsistency one...
So i will close this PR now.

[laurent22]

Actually the cache consistency one is a good fix, any chance you could add just this to a PR? (I'd re-open this one but looks like the source branch is deleted)