Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: support lg templates cross-file copy during Visual Editor copy / paste #2236

Merged
merged 11 commits into from
Mar 12, 2020

Conversation

yeze322
Copy link
Contributor

@yeze322 yeze322 commented Mar 11, 2020

Description

The old copy logic relied on a copyLgTemplate api provided by @zhixzhan , however, as reported in a recent issue #2207 , after lg templates were saved in separated files, it's not capable for cross-file copy behavior anymore.

In this PR, I made a refactoring on the copy-paste flow to make the copy of actions independent from lg file structure and lg operation api.

Comparasion

Compared to the old solution that having a heavy 'paste' logic and external api dependency (copyLgTemplate):

// 1 - copy
const copy = actions => _.deepClone(actions);

// 2 - paste
const paste = actions => {
  const clonedActions = copyConstructor(actions);
  walkAdaptiveActions(
    clonedActions,
    action => {
      action.$designer = getDesigner(action.$designer.name, action.$designer.description);
      if (action.$type === 'SendActivity') {
        action.activity = copyLgTemplate(from=action.activity, to=`bfdactivity-${action.$designer.id}`);
      }
    }
  insert(dialog, actions);
}

The new solution is more similar to a real clipboard in which copied/cut actions keep real lg text:

// 1 - copy
const copy = actions => {
  const copiedActions = copyConstructor(actions);
  const dereferenceLgTemplate = lgStr => getLgTemplateBody(lgStr);
  walkAdaptiveActions(
    copiedActions,
    action => {
      if (action.$type === 'SendActivity') action.activity = dereferenceLgTemplate(action.activity);
    }
  )
}

// 2 - paste
const paste = actions => {
  const copiedActions = copyConstructor(actions);
  const buildLgReference= lgStr => updateLgTemplate(id, lgStr);
  walkAdaptiveActions(
    copiedActions,
    action => {
      if (action.$type === 'SendActivity') action.activity = buildLgReference(action.activity);
    }
  )
  insert(copiedActions);
}

The most significant difference here is when does it handle lg template references.

  • in the old solution, it handles lg references during paste; it's buggy if the source actions were deleted before they were pasted.
  • in the new solution
    • lg references will be dereferenced during 'copy' which means that a '${bfdactivity-1234}' string will be copied as its real value 'hello' (lgTemplate.body)
    • (Not included in this PR) Correspondingly, during 'paste', lg body string 'hello' will be transformed to a reference again by the api createLgTemplate

Benefits

  1. It supports cross-file lg copy since lg body string is kept in clipboard
  2. It fixes a rare user behavior: copy nodes -> delete old nodes -> paste nodes
  3. It will potentially support dumping copied composer actions to plain text files & copying json in plain text file into composer

Todos in the future:

  • create necessary lg templates during paste as described above (need a new createLgTemplate api @zhixzhan )
  • ObiEditor.tsx is exploding, consider to extract copy/paste logic out of it.

Task Item

closes #2207
closes #2210

Screenshots

@yeze322 yeze322 changed the base branch from stable to master March 11, 2020 14:23
@yeze322 yeze322 changed the title refactor: support lg templates cross-file copy during Visual Editor copy / paste [WIP] refactor: support lg templates cross-file copy during Visual Editor copy / paste Mar 11, 2020
@yeze322 yeze322 changed the title [WIP] refactor: support lg templates cross-file copy during Visual Editor copy / paste refactor: support lg templates cross-file copy during Visual Editor copy / paste Mar 11, 2020
@yeze322 yeze322 marked this pull request as ready for review March 11, 2020 15:42
@yeze322 yeze322 changed the title refactor: support lg templates cross-file copy during Visual Editor copy / paste [WIP] refactor: support lg templates cross-file copy during Visual Editor copy / paste Mar 11, 2020
@yeze322
Copy link
Contributor Author

yeze322 commented Mar 11, 2020

@zhixzhan I need a new LG api whose interface is

const createLgTemplate = (templateName: string, templateBody: string, templateParams?: any[]) => Promise<string>; // returns either the new template name or the whole template

Do you have any idea on that?

@yeze322
Copy link
Contributor Author

yeze322 commented Mar 11, 2020

@cwhitten this PR will fix the problem in your 'Move' PR that moving SendActivity to new dialog will lose lg string. Current version works but the value of 'prompt' field in SendActivity will be the lg body string.
For example:

// Action to be moved
{
  $type: 'SendActivity',
  activity: '${bfdactivity-1234()}' // the real content is '- hello'
}
// Action will appear in new dialog 
{
  $type: 'SendActivity',
  activity: '- hello'
}

I will solve that problem when lg api is ready.

@yeze322 yeze322 mentioned this pull request Mar 11, 2020
@github-actions
Copy link

Coverage Status

Coverage decreased (-0.003%) to 40.709% when pulling 06be7c9 on visual/shallow-copy into ffc4d80 on master.

@zhixzhan
Copy link
Contributor

zhixzhan commented Mar 12, 2020

@zhixzhan I need a new LG api whose interface is

const createLgTemplate = (templateName: string, templateBody: string, templateParams?: any[]) => Promise<string>; // returns either the new template name or the whole template

Do you have any idea on that?

I see, this new api would return plain string template content without any reference. I'm not sure is it possible to convert deep nested lg template to plain string.

I think about another solution is restrict composer created bot's lg reference structure:

  1. assume all template in [dialog].lg is auto-generated by composer should not be referenced by other.
  2. all [dialog].lg must import common.lg
  3. [dialog].lg should not import other [dialog].lg

if we apply this rule, we could maintain resource as usual without worry about buggy deep copy.
Not sure, does it make sense?

@yeze322
Copy link
Contributor Author

yeze322 commented Mar 12, 2020

@zhixzhan I used the updateLgTemplate you told me, it could solve my problem!

@yeze322 yeze322 changed the title [WIP] refactor: support lg templates cross-file copy during Visual Editor copy / paste [WIP] g templates cross-file copy during Visual Editor copy / paste Mar 12, 2020
@yeze322 yeze322 changed the title [WIP] g templates cross-file copy during Visual Editor copy / paste refactor: support lg templates cross-file copy during Visual Editor copy / paste Mar 12, 2020
@cwhitten cwhitten merged commit d91eaa4 into master Mar 12, 2020
@cwhitten cwhitten deleted the visual/shallow-copy branch March 12, 2020 15:41
@a-b-r-o-w-n a-b-r-o-w-n mentioned this pull request Mar 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cross-file copy of LU / LG resources
3 participants