fix: Local image resolution not working in IDE #1037

cmumatt · 2022-05-31T21:35:51Z

Description

Resolves #1035.

This PR implements local serving of images and examples in the IDE. This has two benefits:

It allows trios that are not examples to use the "stock" images within the set of examples, and
Local serving of examples and images eliminates the confusing situation where the local instance of Penrose (i.e., a local build or preview site) has different examples than penrose main.

Implementation strategy and design decisions

During build, examples and images are deployed to vite's public directory (documentation) and are served locally from there.

Open questions

This PR does not implement the gist or upload functionality. That will need to be implemented in a subsequent PR.
Images are written to local storage, but we do not yet have a way to invalidate these resources at this time.

codecov · 2022-05-31T21:37:48Z

Codecov Report

Merging #1037 (015bd2f) into main (073d7b3) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1037   +/-   ##
=======================================
  Coverage   63.48%   63.48%           
=======================================
  Files          59       59           
  Lines        7115     7115           
  Branches     1583     1583           
=======================================
  Hits         4517     4517           
  Misses       2502     2502           
  Partials       96       96

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 073d7b3...015bd2f. Read the comment docs.

cloudflare-pages · 2022-05-31T21:39:39Z

Deploying with Cloudflare Pages

Latest commit:	`015bd2f`
Status:	✅ Deploy successful!
Preview URL:	https://b6be81ed.penrose-72l.pages.dev

View logs

wodeni

I like the idea of serving branch-specific registry. This is useful for debugging and previewing. However, I think a more general solution I mentioned below will take the same LOCs, and make a simpler build script. We really shouldn't run monorepo-level build script in docs-site and need to find a more sustainable option (e.g. CI script, node script, or Cloudflare-supported monorepo deployment whenever it's available). That aside, using local storage will free us from copying the SVG files and open up the path for local file uploads in the future.

wodeni · 2022-06-02T01:12:32Z

packages/examples/src/persistent-homology/persistent-homology.sty

