Set output evaluation context for a single node #8440

somebody1234 · 2023-12-01T13:09:58Z

Pull Request Description

Closes Setting evaluation context on a single node. #8072
- Implement handlers for the corresponding buttons on the circular menu
- Add missing icons and styles
- Add functionality to match and extract ASTs

Important Notes

None

Screenshots

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

~~The documentation has been updated, if necessary.~~
~~Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.~~
All code follows the
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
All code has been tested:
- ~~Unit tests have been written where possible.~~
- ~~If GUI codebase was changed, the GUI was tested when built using ./run ide build.~~

…mplementation for AST replacement Untested The matched segments also need to be hidden to, but we need to make sure that the propagated content edits have the correct offset.

…text in the future

vitvakatu · 2023-12-05T09:12:51Z

A little issue in logic:

Kapture.2023-12-05.at.13.07.29.mp4

When using the button near the node, context switch expressions should never stack; instead, we should replace the existing expression with the correct one. Having a “no-op” expression, like enabling output context in the “live” environment is totally okay, and should be considered as no context switch expression present on a node.

somebody1234 · 2023-12-05T10:52:57Z

@vitvakatu Hmm... that would mean you can't have different overrides for different environments, right?

vitvakatu · 2023-12-05T11:14:42Z

@vitvakatu Hmm... that would mean you can't have different overrides for different environments, right?

Yes, this is how the old GUI is implemented. We can confirm with @jdunkerley to make sure. I tried to find the actual discussion around this behavior but failed (there were a lot of drastic changes to the design).

I’m sure that any operation on button shouldn’t cause the change in the visible node’s expression.

somebody1234 · 2023-12-05T11:18:05Z

I’m sure that any operation on button shouldn’t cause the change in the visible node’s expression.

@vitvakatu note that there are alternatives that still preserve this behavior - I think ideally we would recursively hide all context overrides, but only modify the override for the current context (or add one, if none are for the current context)

somebody1234 · 2023-12-05T11:21:00Z

@Frizi thoughts on using widgets for this instead? i think it'd be a cleaner (+ more generic?) solution, but the issue is that we have to communicate the "is node overridden" data all the way up the tree back to the GraphNode (or via an inject) - which is doable, but doesn't exist yet

farmaazon

Any chance for output context overwriting tests? They could be E2E tests if UT would be hard, and they can be done in a separate PR.

app/gui2/src/components/GraphEditor/GraphNode.vue

app/gui2/src/util/ast/string.ts

somebody1234 · 2023-12-05T11:38:53Z

i'll see what i can do for UT

Warnings about `onScopeDispose` being called outside an effect scope Errors caused by unhandled promise rejections

somebody1234 · 2023-12-05T16:17:58Z

Re-requesting review from @farmaazon for feedback on UT: Implementation

somebody1234 · 2023-12-07T09:20:48Z

i think the new approach is a huge bandaid. but for now it works, and hopefully most of the jank can be removed once we get real AST edits?

somebody1234 · 2023-12-07T09:21:34Z

oh yeah, one minor (okay, major) issue: for now at least, i've made the new AST types public, because i need to read and mutate them outside of abstract.ts

somebody1234 · 2023-12-07T11:41:44Z

requesting review from @kazcw because of ast-related changes... maybe this task should be delayed until we have edits?

kazcw

I will need to make some changes to the AST-editing support here to fit my planned direction for the APIs, but I think it's OK to merge; I can update it in-tree in my next PR.

…into ast-edits

Frizi · 2023-12-08T14:23:52Z

app/gui2/mock/index.ts

