Refactor code in `EditorStore` to separate out logic for FormMode and TextMode #1898

gayathrir11 · 2023-02-10T14:32:53Z

Summary

How did you test this change?

Test(s) added
Manual testing (please provide screenshots/recordings)
No testing (please provide an explanation)

changeset-bot · 2023-02-10T14:32:57Z

🦋 Changeset detected

Latest commit: 002fa85

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 28 packages

Name	Type
@finos/legend-application-studio	Patch
@finos/legend-extension-dsl-diagram	Patch
@finos/legend-graph	Patch
@finos/legend-application-studio-bootstrap	Patch
@finos/legend-extension-dsl-data-space	Patch
@finos/legend-extension-dsl-mastery	Patch
@finos/legend-extension-dsl-persistence-cloud	Patch
@finos/legend-extension-dsl-persistence	Patch
@finos/legend-extension-dsl-service	Patch
@finos/legend-extension-dsl-text	Patch
@finos/legend-extension-format-morphir	Patch
@finos/legend-extension-store-flat-data	Patch
@finos/legend-extension-store-relational	Patch
@finos/legend-extension-store-service-store	Patch
@finos/legend-application-pure-ide	Patch
@finos/legend-application-query-bootstrap	Patch
@finos/legend-application-taxonomy-bootstrap	Patch
@finos/legend-manual-tests	Patch
@finos/legend-application-query	Patch
@finos/legend-application-taxonomy	Patch
@finos/legend-application	Patch
@finos/legend-extension-format-graphql	Patch
@finos/legend-extension-format-json-schema	Patch
@finos/legend-query-builder	Patch
@finos/legend-application-studio-deployment	Patch
@finos/legend-application-pure-ide-deployment	Patch
@finos/legend-application-query-deployment	Patch
@finos/legend-application-taxonomy-deployment	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

codecov · 2023-02-13T14:27:18Z

Codecov Report

Merging #1898 (002fa85) into master (645c331) will decrease coverage by 0.02%.
The diff coverage is 30.09%.

@@            Coverage Diff             @@
##           master    #1898      +/-   ##
==========================================
- Coverage   40.32%   40.30%   -0.02%     
==========================================
  Files        1444     1447       +3     
  Lines       65971    66016      +45     
  Branches    15492    15494       +2     
==========================================
+ Hits        26601    26607       +6     
- Misses      39268    39308      +40     
+ Partials      102      101       -1

Impacted Files	Coverage Δ
...tudio/src/components/editor/aux-panel/Problems.tsx	`7.40% <0.00%> (-3.31%)`	⬇️
...nts/editor/command-center/ProjectSearchCommand.tsx	`66.66% <0.00%> (+2.15%)`	⬆️
...ponents/editor/edit-panel/FileGenerationViewer.tsx	`7.69% <0.00%> (ø)`
...rc/components/editor/edit-panel/FunctionEditor.tsx	`14.34% <0.00%> (ø)`
...ditor/edit-panel/GenerationSpecificationEditor.tsx	`11.56% <0.00%> (ø)`
...components/editor/edit-panel/GrammarTextEditor.tsx	`17.02% <0.00%> (ø)`
...src/components/editor/edit-panel/RuntimeEditor.tsx	`18.08% <0.00%> (ø)`
...itor/edit-panel/data-editor/EmbeddedDataEditor.tsx	`17.24% <0.00%> (ø)`
...s/editor/edit-panel/diff-editor/EntityDiffView.tsx	`22.50% <0.00%> (ø)`
...itor/edit-panel/mapping-editor/MappingExplorer.tsx	`66.66% <0.00%> (ø)`
... and 36 more

akphi

The interface makes sense to me, just left some minor comments on further cleanups we can do.

I haven't reviewed in-depth movement of code logic in EditorGraphState.ts and GraphEditModeManagerState.ts - I assume you just moved over code logic, if that's the case, nothing much to check there I think.

My concern is with the switchMode we do in GraphEditorModeManagerState, to me that the fact we need a source mode and the target mode seems unnecessary and can be simplified

