Split editor state from saved state #20323

ppisljar · 2018-06-28T19:54:08Z

visualize editor should not share state with vis, this pr separates the two

elasticmachine · 2018-06-29T11:18:42Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-07-02T08:03:14Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-07-02T10:36:35Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-07-02T13:09:58Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

cjcenizal · 2018-07-02T18:22:28Z

src/ui/public/vis/editors/default/default.js

+            if (lockDirty) {
+              lockDirty = false;
+            } else {
+              $scope.vis.dirty = !angular.equals(newState, $scope.oldState);


It looks like we're using lockDirty to prevent scope.vis.dirty from being updated if there have been any changes to the vis.state. It's not clear to me why we're doing this though... could you add a comment to explain?

when visualization changes its parameter (on vis.params) editor needs to be notified about the change and update the appropriate option, but its dirty state should not change as the setting was already applied on the vis.params

i will add a comment

cjcenizal

This looks great! I tested it locally and things still seem to work fine. The code looks good. I just had some questions about a part of the code I was having a hard time understanding.

cjcenizal · 2018-07-02T18:55:05Z

src/ui/public/vis/editors/default/default.js

@@ -127,6 +137,32 @@ const defaultEditor = function ($rootScope, $compile) {
            catch (e) {} // eslint-disable-line no-empty
          }, true);

