Conversation
- Does not work with ephemeral refractor models
Move suit, rank, card, and hand type definitions, along with utility functions like card_to_exp, string_to_suit, and conversion helpers, from CardProj.re and CardRenderer.re into new CardTypes and CardSyntax modules. This reduces code duplication and improves maintainability without altering functionality.
Implement on_mousedown event handlers for Singleton and Hand card views to toggle between Choose, Flipped, and Show modes. Double-click or shift-click adjusts mode accordingly, enhancing user interaction by allowing direct mode changes via mouse events.
Add CalculatorRenderer.rei defining types for calculator operations, state, and actions, including RichProbe integration. Update ProbeProj.re to reference CalculatorRenderer directly instead of CalculatorRenderer.M for correct projector registration.
- Enhanced renderer selection to only apply a renderer if it can handle the current expression, preventing potential rendering failures when expressions change. - Updated switch case from specific `Some(renderer)` to guarded `Some(renderer) when renderer.can_handle(exp)`, and changed fallback from `None` to `_` for clarity.
…r CalculatorRenderer and CardRenderer - Add `value` types (`int` for Calculator, `state` for Card) - Implement `parse` functions to extract values from expressions - Refactor `init` functions to accept parsed values instead of expressions - Update `render` functions to use pre-parsed values; remove redundant parsing - Reorder type definitions for clarity in CalculatorRenderer
…low for term creation
| Refractors.to_projector( | ||
| Option.value( | ||
| TermData.segment(id, term_data) | ||
| |> Option.map(Segment.unparenthesize) | ||
| |> Option.map(Segment.trim_secondary(Left)) | ||
| |> Option.map(Segment.trim_secondary(Right)) | ||
| |> Option.map(Segment.parenthesize), | ||
| ~default= | ||
| Base.Secondary({ | ||
| id: Id.invalid, | ||
| content: Whitespace(""), | ||
| }), | ||
| ), | ||
| id, | ||
| entry, | ||
| ); |
There was a problem hiding this comment.
@disconcision I need the syntax on refractors so I can use lift_syntax to rewrite the underlying syntax. This is how I have it working after all the recent changes.
The old way I was doing it is here:
hazel/src/web/app/common/ProjectorView.re
Lines 390 to 399 in e9ebe54
I was similarly pulling the segment out of the termdata map. The old ProjectorCore took a segment and the new on takes a piece so I'm parenthesizing it. I'm also having to trim secondary to avoid issues with parsing it to a term in lift_syntax.
I'm not sure if you have thoughts here on a better way to do this.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 58 out of 58 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| | Some(z) => Ok(z) | ||
| | None => Error(Cant_project) | ||
| }; | ||
| | (None, None) => assert(false) |
There was a problem hiding this comment.
The assert(false) on line 241 indicates a case that is expected to never happen (when a refractor is being processed but neither manual nor ephemeral refractor exists). However, using assert(false) causes the program to crash at runtime if this assumption is violated. Consider using a more graceful error handling approach, such as returning Error(Cant_project) or adding a descriptive error message explaining what invariant was violated.
| | (None, None) => assert(false) | |
| | (None, None) => | |
| // Invariant violation: refractor-kind projector without manual or ephemeral refractor. | |
| // Fail gracefully instead of crashing. | |
| Error(Cant_project) |
| | _ => | ||
| failwith("TableRenderer: apply_transformation: not an expression"), | ||
| info.syntax, | ||
| ) | ||
| ) { | ||
| | Some(s) => s | ||
| | None => failwith("TableRenderer: apply_transformation: lift failed") |
There was a problem hiding this comment.
The failwith calls on lines 163 and 168 will crash the application if the syntax is not an expression or if lift_syntax fails. This could happen with malformed data or unexpected edge cases. Consider returning option types or Result types and handling errors gracefully at the call site, especially since this is user-facing functionality that modifies syntax interactively.
| | _ => | |
| failwith("TableRenderer: apply_transformation: not an expression"), | |
| info.syntax, | |
| ) | |
| ) { | |
| | Some(s) => s | |
| | None => failwith("TableRenderer: apply_transformation: lift failed") | |
| | other => | |
| /* If the syntax node is not an expression, leave it unchanged. */ | |
| other, | |
| info.syntax, | |
| ) | |
| ) { | |
| | Some(s) => s | |
| | None => | |
| /* If lifting fails, fall back to the original syntax instead of crashing. */ | |
| info.syntax |
| "CalculatorRenderer: apply_arithmetic_operation: not an expression", | ||
| ), | ||
| info.syntax, | ||
| ) | ||
| ) { | ||
| | Some(s) => s | ||
| | None => | ||
| failwith("CalculatorRenderer: apply_arithmetic_operation: lift failed") |
There was a problem hiding this comment.
Similar to TableRenderer, the failwith calls on lines 100 and 107 will crash the application. These should handle errors more gracefully, especially since the calculator is an interactive user-facing feature. Return option or Result types and handle errors at the UI level.
| | None => failwith("TextArea: get: Not a table") | ||
| } | ||
| | None => failwith("TextArea: get: Not a table") |
There was a problem hiding this comment.
The error messages on lines 58 and 60 say "TextArea: get: Not a table" but this is in TableProj.re, not TextAreaProj.re. The error message should be corrected to "TableProj: get: Not a table" to accurately identify where the error originated.
| | None => failwith("TextArea: get: Not a table") | |
| } | |
| | None => failwith("TextArea: get: Not a table") | |
| | None => failwith("TableProj: get: Not a table") | |
| } | |
| | None => failwith("TableProj: get: Not a table") |
src/haz3lcore/pretty/ExpToSegment.re
Outdated
| let project_table_if = (should_project, pieces) => | ||
| if (should_project) { | ||
| switch (MakeTerm.for_projection([pieces])) { | ||
| | None => failwith("ExpToSegment.fold_if") |
There was a problem hiding this comment.
The error message on line 1054 says "ExpToSegment.fold_if" but it's actually in the project_table_if function. The error message should be updated to "ExpToSegment.project_table_if" to accurately identify the source of the error.
| | None => failwith("ExpToSegment.fold_if") | |
| | None => failwith("ExpToSegment.project_table_if") |
| switch (probe_model_of_sexp(sexp)) { | ||
| | model => model | ||
| | exception _ => {active_renderer: None} |
There was a problem hiding this comment.
The probe_model_of_sexp function on lines 106-110 appears to call itself recursively (probe_model_of_sexp(sexp) calls probe_model_of_sexp). This creates a custom deserialization function that falls back to a default value on exception. However, the pattern match on line 108 (| model => model) is redundant since it just returns the model unchanged. The function could be simplified to just the exception case, or if the intent is to override the generated deserialization, the naming should be clearer to avoid confusion.
| switch (probe_model_of_sexp(sexp)) { | |
| | model => model | |
| | exception _ => {active_renderer: None} | |
| /* Safely deserialize probe_model from sexp, falling back to default on error. */ | |
| switch (sexp) { | |
| | Sexplib.Sexp.List( | |
| [Sexplib.Sexp.List([Sexplib.Sexp.Atom("active_renderer"), ar_sexp])] | |
| ) => | |
| /* Decode active_renderer field; if it raises, fall back to None. */ | |
| switch (oactive_renderer_of_sexp(ar_sexp)) { | |
| | active_renderer => {active_renderer} | |
| | exception _ => {active_renderer: None} | |
| } | |
| /* If the structure doesn't match the expected record layout, use default. */ | |
| | _ => {active_renderer: None} |
| --modal-z: 101; | ||
| } | ||
|
|
||
| /* TABLE TABLES */ |
There was a problem hiding this comment.
The PR title "Probectors" appears to be a portmanteau of "Probe" and "Projector" but this may not be immediately clear to all developers. Consider using a more descriptive title like "Rich Projector Displays for Probed Values" or documenting the term "Probectors" in the codebase if it's intended to be part of the project's terminology.
| action: () => | ||
| Effect.Many([ | ||
| local(CloseMenu), | ||
| parent( | ||
| SetSyntax( | ||
| OptUtil.get_or_fail( | ||
| (left ? "move left" : "move right") ++ " failed", | ||
| move_column(info, dyn_type, h, left), | ||
| ), | ||
| ), | ||
| ), | ||
| ]), |
There was a problem hiding this comment.
The OptUtil.get_or_fail call on line 645 will crash if move_column returns None. Since this is user-facing interactive functionality, the failure should be handled more gracefully - perhaps by showing an error message to the user or disabling the move buttons when the operation is not valid. The current implementation checks can_move_left/can_move_right but then still uses get_or_fail which suggests a logic inconsistency.
| action: () => | |
| Effect.Many([ | |
| local(CloseMenu), | |
| parent( | |
| SetSyntax( | |
| OptUtil.get_or_fail( | |
| (left ? "move left" : "move right") ++ " failed", | |
| move_column(info, dyn_type, h, left), | |
| ), | |
| ), | |
| ), | |
| ]), | |
| action: () => { | |
| let direction_label = left ? "move left" : "move right"; | |
| switch (move_column(info, dyn_type, h, left)) { | |
| | Some(new_syntax) => | |
| Effect.Many([ | |
| local(CloseMenu), | |
| parent(SetSyntax(new_syntax)), | |
| ]) | |
| | None => | |
| JsUtil.alert(direction_label ++ " failed"); | |
| local(CloseMenu) | |
| }; | |
| }, |
| --modal-z: 101; | ||
| } | ||
|
|
||
| /* TABLE TABLES */ |
There was a problem hiding this comment.
The CSS comment "TABLE TABLES" on line 230 appears to be a typo or placeholder. It should probably be "TABLE STYLES" or "TABLE VARIABLES" to match the pattern of other section comments in this file (e.g., "HACKS").
| // @ Test_Editing.tests | ||
| @ Test_AutoProbe.tests | ||
| @ Test_Indentation.tests | ||
| // @ Test_Indentation.tests |
There was a problem hiding this comment.
Tests for Test_Editing and Test_Indentation have been commented out without explanation. If these tests are failing or being temporarily disabled during development, this should be noted in the PR description or the tests should be fixed before merging.
Add max-height constraint (350px) with auto overflow-y to modals containing tables. This prevents modals with large tables from extending beyond viewport height and ensures content remains accessible through scrolling.
…projector-refractor
…projector-refractor
…ize ID membership checks
…projector-refractor
This reverts part of commit 0d6b44e.
…projector-refractor
…projector-refractor
…projector-refractor
Adds a rich "projector-style" display on probed values. Also allows for the underlying syntax to be rewritten by the probed interface. Currently there's a calculator for integers, table editor, and card view.
Calculator
Screencast.From.2025-11-21.11-12-44.webm
Tables
The rich table probe allows for some column actions to rewrite the underlying syntax via function pipelining. If placed on a pattern the table is read-only
Screencast From 2025-11-21 11-16-20.webm