.changeset/poor-bobcats-beam.md

packages/legend-application-studio/src/components/editor/StatusBar.tsx

packages/legend-application-studio/src/components/editor/aux-panel/Problems.tsx

packages/legend-application-studio/src/components/editor/edit-panel/GrammarTextEditor.tsx

packages/legend-application-studio/src/stores/EmbeddedQueryBuilderState.ts

packages/legend-application-studio/src/stores/sidebar-state/LocalChangesState.ts

packages/legend-application-studio/src/stores/project-viewer/ProjectViewerStore.ts

packages/legend-application-studio/src/stores/EditorStore.ts

packages/legend-application-studio/src/stores/GraphEditModeManagerState.ts

gayathrir11 · 2023-02-21T11:02:50Z

I agree having source mode and target mode in switch mode currently doesn't make much sense as we know what to do when we are entering a garph edit mode. But when we have multi file support knowing source mode and target mode is useful as the set of operations we do is different.
Let us assume if we want to switch to form mode, the set of apis we call would be different when we are in single file text mode and multi file text mode. I don't want to do bunch of if-else here again to know what mode i am in currently. I hope this reason is not trivial.

packages/legend-extension-dsl-diagram/src/stores/studio/DiagramEditorState.ts

packages/legend-application-studio/src/stores/EditorTabManagerState.ts

packages/legend-application-studio/src/stores/EditorGraphState.ts

packages/legend-application-studio/src/stores/EditorStore.ts

packages/legend-application-studio/src/stores/EditorTabManagerState.ts

packages/legend-application-studio/src/components/__tests__/LegendStudioExplorerTree.test.tsx

packages/legend-application-studio/src/components/__tests__/EmbeddedQueryBuilder.test.tsx

packages/legend-application-studio/src/components/__tests__/ClassQueryBuilder.test.tsx

akphi · 2023-03-01T19:55:35Z

packages/legend-application-studio/src/stores/project-viewer/ProjectViewerStore.ts