@@ -142,21 +142,21 @@ forall Point p {
    center: p.circle1.center 
    width: p.circle1.r * 2.0
    height: p.circle1.r * 2.0
-    href: "shading.svg"
+    href: "persistent-homology-shading.svg"


I don't follow the renaming here: is this for disambiguating shading.svg?
In general, I still don't think it's a great idea to put all images for all domains in one flat folder. Even if we are serving the examples, we can still keep the folder structure and resolve the image paths by domain-prefix + relative-image-path. No need to move all images to a different folder.

Thanks for reviewing this. As you noted below, once an example is modified, we lose the example root information and no longer know which folder contains the image resource. If there are different, for example, shading.svgs in different folders, it becomes ambiguous which one to choose.

Like I said below, you can use the workspace id to disambiguate across sessions if using local storage.

Actually there's no need to cache image from previous sessions. Within a Style program, images should be unique, so we can cache them when we are in the example phase, and just retrieve the images in local mode.

Thanks and a few clarifying questions since I am new to the new IDE. How are we currently defining session and workspace id? How is the workspace id accessed and when do we generate a new id? In the described workflow, are we requiring a user to load an example with the stock images he or she wants prior to using them in their own diagram? How do we want to invalidate the entries we create in local storage, particularly if they are workspace id specific -- whenever the workspace id changes? Thanks for helping with the additional context!

Thanks and a few clarifying questions since I am new to the new IDE. How are we currently defining session and workspace id? How is the workspace id accessed and when do we generate a new id?

WorkspaceMetadata has the ID of the current workspace, which is roughly identified by a trio of Penrose programs. So whenever you load a new trio, uuid() is called to generate a new ID. You can simply search for all invocations for uuid() to see the call sites. The state management is done by recoil, so maybe look at https://recoiljs.org to see how it works.

In the described workflow, are we requiring a user to load an example with the stock images he or she wants prior to using them in their own diagram?

Not sure if I understand the question. Image loading is done automatically if loading from gist, examples, or roger. Are you talking about the user uploading their own image? In that case, yeah I hope we can build that fairly easily (ie a simple file dialog can be a good starter).

How do we want to invalidate the entries we create in local storage, particularly if they are workspace id specific -- whenever the workspace id changes?

Yeah, except when the workspaces are explicitly saved. You can get the list of these ids from localFilesState.

Thanks, in the latest commit, local storage images are workspace id specific. The editor was creating new uuids when a workspace was saved, which caused saved workspaces to lose their images. I removed the line of code creating the new uuid on workspace save.

It seems it would be better if the images were stored within the Recoil workspace state rather than outside of it. For instance, when a saved workspace is deleted by the user, Recoil would also delete the images from local storage. Right now these images accumulate each time a workspace with images is created and never go away.

My suggestion is to move ahead with this PR since it fixes a bug in production and create an issue to move image resource management into Recoil state.

packages/editor/src/state/atoms.ts

packages/editor/src/components/DiagramPanel.tsx

wodeni

Thanks for making the changes! Now that we are use local storage, I think the svg public folder is no longer necessary. My comments are mostly about removing that. Once that's done, we can undo the file renamings too.

wodeni · 2022-06-03T05:32:17Z

packages/editor/src/components/DiagramPanel.tsx

+          relativePath,
+          new URL(
+            relativePath,
+            window.location.origin + window.location.pathname + "svg/"


Instead of hard-coding the svg/ path, local workspace should only get the image from local storage because the image must came from gist, example, roger, or file upload (this is not implemented but doesn't change the logic) which already cache to local storage when loaded

Removed. This breaks the ability for example images to work for trios that are pasted in, but that doesn't sound like a user case we are supporting right now, which would make this unnecessary.

wodeni · 2022-06-03T05:33:26Z

packages/editor/.gitignore

@@ -8,6 +8,8 @@ pnpm-debug.log*
 lerna-debug.log*

 node_modules
+public/svg


Okay to deploy the example source files, but svgs shouldn't be in the same folder anymore, so this shouldn't be needed.

wodeni · 2022-06-03T05:33:46Z

packages/editor/package.json

@@ -6,9 +6,9 @@
    "start": "vite",
    "dev": "vite",
    "watch": "vite",
-    "clean": "rimraf dist",
+    "clean": "rimraf dist;rimraf public/svg;rimraf public/examples",


No need to have svg folder.

Also do rimraf dist public/examples

wodeni · 2022-06-03T05:34:32Z

packages/editor/package.json

    "typecheck": "tsc --noEmit",
-    "build": "NODE_OPTIONS='--max-old-space-size=8192' vite build",
+    "build": "shx mkdir -p ./public/svg/;shx cp ../examples/src/*/*.svg ./public/svg;shx mkdir -p ./public/examples/;shx cp -r ../examples/src/* ./public/examples/;NODE_OPTIONS='--max-old-space-size=8192' vite build",


Should remove the svg related steps

Serve local images & examples locally

3dec9b0

cmumatt added 4 commits May 31, 2022 17:40

Add examples to editor yarn clean

70ff522

Remvoed debug message. Added shx dep.

d880dad

Retain at least one absolute URL as a test case.

804bb5b

Retain at least one absolute URL as a test case.

68c91b3

cmumatt marked this pull request as ready for review June 1, 2022 16:35

cmumatt requested review from joshsunshine, maxkrieger and wodeni June 1, 2022 16:36

wodeni requested changes Jun 2, 2022

View reviewed changes

cmumatt added 4 commits June 2, 2022 10:18

Use base URL in local fetch

c3cab30

Use URL class for resolving registry

1bd1fd7

Use URL class for resolving examples

0d5eeb6

Added local storage caching

882eacc

cmumatt mentioned this pull request Jun 2, 2022

Allow registry trios that are not IDE examples #1034

Closed

cmumatt requested a review from wodeni June 2, 2022 21:36

Merge remote-tracking branch 'origin/main' into fix-1035

f49bf74

wodeni requested changes Jun 3, 2022

View reviewed changes

cmumatt added 2 commits June 3, 2022 14:25

Remove ability to retrieve local images via url

7457346

Make imaged workspace-id specific

015bd2f

cmumatt merged commit c5220b4 into main Jun 3, 2022

This was referenced Jun 3, 2022

fix: Absolute URLs in WOS example #1042

Closed

Local image state should be managed by workspace #1043

Open

cmumatt deleted the fix-1035 branch June 5, 2022 19:19

cmumatt linked an issue Jun 13, 2022 that may be closed by this pull request

fix: docs-site build assumes *ix-like script environments #917

Closed

cmumatt mentioned this pull request Jun 13, 2022

fix: docs-site build assumes *ix-like script environments #917

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Local image resolution not working in IDE #1037

fix: Local image resolution not working in IDE #1037

cmumatt commented May 31, 2022 •

edited

codecov bot commented May 31, 2022 •

edited

cloudflare-pages bot commented May 31, 2022 •

edited

wodeni left a comment

wodeni Jun 2, 2022

cmumatt Jun 2, 2022 •

edited

wodeni Jun 2, 2022

wodeni Jun 2, 2022

cmumatt Jun 2, 2022

wodeni Jun 3, 2022

cmumatt Jun 3, 2022

wodeni left a comment

wodeni Jun 3, 2022 •

edited

cmumatt Jun 3, 2022

wodeni Jun 3, 2022

cmumatt Jun 3, 2022

wodeni Jun 3, 2022

wodeni Jun 3, 2022

cmumatt Jun 3, 2022

wodeni Jun 3, 2022

cmumatt Jun 3, 2022

fix: Local image resolution not working in IDE #1037

fix: Local image resolution not working in IDE #1037

Conversation

cmumatt commented May 31, 2022 • edited

Description

Implementation strategy and design decisions

Open questions

codecov bot commented May 31, 2022 • edited

Codecov Report

cloudflare-pages bot commented May 31, 2022 • edited

Deploying with Cloudflare Pages

wodeni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmumatt Jun 2, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wodeni left a comment

Choose a reason for hiding this comment

wodeni Jun 3, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmumatt commented May 31, 2022 •

edited

codecov bot commented May 31, 2022 •

edited

cloudflare-pages bot commented May 31, 2022 •

edited

cmumatt Jun 2, 2022 •

edited

wodeni Jun 3, 2022 •

edited