Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Excessive use of JSON stringification in PTree file format #106

Open
dracorpg opened this issue Jun 6, 2021 · 3 comments
Open

Excessive use of JSON stringification in PTree file format #106

dracorpg opened this issue Jun 6, 2021 · 3 comments

Comments

@dracorpg
Copy link
Contributor

dracorpg commented Jun 6, 2021

By looking into a .ptree file with a text editor, I noticed - cf #98 (comment) - that PTree doesn't dump a JSON representation of its complete object hierarchy, but rather nests JSON string representations at each hierarchical level (resulting in massive amounts of quote escaping).
Thus the beginning of a .ptree file looks like this: {"tree":"{\"item_list\":[\"{\\\"id\\\":0,\\\"type\\\":\\\"root\\\".... This not only really feels wrong and inflates the file size, it also impairs the interoperability of PTree by making it much more painful to write software that would parse the contents of a .ptree file.

The culprit is the use of JSON string-ification at several steps of its internal object structure saving/loading system. Internally, the entire code base should manipulate hierarchies of plain JS Objects and only ever make use of JSON.stringify / JSON.parse at a single point: when writing/reading the structure into/from a file on disk.

Unnecessary round trips through the JSON formatter/parser are also used internally for most IPC requests (sending data to/from popup windows) which are perfectly able to work with JS object hierarchies as arguments.

I know changing this isn't much fun, so I'm only creating this issue now because I've spent some time working on it seriously :)
(I still have to implement read compatibility with the previous format, though, so the PR's not ready yet)

@smariel
Copy link
Owner

smariel commented Jun 20, 2021

This is because old versions of Electron didn't handled Javascript Objects in inter-process communication (they were serialized, resulting in the lost of all methods, etc.). The entire PTree has been built with stringification. Electron has now greatly evolved and PTree may be rewriten. This would simplify communication between process and windows, simplify saves, loads and copies, etc. But it's a huge work for a major version with potential bugs and broken compatibility.

@dracorpg
Copy link
Contributor Author

dracorpg commented Jun 20, 2021

I understand where you're coming from.

However, note that the changes I made in the linked PR #107 (now ready to merge, or so I believe) are actually quite minimal. There is no more reliance on IPC passing of full-featured PTree internal objects (with methods etc. coming from their prototypes) than there is currently. Only plain object attributes (i.e. what is preserved when serializing to JSON strings then parsing the result) are accessed, the code just doesn't explicitely perform JSON encoding/parsing steps (except when saving/loading to/from disk, of course).

Actually (from my admittedly limited understanding of JS, so I may be wrong on this) this should work just as well on the older versions of Electron which handled the IPC through JSON serialization as it does on the current ones that use the Structured Clone algorithm :)

@smariel smariel added this to the v2.0.2 milestone Oct 23, 2021
@smariel smariel closed this as completed Oct 23, 2021
@smariel smariel reopened this Oct 23, 2021
@smariel smariel removed this from the v2.0.2 milestone Oct 23, 2021
@smariel
Copy link
Owner

smariel commented Oct 24, 2021

Unfortunately the PR #107 introduced a regression with the undo function. I had no time to fix it before the last release (v2.0.2) so I reverted the commit. Feel free to take a second look at it because it's still a good idea. Thanks for your comprehension ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants