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: display errors in the Simple component #953

Merged
merged 6 commits into from May 10, 2022
Merged

feat: display errors in the Simple component #953

merged 6 commits into from May 10, 2022

Conversation

wodeni
Copy link
Member

@wodeni wodeni commented May 9, 2022

Description

Related issue/PR: #535, #671

Simple is a raw canvas for Penrose-generated diagrams. In the past iterations, the component didn't handle errors and rendered a blank canvas. In this PR, whenever Simple encounters an error, it renders the pretty-printed error message directly on canvas.

Implementation strategy and design decisions

  • Remove memoize from CollectLabels
  • Keep error in the state of Simple. This is mostly to let React handle re-rendering, which is a different case from penroseState (?)
  • Used error rendering code from browser-ui
  • Added errors to the storybook example
  • Change Edgeworth to use Simple (see below)

Examples with steps to reproduce them

In synthesizer-ui, I changed Gridbox so it's just a wrapper around Simple, a few minor notes:

  • Gridbox has an overlay for mutation info and used to conditionally render the Penrose diagram whenever the user toggles the overlay. When using <Simple>, this can be very slow because re-rendering will also run the Penrose optimizer. Instead, I always keep <Simple> in the parent div, and only stack the info div on top of it once toggled.
  • In terms of the behavior of Edgeworth, the system seems to render all diagrams at once after loading on a blank screen for a while, not sure if this is new.
  • See below: "mutated program 3" show an error

image

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

Open questions

Questions that require more discussion or to be addressed in future development:

@codecov
Copy link

codecov bot commented May 9, 2022

Codecov Report

Merging #953 (60fda21) into main (4c428cf) will decrease coverage by 0.08%.
The diff coverage is 86.66%.

@@            Coverage Diff             @@
##             main     #953      +/-   ##
==========================================
- Coverage   63.19%   63.10%   -0.09%     
==========================================
  Files          62       62              
  Lines        7930     7911      -19     
  Branches     1827     1826       -1     
==========================================
- Hits         5011     4992      -19     
  Misses       2795     2795              
  Partials      124      124              
Impacted Files Coverage Δ
packages/core/src/compiler/Style.ts 79.07% <ø> (ø)
packages/core/src/utils/CollectLabels.ts 66.32% <86.66%> (-0.35%) ⬇️
packages/core/src/engine/Optimizer.ts 81.41% <0.00%> (-0.75%) ⬇️
packages/core/src/index.ts 70.06% <0.00%> (-0.41%) ⬇️

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 4c428cf...60fda21. Read the comment docs.

@cloudflare-pages
Copy link

cloudflare-pages bot commented May 9, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 60fda21
Status: ✅  Deploy successful!
Preview URL: https://3ffd57f2.penrose-panes.pages.dev

View logs

Copy link
Collaborator

@samestep samestep left a comment

Choose a reason for hiding this comment

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

Awesome, thanks!

I tried running synthesizer-ui locally (to compare this branch with main) and had a couple questions:

  1. When I ran it on main, I got an error in Mutated Program #5, but when I ran it on this branch, the error was instead in Mutated Program #4. Is the synthesizer nondeterministic?
  2. For the diagrams that don't error, I can click anywhere on the box to see the mutation overlay, but for the diagrams that do error, this only works if i click above the bottom of the error message. Could you change it so that errored diagrams let you click anywhere on the box to see the mutation overlay?

*/
const tex2svg = memoize(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you clarify why you removed this memoize?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, #535 noted this issue, but there's no PR that fixes it. This fix doesn't close the issue though, because when downloading the SVGs (which is currently broken in Edgeworth, to be addressed in another PR) the labels still disappear.

packages/core/src/compiler/Style.ts Show resolved Hide resolved
@wodeni
Copy link
Member Author

wodeni commented May 10, 2022

When I ran it on main, I got an error in Mutated Program #5, but when I ran it on this branch, the error was instead in Mutated Program #4. Is the synthesizer nondeterministic?

Yes, I pass in a randomized seed to the synthesizer, which is the expected behavior of this GUI, as users expect different diagrams every time they hit Generate diagrams.

For the diagrams that don't error, I can click anywhere on the box to see the mutation overlay, but for the diagrams that do error, this only works if i click above the bottom of the error message. Could you change it so that errored diagrams let you click anywhere on the box to see the mutation overlay?

Correct. I finally learned that flexbox and height: 100% does the trick for me. The most recent commit includes that fix.

@samestep
Copy link
Collaborator

Makes sense, thanks!

@wodeni wodeni merged commit aa6209f into main May 10, 2022
@wodeni wodeni deleted the simple-error branch May 24, 2022 18:36
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.

None yet

2 participants