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

feat: Make SVGs "Penrose-editable" #1171

Merged
merged 27 commits into from Dec 14, 2022

Conversation

lmxlucy
Copy link
Contributor

@lmxlucy lmxlucy commented Dec 10, 2022

Description

Related issue/PR: #1129

Provide editability of any (SVG) diagram created in Penrose.

  1. SVGs exported from Penrose contain (in metadata) the entire source code used to generate the diagram (Domain, Style, and Substance), along with the seed and software version.
  2. The Penrose IDE can directly import an SVG, and automatically import this metadata.

Implementation strategy and design decisions

Penrose SVG Export:

  1. Figure out where SVGs are exported
  2. Programmatically retrieve the Penrose version, variation string, substance code, style code, and domain code
  3. Add this code to the exported SVG metadata

Penrose SVG Import:

  1. Figure out how to upload SVG (dropzone in a new SVG tab)
  2. Figure out how to load trios (focus on In-browser storage)
  3. Extract trios content from SVG and load this code to trios in browser

Examples with steps to reproduce them

Export Penrose SVG

  1. Load word-cloud trios from Roger
  2. Click Compile to compile diagram from triosScreen Shot 2022-12-09 at 8 41 38 PM
  3. Click SVG to export the diagram in svg
  4. Open the exported svg in text editor to see added Penrose tagScreen Shot 2022-12-09 at 8 44 45 PM

Import Penrose SVG

  1. For comparison purpose, load walk-on-spheres from Roger and compileScreen Shot 2022-12-09 at 8 45 33 PM
  2. Upload the word-cloud svg downloaded above in the svg tab and CompileScreen Shot 2022-12-09 at 8 48 13 PM
  3. Check trios content and the newly compiled diagram match the uploaded svg (the titles of trios stay as walk-on-spheres)Screen Shot 2022-12-09 at 8 48 49 PM

Checklist

  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new ESLint warnings
  • I have reviewed any generated changes to the diagrams/ folder

@lmxlucy lmxlucy changed the title feat: Penrose editable svgs feat: Make SVGs "Penrose-editable" Dec 10, 2022
@wodeni wodeni linked an issue Dec 10, 2022 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Dec 10, 2022

Codecov Report

Merging #1171 (1388c70) into main (3c9b12b) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1171   +/-   ##
=======================================
  Coverage   62.07%   62.07%           
=======================================
  Files          59       59           
  Lines        7111     7111           
  Branches     1708     1708           
=======================================
  Hits         4414     4414           
  Misses       2614     2614           
  Partials       83       83           
Impacted Files Coverage Δ
packages/core/src/renderer/Renderer.ts 25.92% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@wodeni wodeni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall! Thanks again for submitting the PR 🎉!

I only have a few minor comments that might improve the UX a little bit. I tested the new feature locally and everything seems to run very smoothly. For testing, it might be useful to start adding some tests to editor for this (e.g. serializing all diagrams from the registry and loading them again). This might require a bit more changes and is probably worth another PR.

packages/core/src/renderer/Renderer.ts Outdated Show resolved Hide resolved
packages/editor/src/components/DiagramPanel.tsx Outdated Show resolved Hide resolved
packages/editor/src/App.tsx Outdated Show resolved Hide resolved
Copy link
Member

@wodeni wodeni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for cleaning the diff! Small comments on variation loading below.

packages/editor/src/components/SvgUploader.tsx Outdated Show resolved Hide resolved
@wodeni wodeni self-requested a review December 13, 2022 04:15
Copy link
Member

@wodeni wodeni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! variation works correctly now. I think it'd be great if we auto-compile uploaded diagrams as a finishing touch. Also a small comment on calls to trim, not a functional issue, just want to future-proof this.

packages/editor/src/components/SvgUploader.tsx Outdated Show resolved Hide resolved
packages/editor/src/components/SvgUploader.tsx Outdated Show resolved Hide resolved
packages/editor/src/components/SvgUploader.tsx Outdated Show resolved Hide resolved
@lmxlucy
Copy link
Contributor Author

lmxlucy commented Dec 13, 2022

I think it'd be great if we auto-compile uploaded diagrams as a finishing touch.

I tried to auto-compile diagram after upload. I tried compileDiagram inside handleChange both inside(after current workspace is updated with uploaded trios) and after reader.onload but diagram was not compiled and no error was displayed. I also did not find other events appropriate for compileDiagram. I am not sure what the problem is. Do you have any ideas on what to do?

@wodeni
Copy link
Member

wodeni commented Dec 13, 2022

I think it'd be great if we auto-compile uploaded diagrams as a finishing touch.

I tried to auto-compile diagram after upload. I tried compileDiagram inside handleChange both inside(after current workspace is updated with uploaded trios) and after reader.onload but diagram was not compiled and no error was displayed. I also did not find other events appropriate for compileDiagram. I am not sure what the problem is. Do you have any ideas on what to do?

My guess is that you did not await for compileDiagram. The function is asynchronous (for retrieving math labels and resolving images mostly) and callers need to wait for it to complete. Your onload will need to be async too probably.

@lmxlucy
Copy link
Contributor Author

lmxlucy commented Dec 13, 2022

I think it'd be great if we auto-compile uploaded diagrams as a finishing touch.

I tried to auto-compile diagram after upload. I tried compileDiagram inside handleChange both inside(after current workspace is updated with uploaded trios) and after reader.onload but diagram was not compiled and no error was displayed. I also did not find other events appropriate for compileDiagram. I am not sure what the problem is. Do you have any ideas on what to do?

My guess is that you did not await for compileDiagram. The function is asynchronous (for retrieving math labels and resolving images mostly) and callers need to wait for it to complete. Your onload will need to be async too probably.

Thank you!! Good to go now!!

Copy link
Member

@wodeni wodeni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final thing: you can simply await the function call. Looking forward to having this in main!

packages/editor/src/components/SvgUploader.tsx Outdated Show resolved Hide resolved
Co-authored-by: Wode "Nimo" Ni <woden@cs.cmu.edu>
@lmxlucy
Copy link
Contributor Author

lmxlucy commented Dec 14, 2022

Final thing: you can simply await the function call. Looking forward to having this in main!

Thank you!

@wodeni wodeni self-requested a review December 14, 2022 06:12
Copy link
Member

@wodeni wodeni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonderful 💯! Merging if CI passes.Thanks again for the PR!

@wodeni
Copy link
Member

wodeni commented Dec 14, 2022

Looks like format is not passing. @lmxlucy do you mind running yarn format and push again?

@wodeni
Copy link
Member

wodeni commented Dec 14, 2022

Looks like something is off with your prettier configuration. Be sure to run yarn format at the repo root, and do not use a global installation of prettier (I guess that's the reason why the diffs have conflicts).

Remove package-lock.json. Looks like this is a byproduct of running npm install, which we don't use.

@lmxlucy
Copy link
Contributor Author

lmxlucy commented Dec 14, 2022

Looks like something is off with your prettier configuration. Be sure to run yarn format at the repo root, and do not use a global installation of prettier (I guess that's the reason why the diffs have conflicts).

I did run it at repo root and my prettier version:
Screen Shot 2022-12-14 at 12 55 12 PM

Remove package-lock.json. Looks like this is a byproduct of running npm install, which we don't use.

That's my bad. I might have accidentally did it on this repo when I meant to do it on others. Removed.

@wodeni
Copy link
Member

wodeni commented Dec 14, 2022

Looks like we have prettier set at 2.2.1, not 2.3.0.

"prettier": "2.2.1",

Might wanna reset your build?

@lmxlucy
Copy link
Contributor Author

lmxlucy commented Dec 14, 2022

Looks like we have prettier set at 2.2.1, not 2.3.0.

"prettier": "2.2.1",

Might wanna reset your build?

I changed my prettier version to 2.3.0 and cleaned my build. But there seem to still be conflicts

@wodeni
Copy link
Member

wodeni commented Dec 14, 2022

Looks like we have prettier set at 2.2.1, not 2.3.0.

"prettier": "2.2.1",

Might wanna reset your build?

I changed my prettier version to 2.3.0 and cleaned my build. But there seem to still be conflicts

To clarify: 2.2.1 is the expected version. Try downgrading or refreshing the build again?

@wodeni wodeni merged commit edb5dc8 into penrose:main Dec 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make SVGs "Penrose-editable"
5 participants