From 5fe259f4225dcf012620b98630301991b3fae5db Mon Sep 17 00:00:00 2001 From: Umesh Timalsina Date: Tue, 5 May 2020 11:01:57 -0500 Subject: [PATCH 1/5] Allow type change in ArtifactIndex (Closes #1681) --- .../ArtifactIndex/ArtifactIndexControl.js | 9 +++--- .../ArtifactIndex/ArtifactIndexWidget.js | 32 +++++++++++-------- .../widgets/ArtifactIndex/ModelRow.html | 2 +- 3 files changed, 24 insertions(+), 19 deletions(-) diff --git a/src/visualizers/panels/ArtifactIndex/ArtifactIndexControl.js b/src/visualizers/panels/ArtifactIndex/ArtifactIndexControl.js index c7d666b0f..ece7c26c8 100644 --- a/src/visualizers/panels/ArtifactIndex/ArtifactIndexControl.js +++ b/src/visualizers/panels/ArtifactIndex/ArtifactIndexControl.js @@ -72,12 +72,11 @@ define([ }; this._widget.getConfigDialog = () => new ConfigDialog(this._client); - this._widget.onNameChange = (id, newName) => { - var name = this._client.getNode(id).getAttribute('name'), - msg = `Renamed "${name}" artifact to "${newName}"`; - + this._widget.onAttributeChange = (id, attr, newValue) => { + const attrValue = this._client.getNode(id).getAttribute(attr), + msg = `Changed "${attr}'s" value from ${attrValue}" to "${newValue}"`; this._client.startTransaction(msg); - this._client.setAttribute(id, 'name', newName); + this._client.setAttribute(id, attr, newValue); this._client.completeTransaction(); }; }; diff --git a/src/visualizers/widgets/ArtifactIndex/ArtifactIndexWidget.js b/src/visualizers/widgets/ArtifactIndex/ArtifactIndexWidget.js index 25c0f296c..887aabda4 100644 --- a/src/visualizers/widgets/ArtifactIndex/ArtifactIndexWidget.js +++ b/src/visualizers/widgets/ArtifactIndex/ArtifactIndexWidget.js @@ -79,19 +79,9 @@ define([ event.stopPropagation(); event.preventDefault(); }); - node.$name.on('dblclick', event => { - const name = $(event.target); - name.editInPlace({ - css: { - 'z-index': 1000 - }, - onChange: (oldVal, newVal) => { - if (newVal && newVal !== oldVal) { - this.onNameChange(desc.id, newVal); - } - } - }); - }); + node.$name.on('dblclick', {id : desc.id, attr: 'name'}, this.editInPlace.bind(this)); + + node.$type.on('dblclick', {id: desc.id, attr: 'type'}, this.editInPlace.bind(this)); node.$info.on('click', event => { event.stopPropagation(); @@ -139,6 +129,22 @@ define([ } }; + ArtifactIndexWidget.prototype.editInPlace = function(event) { + const el = $(event.target); + const id = event.data.id; + const attr = event.data.attr; + el.editInPlace({ + css: { + 'z-index' : 1000 + }, + onChange: (oldVal, newVal) => { + if (newVal && newVal !== oldVal) { + this.onAttributeChange(id, attr, newVal); + } + } + }); + }; + /* * * * * * * * Visualizer event handlers * * * * * * * */ diff --git a/src/visualizers/widgets/ArtifactIndex/ModelRow.html b/src/visualizers/widgets/ArtifactIndex/ModelRow.html index 17f7fe41b..85681784c 100644 --- a/src/visualizers/widgets/ArtifactIndex/ModelRow.html +++ b/src/visualizers/widgets/ArtifactIndex/ModelRow.html @@ -1,6 +1,6 @@ - + From 7a0ec688af6b64231b30a93daac7593bf76c0b60 Mon Sep 17 00:00:00 2001 From: Umesh Timalsina Date: Thu, 7 May 2020 16:25:53 -0500 Subject: [PATCH 2/5] Change attribute change commit message and ask confirmation for type change 1. Change meaninfule commit message when changing the attribute of an Artifact Node. 2. Add a ConfirmDialog with warnings for artifact data type Change and change it only if confirmed. --- .../ArtifactIndex/ArtifactIndexControl.js | 6 ++- .../ArtifactIndex/ArtifactIndexWidget.js | 45 +++++++++++++++---- 2 files changed, 40 insertions(+), 11 deletions(-) diff --git a/src/visualizers/panels/ArtifactIndex/ArtifactIndexControl.js b/src/visualizers/panels/ArtifactIndex/ArtifactIndexControl.js index ece7c26c8..ce215a0a5 100644 --- a/src/visualizers/panels/ArtifactIndex/ArtifactIndexControl.js +++ b/src/visualizers/panels/ArtifactIndex/ArtifactIndexControl.js @@ -73,8 +73,10 @@ define([ this._widget.getConfigDialog = () => new ConfigDialog(this._client); this._widget.onAttributeChange = (id, attr, newValue) => { - const attrValue = this._client.getNode(id).getAttribute(attr), - msg = `Changed "${attr}'s" value from ${attrValue}" to "${newValue}"`; + const node = this._client.getNode(id); + const name = node.getAttribute('name'); + const oldValue = node.getAttribute(attr), + msg = `Set ${name} artifact's ${attr} to ${newValue} (from ${oldValue})`; this._client.startTransaction(msg); this._client.setAttribute(id, attr, newValue); this._client.completeTransaction(); diff --git a/src/visualizers/widgets/ArtifactIndex/ArtifactIndexWidget.js b/src/visualizers/widgets/ArtifactIndex/ArtifactIndexWidget.js index 887aabda4..7bd112db7 100644 --- a/src/visualizers/widgets/ArtifactIndex/ArtifactIndexWidget.js +++ b/src/visualizers/widgets/ArtifactIndex/ArtifactIndexWidget.js @@ -79,9 +79,19 @@ define([ event.stopPropagation(); event.preventDefault(); }); - node.$name.on('dblclick', {id : desc.id, attr: 'name'}, this.editInPlace.bind(this)); - - node.$type.on('dblclick', {id: desc.id, attr: 'type'}, this.editInPlace.bind(this)); + node.$name.on('dblclick', event => this.editInPlace(event,{ + nodeId : desc.id, + targetAttribute : 'name', + nodeName : node.$name.text(), + confirmation : null + })); + + node.$type.on('dblclick', event => this.editInPlace(event, { + nodeId : desc.id, + targetAttribute : 'type', + confirmation : this.confirmArtifactTypeChange, + nodeName: node.$name.text() + })); node.$info.on('click', event => { event.stopPropagation(); @@ -99,12 +109,20 @@ define([ return await dialog.show(); }; + ArtifactIndexWidget.prototype.confirmArtifactTypeChange = async function(target, values) { + const { newValue, oldValue } = values; + const title = `Change data type for ${target}?`; + const body = `Changing the data type from ${oldValue} to ${newValue} + will not change the underlying data and can cause deserialization errors when used in a pipeline. Continue?`; + const dialog = new ConfirmDialog(title, body); + return await dialog.show(); + }; + ArtifactIndexWidget.prototype.getAuthenticationConfig = async function (dataInfo) { const {backend} = dataInfo; const metadata = Storage.getStorageMetadata(backend); metadata.configStructure = metadata.configStructure .filter(option => option.isAuth); - if (metadata.configStructure.length) { const configDialog = this.getConfigDialog(); const title = `Authenticate with ${metadata.name}`; @@ -129,17 +147,26 @@ define([ } }; - ArtifactIndexWidget.prototype.editInPlace = function(event) { + ArtifactIndexWidget.prototype.editInPlace = function(event, opts) { const el = $(event.target); - const id = event.data.id; - const attr = event.data.attr; + const id = opts.nodeId; + const attr = opts.targetAttribute; + el.editInPlace({ css: { 'z-index' : 1000 }, - onChange: (oldVal, newVal) => { + onChange: async (oldVal, newVal) => { if (newVal && newVal !== oldVal) { - this.onAttributeChange(id, attr, newVal); + const confirmed = opts.confirmation ? await opts.confirmation.call(this, + opts.nodeName, + {newValue: newVal, oldValue: oldVal} + ) : true; + if(confirmed) { + this.onAttributeChange(id, attr, newVal); + } else { + el.text(oldVal); + } } } }); From 751376348121be8f17bc85c4e26230f3adf374e9 Mon Sep 17 00:00:00 2001 From: Brian Broll Date: Fri, 8 May 2020 09:05:01 -0500 Subject: [PATCH 3/5] Only show "from __" msg if previously set --- src/visualizers/panels/ArtifactIndex/ArtifactIndexControl.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/visualizers/panels/ArtifactIndex/ArtifactIndexControl.js b/src/visualizers/panels/ArtifactIndex/ArtifactIndexControl.js index ce215a0a5..56c539be5 100644 --- a/src/visualizers/panels/ArtifactIndex/ArtifactIndexControl.js +++ b/src/visualizers/panels/ArtifactIndex/ArtifactIndexControl.js @@ -75,8 +75,9 @@ define([ this._widget.onAttributeChange = (id, attr, newValue) => { const node = this._client.getNode(id); const name = node.getAttribute('name'); - const oldValue = node.getAttribute(attr), - msg = `Set ${name} artifact's ${attr} to ${newValue} (from ${oldValue})`; + const oldValue = node.getAttribute(attr); + const fromMsg = oldValue ? ` (from ${oldValue})` : ''; + const msg = `Set ${name} artifact's ${attr} to ${newValue}${fromMsg}`; this._client.startTransaction(msg); this._client.setAttribute(id, attr, newValue); this._client.completeTransaction(); From 455614c49bebe28c79e8d9ed60762391c6cf88bf Mon Sep 17 00:00:00 2001 From: Umesh Timalsina Date: Fri, 8 May 2020 09:24:32 -0500 Subject: [PATCH 4/5] pass only changed values to the confirmation func in editInPlace --- src/visualizers/widgets/ArtifactIndex/ArtifactIndexWidget.js | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/visualizers/widgets/ArtifactIndex/ArtifactIndexWidget.js b/src/visualizers/widgets/ArtifactIndex/ArtifactIndexWidget.js index 7bd112db7..9494cd223 100644 --- a/src/visualizers/widgets/ArtifactIndex/ArtifactIndexWidget.js +++ b/src/visualizers/widgets/ArtifactIndex/ArtifactIndexWidget.js @@ -82,15 +82,13 @@ define([ node.$name.on('dblclick', event => this.editInPlace(event,{ nodeId : desc.id, targetAttribute : 'name', - nodeName : node.$name.text(), confirmation : null })); node.$type.on('dblclick', event => this.editInPlace(event, { nodeId : desc.id, targetAttribute : 'type', - confirmation : this.confirmArtifactTypeChange, - nodeName: node.$name.text() + confirmation : this.confirmArtifactTypeChange.bind(this, node.$name.text()), })); node.$info.on('click', event => { @@ -159,7 +157,6 @@ define([ onChange: async (oldVal, newVal) => { if (newVal && newVal !== oldVal) { const confirmed = opts.confirmation ? await opts.confirmation.call(this, - opts.nodeName, {newValue: newVal, oldValue: oldVal} ) : true; if(confirmed) { From f425b7d6eaf6bb5f3b1697ae68ad660f8f1da278 Mon Sep 17 00:00:00 2001 From: Umesh Timalsina Date: Fri, 8 May 2020 09:57:31 -0500 Subject: [PATCH 5/5] Use positional parameters for type change confirmation function --- .../widgets/ArtifactIndex/ArtifactIndexWidget.js | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/visualizers/widgets/ArtifactIndex/ArtifactIndexWidget.js b/src/visualizers/widgets/ArtifactIndex/ArtifactIndexWidget.js index 9494cd223..fb21f681c 100644 --- a/src/visualizers/widgets/ArtifactIndex/ArtifactIndexWidget.js +++ b/src/visualizers/widgets/ArtifactIndex/ArtifactIndexWidget.js @@ -107,8 +107,7 @@ define([ return await dialog.show(); }; - ArtifactIndexWidget.prototype.confirmArtifactTypeChange = async function(target, values) { - const { newValue, oldValue } = values; + ArtifactIndexWidget.prototype.confirmArtifactTypeChange = async function(target, newValue, oldValue) { const title = `Change data type for ${target}?`; const body = `Changing the data type from ${oldValue} to ${newValue} will not change the underlying data and can cause deserialization errors when used in a pipeline. Continue?`; @@ -156,9 +155,8 @@ define([ }, onChange: async (oldVal, newVal) => { if (newVal && newVal !== oldVal) { - const confirmed = opts.confirmation ? await opts.confirmation.call(this, - {newValue: newVal, oldValue: oldVal} - ) : true; + const confirmed = opts.confirmation ? await opts.confirmation.call( + this, newVal, oldVal) : true; if(confirmed) { this.onAttributeChange(id, attr, newVal); } else {