+            isGraphBuildFailure: true,
+          }),
+        );
+        if (this.editorStore.isInFormMode) {


I forgot to mention, the biggest thing about this task is that we must remove isInFormMode and isInTextmode from EditorStore, those methods should not be needed, we should add methods to the interface everywhere we see differentiating behavior like this.

packages/legend-application-studio/src/stores/GraphEditModeManagerState.ts

packages/legend-application-studio/src/stores/EditorStore.ts

akphi

left some minor comments, like last time, the one thing that gates me from fully accepting this change is the complexity in switchMode(); like any function of this type, if we can break it into consistent, simple interface function call that would be great, from my perspective, changing mode is an operation that is used by higher-level code that has no knowledge of the mode implementation, what we're doing here is practically leaking of mode-specific logic.

Now, the only 3 places we should allow this sort of thing to me are:

When switching mode since we have to explicitly mention the mode to switch to
When displaying the mode switcher button (maybe, right now we have binary-state button, but if we have a dropdown or selector, this can even go away)
When we initialize the mode in EditorStore or so, we are biased since we have to pick a default

akphi · 2023-03-08T13:11:48Z

packages/legend-application-studio/src/components/editor/Editor.tsx

-                        {editable && editorStore.isInGrammarTextMode && (
-                          <GrammarTextEditor />
-                        )}
+                          editorStore.graphEditorMode.mode ===


not so satisfied with this, but we can deal with this later.

see my comment below for ProjectViewer

akphi · 2023-03-08T13:13:47Z

packages/legend-application-studio/src/components/editor/aux-panel/Problems.tsx

@@ -64,7 +58,8 @@ const ProblemItem = observer((props: { problem: Problem }) => {
        </div>
        {problem.sourceInformation && (
          <div className="auxiliary-panel__problem__source">
-            {editorStore.isInGrammarTextMode &&
+            {editorStore.graphEditorMode.mode ===


potentially if we have a place in rendering code layer to handle differentiating codepath, this is a good candidate to add there, together with all places we have to use editorStore.graphEditorMode.mode since although we decompose well at the state layer, our rendering is still messy

akphi · 2023-03-08T13:18:32Z

packages/legend-application-studio/src/stores/project-viewer/ProjectViewerStore.ts

-            ),
-          );
-        } catch {
+        yield flowResult(


note, here we're making assumption we're already in form mode, what if we're not?

this function gets called only when we initialize studio. So the mode we are in would be FORM

akphi · 2023-03-08T13:19:51Z

packages/legend-application-studio/src/stores/GraphEditFormModeState.ts

+import { LegendStudioTelemetry } from './LegendStudioTelemetry.js';
+import { GraphEditorMode } from './GraphEditorMode.js';
+
+export class GraphEditFormModeState extends GraphEditorMode {


--> FormGraphEditMode, also rename the file

akphi · 2023-03-08T13:20:15Z

packages/legend-application-studio/src/stores/GraphEditSingleFileGrammarModeState.ts

+import { GraphEditorMode } from './GraphEditorMode.js';
+import { ElementEditorState } from './editor-state/element-editor-state/ElementEditorState.js';
+
+export class GraphEditSingleFileGrammarModeState extends GraphEditorMode {


--> SingleFileGrammarGraphEditMode (also rename the file)

packages/legend-application-studio/src/stores/EditorStore.ts

akphi · 2023-03-08T13:33:36Z

packages/legend-application-studio/src/stores/EditorStore.ts

@@ -1187,11 +894,73 @@ export class EditorStore implements CommandRegistrar {
    );
  }



probably can move these 2 methods to inside of EditorGraphState now

akphi · 2023-03-08T13:33:51Z

packages/legend-application-studio/src/stores/EditorStore.ts

@@ -169,8 +134,8 @@ export class EditorStore implements CommandRegistrar {
  sdlcState: EditorSDLCState;
  graphState: EditorGraphState;
  graphManagerState: GraphManagerState;


Hm, in terms of organization at EditorStore level, I think we should move EditorStore.graphEditorMode to EditorStore.graphState.mode, I want us to start thinking about how we organize this. Maybe, what we should do is extending GraphManagerState and have LegendStudioGraphManagerState or something and put all graph handling logic of EditorGraphState in there, but colocating GraphEditorMode with EditorGraphState is sensible as it is what we had before

akphi · 2023-03-08T13:36:46Z

packages/legend-application-studio/src/stores/EditorStore.ts

+        const graphEditorMode = new GraphEditSingleFileGrammarModeState(this);
+        try {
+          yield flowResult(
+            graphEditorMode.cleanupBeforeEntering(fallbackOptions),


why are we still calling different set of methods when switching mode? I felt like we can also following a fix procedure, i.e. simplifying this down to bare-bone mode-independent logic, i.e.

attempt to leave and cleanup current mode check if we can enter target mode initialize target mode switch to new mode

akphi · 2023-03-08T13:39:32Z

packages/legend-application-studio/src/stores/EditorStore.ts

-      this.localChangesState = new TextLocalChangesState(this, this.sdlcState);
+  *switchModes(
+    to: GRAPH_EDITOR_MODE,
+    fallbackOptions?: {


I guess we can sleep on this for now, worst case, we have different set of methods: handleGraphBuilderFailure() and handleCompilationFailure()

akphi

Like said, let's address comments here in a later PR, the organization benefit we got out of this PR is already huge. Thanks @gayathrir11!!

gayathrir11 requested review from pierredebelen, akphi and MauricioUyaguari as code owners February 10, 2023 14:32

finos-cla-bot bot added the cla-present CLA Signed label Feb 10, 2023

gayathrir11 changed the title ~~Refactor code in to separate out logic for FormMode and TextMode~~ Refactor code in EditorStore to separate out logic for FormMode and TextMode Feb 10, 2023

gayathrir11 force-pushed the refactor branch from 0d3d6d7 to d711374 Compare February 13, 2023 14:15

akphi reviewed Feb 16, 2023

View reviewed changes

akphi added Application: Studio Issues related to Legend Studio application Type: Refactor labels Feb 16, 2023

gayathrir11 force-pushed the refactor branch 5 times, most recently from b655d41 to 88d411c Compare February 24, 2023 11:44

akphi reviewed Feb 27, 2023

View reviewed changes

packages/legend-extension-dsl-diagram/src/stores/studio/DiagramEditorState.ts Outdated Show resolved Hide resolved

akphi reviewed Feb 27, 2023

View reviewed changes

packages/legend-application-studio/src/stores/EditorTabManagerState.ts Show resolved Hide resolved

akphi requested changes Feb 27, 2023

View reviewed changes

packages/legend-application-studio/src/stores/EditorGraphState.ts Outdated Show resolved Hide resolved

packages/legend-application-studio/src/stores/EditorStore.ts Show resolved Hide resolved

packages/legend-application-studio/src/stores/EditorStore.ts Show resolved Hide resolved

gayathrir11 requested a review from a team as a code owner February 28, 2023 10:45

gayathrir11 force-pushed the refactor branch from bea20e8 to 52c338b Compare February 28, 2023 10:59

akphi requested changes Mar 5, 2023

View reviewed changes

gayathrir11 force-pushed the refactor branch from 52c338b to 410d206 Compare March 8, 2023 09:44

akphi requested changes Mar 8, 2023

View reviewed changes

Refactor code in to separate out logic for FormMode and TextMode

315e5cd

gayathrir11 force-pushed the refactor branch from 410d206 to 315e5cd Compare March 13, 2023 06:47

Merge branch 'master' into refactor

002fa85

akphi approved these changes Mar 16, 2023

View reviewed changes

akphi merged commit cc0030d into finos:master Mar 16, 2023

akphi mentioned this pull request Mar 16, 2023

Feature request: Change Query text mode to be inside query window rather than a new modal #2009

Open

2 tasks

gayathrir11 deleted the refactor branch May 19, 2023 07:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor code in `EditorStore` to separate out logic for FormMode and TextMode #1898

Refactor code in `EditorStore` to separate out logic for FormMode and TextMode #1898

gayathrir11 commented Feb 10, 2023

changeset-bot bot commented Feb 10, 2023 •

edited

Loading

codecov bot commented Feb 13, 2023 •

edited

Loading

akphi left a comment

gayathrir11 commented Feb 21, 2023

akphi Mar 1, 2023

akphi left a comment

akphi Mar 8, 2023

akphi Mar 8, 2023

akphi Mar 8, 2023

akphi Mar 8, 2023

gayathrir11 Mar 8, 2023

akphi Mar 8, 2023

akphi Mar 8, 2023

akphi Mar 8, 2023

akphi Mar 8, 2023

akphi Mar 8, 2023

akphi Mar 8, 2023

akphi left a comment

		@@ -1187,11 +894,73 @@ export class EditorStore implements CommandRegistrar {
		);
		}

Refactor code in EditorStore to separate out logic for FormMode and TextMode #1898

Refactor code in EditorStore to separate out logic for FormMode and TextMode #1898

Conversation

gayathrir11 commented Feb 10, 2023

Summary

How did you test this change?

changeset-bot bot commented Feb 10, 2023 • edited Loading

🦋 Changeset detected

codecov bot commented Feb 13, 2023 • edited Loading

Codecov Report

akphi left a comment

Choose a reason for hiding this comment

gayathrir11 commented Feb 21, 2023

Choose a reason for hiding this comment

akphi 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akphi left a comment

Choose a reason for hiding this comment

Refactor code in `EditorStore` to separate out logic for FormMode and TextMode #1898

Refactor code in `EditorStore` to separate out logic for FormMode and TextMode #1898

changeset-bot bot commented Feb 10, 2023 •

edited

Loading

codecov bot commented Feb 13, 2023 •

edited

Loading