chore(client): WIP refactor tab content updating workarounds

- App.js#openEmptyFile - BpmnEditor#handleNamespace
camunda · Nov 29, 2018 · f74e679 · f74e679
1 parent 79cd80f
commit f74e679
Show file tree

Hide file tree

Showing 7 changed files with 106 additions and 67 deletions.
diff --git a/client/src/app/App.js b/client/src/app/App.js
@@ -10,7 +10,8 @@ import {
 import {
   assign,
   debounce,
-  forEach
+  forEach,
+  merge
 } from 'min-dash';
 
 import Toolbar from './Toolbar';
@@ -230,11 +231,13 @@ export class App extends Component {
       throw new Error('must not change tab.id');
     }
 
-    const updatedTab = {
-      ...tab,
-      ...newAttrs
+    let updatedTab = {
+      ...tab
     };
 
+    // allow updating nested attributes
+    updatedTab = merge(updatedTab, newAttrs);
+
     this.setState((state) => {
 
       const {
@@ -509,21 +512,20 @@ export class App extends Component {
 
     const response = await dialog.showEmptyFileDialog({
       file,
-      fileType
+      type: fileType
     });
 
     if (response == 'create') {
-      assign(file, {
-        contents: tabsProvider.getInitialFileContents(fileType)
-      });
 
       // TODO(philippfromme): fix dirty state
       let tab = this.addTab(
         tabsProvider.createTabForFile(file)
       );
 
-      this.handleTabChanged(tab)({
-        dirty: true
+      tab = await this.updateTab(tab, {
+        file: {
+          contents: tabsProvider.getInitialFileContents(fileType)
+        }
       });
 
       await this.selectTab(tab);
@@ -697,10 +699,6 @@ export class App extends Component {
       });
     }
 
-    if ('contents' in properties) {
-      tab.file.contents = properties.contents;
-    }
-
     tabState = {
       ...tabState,
       ...properties
@@ -713,6 +711,24 @@ export class App extends Component {
     this.updateMenu(tabState);
   }
 
+  /**
+   * Handle if tab content is updated.
+   *
+   * @param {String} new tab content
+   */
+  handleTabContentUpdated = (tab) => (newContent) => {
+
+    if (!newContent) {
+      return;
+    }
+
+    this.updateTab(tab, {
+      file: {
+        contents: newContent
+      }
+    });
+  }
+
   tabSaved(tab, newFile) {
 
     const {
@@ -1342,6 +1358,7 @@ export class App extends Component {
                   tab={ activeTab }
                   layout={ layout }
                   onChanged={ this.handleTabChanged(activeTab) }
+                  onContentUpdated={ this.handleTabContentUpdated(activeTab) }
                   onError={ this.handleTabError(activeTab) }
                   onWarning={ this.handleTabWarning(activeTab) }
                   onShown={ this.handleTabShown(activeTab) }

diff --git a/client/src/app/TabsProvider.js b/client/src/app/TabsProvider.js
@@ -229,6 +229,9 @@ export default class TabsProvider {
       get name() {
         return this.file.name;
       },
+      set name(newName) {
+        this.file.name = newName;
+      },
       get title() {
         return this.file.path || 'unsaved';
       },

diff --git a/client/src/app/__tests__/AppSpec.js b/client/src/app/__tests__/AppSpec.js
@@ -731,30 +731,45 @@ describe('<App>', function() {
   });
 
 
-  describe('tab changing', function() {
+  describe('#handleTabContentUpdated', function() {
 
-    it('should update contents on tab changed', async function() {
+    let app, tab;
 
-      // given
-      const {
-        app
-      } = createApp();
+    beforeEach(async function() {
+
+      app = createApp().app;
 
       await app.createDiagram('bpmn');
 
-      const {
-        activeTab
-      } = app.state;
+      tab = app.state.activeTab;
 
-      const newContents = 'foo';
+    });
+
+    it('should call #updateTab when new content is given', async function() {
+
+      // given
+      const updateSpy = spy(app, 'updateTab');
 
       // when
-      app.handleTabChanged(activeTab)({
-        contents: newContents
+      app.handleTabContentUpdated(tab)({
+        newContent: '< bar/>'
       });
 
       // then
-      expect(activeTab.file.contents).to.equal(newContents);
+      expect(updateSpy).to.have.been.called;
+    });
+
+
+    it('should NOT call #updateTab when NO new content given', async function() {
+
+      // given
+      const updateSpy = spy(app, 'updateTab');
+
+      // when
+      app.handleTabContentUpdated(tab)();
+
+      // then
+      expect(updateSpy).to.not.have.been.called;
     });
 
   });
@@ -1570,6 +1585,26 @@ describe('<App>', function() {
     });
 
 
+    it('should update tab with nested attributes', async function() {
+
+      // given
+      const newAttrs = {
+        file: {
+          name: 'foo.bpmn'
+        }
+      };
+
+      // when
+      const updatedTab = await app.updateTab(tab, newAttrs);
+
+      const file = updatedTab.file;
+
+      // then
+      expect(file.name).to.eql('foo.bpmn');
+      expect(file.contents).to.exist;
+    });
+
+
     it('should update navigation history', async function() {
       // given
       const newAttrs = {

diff --git a/client/src/app/__tests__/mocks/index.js b/client/src/app/__tests__/mocks/index.js
@@ -152,6 +152,9 @@ export class TabsProvider {
       get name() {
         return this.file.name;
       },
+      set name(newName) {
+        this.file.name = newName;
+      },
       file,
       type
     };

diff --git a/client/src/app/tabs/MultiSheetTab.js b/client/src/app/tabs/MultiSheetTab.js
@@ -289,7 +289,8 @@ export class MultiSheetTab extends CachedComponent {
       id,
       xml,
       layout,
-      onAction
+      onAction,
+      onContentUpdated
     } = this.props;
 
     if (!sheets) {
@@ -315,6 +316,7 @@ export class MultiSheetTab extends CachedComponent {
             onContextMenu={ this.handleContextMenu }
             onAction={ onAction }
             onChanged={ this.handleChanged }
+            onContentUpdated={ onContentUpdated }
             onError={ this.handleError }
             onImport={ this.handleImport }
             onLayoutChanged={ this.handleLayoutChanged } />

diff --git a/client/src/app/tabs/bpmn/BpmnEditor.js b/client/src/app/tabs/bpmn/BpmnEditor.js
@@ -36,10 +36,6 @@ import {
   replaceNamespace
 } from './util/namespace';
 
-import {
-  assign
-} from 'min-dash';
-
 const NAMESPACE_URL_ACTIVITI = 'http://activiti.org/bpmn',
       NAMESPACE_URL_CAMUNDA = 'http://camunda.org/schema/1.0/bpmn',
       NAMESPACE_PREFIX_ACTIVITI = 'activiti',
@@ -223,7 +219,8 @@ export class BpmnEditor extends CachedComponent {
 
   handleNamespace = async (xml) => {
     const {
-      onAction
+      onAction,
+      onContentUpdated
     } = this.props;
 
     const answer = await onAction('show-dialog', getNamespaceDialog());
@@ -236,9 +233,10 @@ export class BpmnEditor extends CachedComponent {
         oldNamespaceUrl: NAMESPACE_URL_ACTIVITI
       });
 
-      this.handleChanged({
-        xml
-      });
+      if (typeof onContentUpdated === 'function') {
+        onContentUpdated(xml);
+      }
+
     }
 
     return xml;
@@ -267,10 +265,6 @@ export class BpmnEditor extends CachedComponent {
 
 
   handleChanged = (event = {}) => {
-    const {
-      xml
-    } = event;
-
     const {
       modeler
     } = this.getCached();
@@ -310,10 +304,6 @@ export class BpmnEditor extends CachedComponent {
       zoom: true
     };
 
-    if (xml) {
-      assign(newState, { contents : xml });
-    }
-
     const contextMenu = getBpmnContextMenu(newState);
 
     const editMenu = getBpmnEditMenu(newState);

diff --git a/client/src/app/tabs/bpmn/__tests__/BpmnEditorSpec.js b/client/src/app/tabs/bpmn/__tests__/BpmnEditorSpec.js
@@ -212,7 +212,7 @@ describe('<BpmnEditor>', function() {
 
   describe('#handleNamespace', function() {
 
-    it('should replace namespace', async function() {
+    it('should replace namespace', function(done) {
 
       // given
       const onAction = action => {
@@ -222,11 +222,9 @@ describe('<BpmnEditor>', function() {
         }
       };
 
-      const changedSpy = newState => {
+      const updatedSpy = contents => {
 
         // then
-        const { contents } = newState;
-
         expect(contents).to.exist;
 
         expect(contents.includes('xmlns:activiti="http://activiti.org/bpmn"')).to.be.false;
@@ -237,17 +235,19 @@ describe('<BpmnEditor>', function() {
 
         expect(contents.includes('activiti:assignee')).to.be.false;
         expect(contents.includes('camunda:assignee')).to.be.true;
+
+        done();
       };
 
       // when
       renderEditor(activitiXML, {
         onAction,
-        onChanged: changedSpy
+        onContentUpdated: updatedSpy
       });
     });
 
 
-    it('should NOT replace namespace', async function() {
+    it('should NOT replace namespace', function() {
 
       // given
       const onAction = action => {
@@ -257,28 +257,15 @@ describe('<BpmnEditor>', function() {
         }
       };
 
-      const changedSpy = newState => {
-
-        // then
-        const { contents } = newState;
-
-        expect(contents).to.exist;
-
-        expect(contents.includes('xmlns:activiti="http://activiti.org/bpmn"')).to.be.true;
-        expect(contents.includes('xmlns:camunda="http://camunda.org/schema/1.0/bpmn"')).to.be.false;
-
-        expect(contents.includes('targetNamespace="http://activiti.org/bpmn"')).to.be.true;
-        expect(contents.includes('targetNamespace="http://camunda.org/schema/1.0/bpmn"')).to.be.false;
-
-        expect(contents.includes('activiti:assignee')).to.be.true;
-        expect(contents.includes('camunda:assignee')).to.be.false;
-      };
+      const updatedSpy = spy();
 
       // when
       renderEditor(activitiXML, {
         onAction,
-        onChanged: changedSpy
+        onContentUpdated: updatedSpy
       });
+
+      expect(updatedSpy).to.not.have.been.called;
     });
 
   });
@@ -527,6 +514,7 @@ function renderEditor(xml, options = {}) {
     layout,
     onAction,
     onChanged,
+    onContentUpdated,
     onError,
     onImport,
     onLayoutChanged
@@ -542,6 +530,7 @@ function renderEditor(xml, options = {}) {
         onError={ onError || noop }
         onImport={ onImport || noop }
         onLayoutChanged={ onLayoutChanged || noop }
+        onContentUpdated={ onContentUpdated || noop }
         cache={ options.cache || new Cache() }
         layout={ layout || {
           minimap: {