+          $scope.$watch(() => {


I think we can make this a little easier to understand with unabbreviated variable names, variables to reference the object properties, and early exits:

$scope.$watch(() => { return $scope.vis.getCurrentState(false); }, (newState) => { // figure out what changed and copy into editor state const updateEditorStateWithChanges = (newState, oldState, editorState) => { for (const prop in newState) { const newStateValue = newState[prop]; const oldStateValue = oldState[prop]; const editorStateValue = editorState[prop]; if (typeof newStateValue === 'object') { if (editorStateValue) { // Keep traversing. return updateEditorStateWithChanges(newStateValue, oldStateValue, editorStateValue); } const newStateValueCopy = _.cloneDeep(newStateValue); editorState[prop] = newStateValueCopy; oldState[prop] = newStateValueCopy; return; } if (newStateValue !== oldStateValue) { oldState[prop] = newStateValue; editorState[prop] = newStateValue; lockDirty = true; } } }; updateEditorStateWithChanges(newState, $scope.oldState, $scope.state); }, true);

cjcenizal · 2018-07-02T18:55:37Z

src/ui/public/vis/editors/default/default.js

+                  if (src[prop] !== org[prop]) {
+                    org[prop] = src[prop];
+                    dst[prop] = src[prop];
+                    lockDirty = true;


Why do we only set lockDirty here but not on line 150?

bug, thanks for finding this

cjcenizal · 2018-07-02T18:57:37Z

src/ui/public/vis/editors/default/default.js

+                    dst[prop] = _.cloneDeep(src[prop]);
+                    org[prop] = _.cloneDeep(src[prop]);
+                  } else {
+                    recursiveCopyChanged(src[prop], org[prop], dst[prop]);


Why do we continue recursing under this condition (when this node is an object, and the node already exists in the editor state)? Why not update it?

when a node is an object we need to recurse, as we want to only update specific keys.
if the object is not find in the editorState, then the whole object is copied over.

elasticmachine · 2018-07-03T07:42:53Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

src/ui/public/vis/editors/default/default.js

            $scope.vis.updateState();
            $scope.vis.dirty = false;
          };
          $scope.resetEditableVis = () => {
-            $scope.vis.resetState();


timroes · 2018-07-03T09:10:54Z

src/ui/public/vis/vis.js

          .map(agg => agg.toJSON())
          .filter(agg => includeDisabled || agg.enabled)
          .filter(Boolean)
      };
    }

+    copyCurrentState(includeDisabled) {


I would add includeDisabled = false just to make it visible directly when seeing the method what's the default value of this, instead of needing to search which method is actually called by this one, and what that method does with the parameter. (Maybe also give it a default value in getSerializableState.)

markov00

LGTM. Tested with demo flight visualizations.
Can be merged after @timroes comments are resolved.

timroes · 2018-07-03T11:19:47Z

src/ui/public/vis/vis.js

-        params: this.params,
-        aggs: this.aggs
+        title: state.title,
+        type: state.type.name || state.type,


In which case do we need that state.type?

timroes · 2018-07-03T12:18:10Z

src/ui/public/vis/editors/default/default.js

          // track state of editable vis vs. "actual" vis
          $scope.stageEditableVis = () => {
+            $scope.vis.setCurrentState($scope.state);


If there is no specific reason for the order of these two lines, I would rather first get the oldStat and then start setting the new one (and updating it), since it feels a bit more natural to read imho. I looked at this and wondered at first why we set the current state before actually getting the old state.

timroes · 2018-07-03T12:20:09Z

src/ui/public/vis/editors/default/default.js

            $scope.vis.updateState();
            $scope.vis.dirty = false;
          };
          $scope.resetEditableVis = () => {
-            $scope.vis.resetState();
+            $scope.state = $scope.vis.copyCurrentState();
+            $scope.oldState = $scope.vis.getSerializableState($scope.state);


Do we need to set the oldState here? Shouldn't it be the state we are resetting to anyway?

timroes · 2018-07-03T12:27:24Z

src/ui/public/vis/editors/default/default.js

            $scope.vis.updateState();
            $scope.vis.dirty = false;
          };
          $scope.resetEditableVis = () => {
-            $scope.vis.resetState();
+            $scope.state = $scope.vis.copyCurrentState();


I think copyCurrentState should include the enabled aggs here. At least we're having a bug with that. If you disable an aggregation and apply that setting, and then change anything (no matter where) and hit reset, that disabled aggregation will vanish from the editor.

timroes

Outstanding issues with disabled aggregations. Besides that code already looks good. Will do a final review once fixed.

elasticmachine · 2018-07-03T14:13:16Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

cjcenizal · 2018-07-03T15:15:08Z

src/ui/public/vis/editors/default/default.js

+                  const editorStateValue = editorState[prop];
+
+                  if (typeof newStateValue === 'object') {
+                    if (editorStateValue) {


Your explanation about this sounds great. Makes sense to me. Can we add a comment?

// When a node is an object we need to recurse, as we want to only update specific keys. // But if the object is not found in the editorState, then it's safe to update the node with // the entire object.

cjcenizal

🍄 Code LGTM!

Issues have been adressed

ppisljar added WIP Work in progress Feature:Visualizations Generic visualization features (in case no more specific feature label is available) labels Jun 28, 2018

ppisljar force-pushed the fix/editorState branch 4 times, most recently from cacfb5c to a8c600e Compare June 29, 2018 07:16

elastic deleted a comment from elasticmachine Jun 29, 2018

ppisljar added review v6.4.0 and removed WIP Work in progress labels Jun 29, 2018

ppisljar requested review from timroes, nreese and markov00 June 29, 2018 09:15

cjcenizal self-requested a review June 29, 2018 14:11

giving editor its own state

ca1119b

ppisljar force-pushed the fix/editorState branch 2 times, most recently from 46763a9 to c3a3490 Compare July 2, 2018 06:54

ppisljar force-pushed the fix/editorState branch from c3a3490 to 36749ed Compare July 2, 2018 09:24

cleanup

11b7bc5

ppisljar force-pushed the fix/editorState branch from 36749ed to 11b7bc5 Compare July 2, 2018 11:57

cjcenizal reviewed Jul 2, 2018

View reviewed changes

updated based on review from CJ

99851ec

timroes changed the title ~~giving visualize editor its own state~~ Split editor state from saved state Jul 3, 2018

timroes added the v7.0.0 label Jul 3, 2018

timroes reviewed Jul 3, 2018

View reviewed changes

src/ui/public/vis/editors/default/default.js

$scope.vis.updateState();

$scope.vis.dirty = false;

};

$scope.resetEditableVis = () => {

$scope.vis.resetState();

This comment was marked as resolved.

Sign in to view

timroes reviewed Jul 3, 2018

View reviewed changes

markov00 approved these changes Jul 3, 2018

View reviewed changes

timroes reviewed Jul 3, 2018

View reviewed changes

timroes previously requested changes Jul 3, 2018

View reviewed changes

updating based on tims review

4d5be42

ppisljar removed the request for review from nreese July 3, 2018 13:03

cjcenizal reviewed Jul 3, 2018

View reviewed changes

cjcenizal approved these changes Jul 3, 2018

View reviewed changes

ppisljar merged commit a9c8bfe into elastic:master Jul 4, 2018

ppisljar mentioned this pull request Jul 4, 2018

[6.x] Split editor state from saved state (#20323) #20451

Merged

ppisljar added a commit to ppisljar/kibana that referenced this pull request Jul 4, 2018

Split editor state from saved state (elastic#20323)

aeff2dd

ppisljar deleted the fix/editorState branch July 4, 2018 07:12

ppisljar added a commit to ppisljar/kibana that referenced this pull request Jul 4, 2018

Split editor state from saved state (elastic#20323)

d245baf

ppisljar added a commit that referenced this pull request Jul 4, 2018

Split editor state from saved state (#20323) (#20451)

53d3879

ppisljar restored the fix/editorState branch September 26, 2018 10:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split editor state from saved state #20323

Split editor state from saved state #20323

ppisljar commented Jun 28, 2018

elasticmachine commented Jun 29, 2018

elasticmachine commented Jul 2, 2018

elasticmachine commented Jul 2, 2018

elasticmachine commented Jul 2, 2018

cjcenizal Jul 2, 2018

ppisljar Jul 2, 2018

cjcenizal left a comment

cjcenizal Jul 2, 2018

cjcenizal Jul 2, 2018

ppisljar Jul 3, 2018

cjcenizal Jul 2, 2018

ppisljar Jul 3, 2018

elasticmachine commented Jul 3, 2018

This comment was marked as resolved.

timroes Jul 3, 2018

markov00 left a comment

timroes Jul 3, 2018

timroes Jul 3, 2018

timroes Jul 3, 2018

timroes Jul 3, 2018

timroes left a comment

elasticmachine commented Jul 3, 2018

cjcenizal Jul 3, 2018

cjcenizal left a comment

Split editor state from saved state #20323

Split editor state from saved state #20323

Conversation

ppisljar commented Jun 28, 2018

elasticmachine commented Jun 29, 2018

💔 Build Failed

elasticmachine commented Jul 2, 2018

💔 Build Failed

elasticmachine commented Jul 2, 2018

💔 Build Failed

elasticmachine commented Jul 2, 2018

💚 Build Succeeded

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal 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

elasticmachine commented Jul 3, 2018

💚 Build Succeeded

This comment was marked as resolved.

Choose a reason for hiding this comment

markov00 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

timroes left a comment

Choose a reason for hiding this comment

elasticmachine commented Jul 3, 2018

💚 Build Succeeded

Choose a reason for hiding this comment

cjcenizal left a comment

Choose a reason for hiding this comment