+  return mockNode()
+}
+
+export function waitForMainModule(projectStore?: ProjectStore) {


I can't find any usages of this function. Can it be removed?

was required previously when I used component tests for GraphNode

Frizi · 2023-12-08T14:26:16Z

app/gui2/mock/providers.ts

+import { Rect } from '../src/util/rect'
+import { Vec2 } from '../src/util/vec2'
+
+export const graphNavigator: GraphNavigator = {


Why does this have to be mocked? I feel like no providers should need any mocking, except for things related to network requests.

previously we had component tests for GraphNode... I guess the correct solution is maybe to use a wrapper component then?

but also, we're no longer doing component tests so it's not like these are currently used either.

Frizi · 2023-12-08T14:27:36Z

app/gui2/mock/vue.ts

+
+// It is currently not feasible to use generics here, as the type of the component's emits
+// is not exposed.
+export function handleEmit(wrapper: VueWrapper<any>, event: string, fn: (...args: any[]) => void) {


Is this used anywhere?

as above, previously used for component tests. i figured it's fine to keep it if we may want to do component testing in the future, but i have no problems with removing all unused code because we are probably not planning to do that anytime soon anyway.

Frizi · 2023-12-08T14:59:37Z

app/gui2/src/util/ast/match.ts

+    placeholder,
+  )
+  if (replacements.length !== 0) {
+    // throw new Error(`${replacements.length} unused replacements.`)


Is this error commented out unintentionally?

no - nice catch, thanks, will fix.

somebody1234 · 2023-12-08T15:41:55Z

For posterity: the additional logs added on errors were added because Vitest was throwing "unhandled exception" errors .

Note also that the fix that worked there, was to add mocks for the LS and data server.

app/gui2/src/util/ast/match.ts

somebody1234 added 4 commits December 1, 2023 18:46

isMatch function to match CST subtrees

ce332a1

(broken) Detect current Output Context enabled state

391c526

Implement extractMatches to extract CST subtrees

a23dddc

Interactivity for output context override in Circular Menu; initial i…

a079f66

…mplementation for AST replacement Untested The matched segments also need to be hidden to, but we need to make sure that the propagated content edits have the correct offset.

somebody1234 added CI: No changelog needed Do not require a changelog entry for this PR. x-new-feature Type: new feature request -gui labels Dec 1, 2023

somebody1234 requested a review from Frizi December 1, 2023 13:10

somebody1234 added 7 commits December 2, 2023 00:51

Basic tests for string escape

f3fa62f

Attempt to hide overridden output context expressions from GraphNode

ae6f53d

Merge branch 'develop' into wip/sb/set-eval-ctx

9ea511b

Fix CSS for CircularMenu

b658ef6

Fix AST match extraction by using parseLine instead of parse

32e7b28

Fix inserted text

70425c2

Refactor to make it easier to fix node icons for overriden output con…

c5bf8f6

…text in the future

somebody1234 marked this pull request as ready for review December 4, 2023 10:19

Only allow a single output context override

09dca91

farmaazon approved these changes Dec 5, 2023

View reviewed changes

Address review

48e9a55

somebody1234 added 2 commits December 6, 2023 01:06

Initial component test for GraphNode including execution mode override

fbc457e

Warnings about `onScopeDispose` being called outside an effect scope Errors caused by unhandled promise rejections

Get rid of unhandledRejection errors

882c23b

somebody1234 requested a review from farmaazon December 5, 2023 16:17

More consistent handling of default

d163b54

somebody1234 requested a review from kazcw December 7, 2023 11:41

somebody1234 added 3 commits December 7, 2023 22:13

Move ast/string to ast/text

795f836

Fix text escaping

55e97bc

Fix error that only occurs on built package

5575023

kazcw approved these changes Dec 7, 2023

View reviewed changes

kazcw added 2 commits December 8, 2023 05:05

Merge remote-tracking branch 'origin/wip/kw/parse-inline-signatures' …

351eea7

…into ast-edits

Merge remote-tracking branch 'origin/wip/sb/set-eval-ctx' into ast-edits

7786b70

Frizi approved these changes Dec 8, 2023

View reviewed changes

kazcw added 8 commits December 10, 2023 19:28

wip

ba211b0

Merge remote-tracking branch 'origin/develop' into ast-edits

2b038df

wip

4362fc8

Pass tests

9888941

Merge commit '0974d5ff1e1729148e7b74c98c82e03a5ad8a751' into ast-edits

41d4172

Merge commit '3d76459ba7079c6bd115bbfec2b0911881e527e5' into ast-edits

e71c60b

Merge commit 'be2ae3279cbfddf30007f2007b0aaa19cfa14891' into ast-edits

8aa3e0e

Merge remote-tracking branch 'origin/develop' into ast-edits

0ded00d

somebody1234 commented Dec 13, 2023

View reviewed changes

app/gui2/src/util/ast/match.ts Outdated Show resolved Hide resolved

kazcw added 2 commits December 13, 2023 06:45

Clean up tests

791c841

Rerun CI?

bf63ea8

kazcw added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Dec 13, 2023

kazcw and others added 2 commits December 13, 2023 07:38

Rerun CI?

371ad48

Merge branch 'develop' into wip/sb/set-eval-ctx

19906cb

somebody1234 added CI: Ready to merge This PR is eligible for automatic merge and removed CI: Clean build required CI runners will be cleaned before and after this PR is built. labels Dec 15, 2023

mergify bot merged commit 927df16 into develop Dec 15, 2023
33 of 35 checks passed

mergify bot deleted the wip/sb/set-eval-ctx branch December 15, 2023 10:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set output evaluation context for a single node #8440

Set output evaluation context for a single node #8440

somebody1234 commented Dec 1, 2023 •

edited

Loading

vitvakatu commented Dec 5, 2023

somebody1234 commented Dec 5, 2023

vitvakatu commented Dec 5, 2023

somebody1234 commented Dec 5, 2023

somebody1234 commented Dec 5, 2023

farmaazon left a comment

somebody1234 commented Dec 5, 2023

somebody1234 commented Dec 5, 2023

somebody1234 commented Dec 7, 2023

somebody1234 commented Dec 7, 2023

somebody1234 commented Dec 7, 2023

kazcw left a comment

Frizi Dec 8, 2023

somebody1234 Dec 8, 2023

Frizi Dec 8, 2023

somebody1234 Dec 8, 2023

Frizi Dec 8, 2023

somebody1234 Dec 8, 2023

Frizi Dec 8, 2023

somebody1234 Dec 8, 2023

somebody1234 commented Dec 8, 2023 •

edited

Loading

Set output evaluation context for a single node #8440

Set output evaluation context for a single node #8440

Conversation

somebody1234 commented Dec 1, 2023 • edited Loading

Pull Request Description

Important Notes

Screenshots

Checklist

vitvakatu commented Dec 5, 2023

somebody1234 commented Dec 5, 2023

vitvakatu commented Dec 5, 2023

somebody1234 commented Dec 5, 2023

somebody1234 commented Dec 5, 2023

farmaazon left a comment

Choose a reason for hiding this comment

somebody1234 commented Dec 5, 2023

somebody1234 commented Dec 5, 2023

somebody1234 commented Dec 7, 2023

somebody1234 commented Dec 7, 2023

somebody1234 commented Dec 7, 2023

kazcw left a 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

Choose a reason for hiding this comment

somebody1234 commented Dec 8, 2023 • edited Loading

somebody1234 commented Dec 1, 2023 •

edited

Loading

somebody1234 commented Dec 8, 2023 •

edited

Loading