ngl icon indicating copy to clipboard operation
ngl copied to clipboard

fix: don't dispose structure

Open sto3psl opened this issue 1 year ago • 1 comments

This is a fix for a regression introduced in 3405abbd0a8ec1de35aa63d0e90126beb5d07b44.

I went through the code and noticed that in commit 3405abbd0a8ec1de35aa63d0e90126beb5d07b44 the file structure-representation.ts was changed. Even though the commit was supposed to only update new Typescript rules it changed behaviour, causing the bug.


When updating ngl in our project from 2.0.0-dev.39 to 2.0.0-dev.40 I noticed that adding new representations after removing old ones didn't work anymore. The old representations were simply removed and new ones didn't appear in the viewer.

It is working as expected in version 2.0.0-dev.39 as you can see in this Codesandbox:
https://codesandbox.io/s/pedantic-dan-z9ycdv

https://user-images.githubusercontent.com/5798652/183840658-6c47f0d5-4a7a-4844-b015-57cb7e3bd640.mp4

After installing version 2.0.0-dev.40 (it happens in 2.0.0 too but that isn't available on npm) it looks like this: https://codesandbox.io/s/upbeat-danny-o5td2v

https://user-images.githubusercontent.com/5798652/183840902-874b6e74-e0d1-413a-840d-a3660bec973e.mp4

My change keeps the TS change and restores the previous behaviour.

sto3psl avatar Aug 10 '22 07:08 sto3psl

Interesting. The original code deleted both the structure and the view; that change switched to disposing them both. I expect your fix won't cause any problems directly, but it would be good to understand whether it could cause a memory leak (keeping a ref to the structure) before merging.

garyo avatar Aug 14 '22 13:08 garyo

I ran some benchmarks in Chrome by taking multiple heap snapshots in the Memory Tab of Devtools. I made a page with the fixed ngl version that removes and adds representations every 200ms and let it run over several minutes. Memory usage did not increase, so I assume the QC is doing it's job.

grafik

RepresentationElement visible in the screenshot never changes in shallow or retained size.

sto3psl avatar Aug 24 '22 11:08 sto3psl

Good spot, thanks @sto3psl and @garyo for reviewing. The StructureRepresentation instance owns a StructureView instance, created here (which provides the selection behaviour for the representation) and it makes sense to dispose that when clearing up the representation, but the StructureRepresentation doesn't own the Structure instance so calling dispose on that is indeed a bug.

fredludlow avatar Sep 12 '22 12:09 fredludlow