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

DYN-6186: Clear any element binding data from DYN upon SaveAs #14394

Merged
merged 16 commits into from
Sep 13, 2023

Conversation

aparajit-pratap
Copy link
Contributor

@aparajit-pratap aparajit-pratap commented Sep 8, 2023

Purpose

Clear any element binding data from DYN upon SaveAs. Treat newly saved DYN file as completely new file.

Save As prompts the following dialog:
image

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated
  • This PR contains no files larger than 50 MB

Release Notes

Clear any element binding data from DYN upon SaveAs.

Reviewers

(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)

(FILL ME IN, optional) Any additional notes to reviewers or testers.

FYIs

(FILL ME IN, Optional) Names of anyone else you wish to be notified of

@QilongTang
Copy link
Contributor

QilongTang commented Sep 8, 2023

Thank you @aparajit-pratap This is a good first stab while I do feel like informing every user about element binding at SaveAs with a dialog is a bit overkill. Given we are no longer implementing this for 2.19 release, could we implement something similar to the open dialog to give user an option to dump element binding data as alternative? So that user have an option to fallback to legacy SaveAS

image

image

@aparajit-pratap
Copy link
Contributor Author

aparajit-pratap commented Sep 11, 2023

Thank you @aparajit-pratap This is a good first stab while I do feel like informing every user about element binding at SaveAs with a dialog is a bit overkill. Given we are no longer implementing this for 2.19 release, could we implement something similar to the open dialog to give user an option to dump element binding data as alternative? So that user have an option to fallback to legacy SaveAS

@QilongTang what I understood by having a second SaveAs option, was in addition to the Save As we have today, we restore the legacy SaveAs as the second option, which does not modify node GUIDs, correct? If we are on the same page about that, are you suggesting I add that as an option to the File -> Save As dialog - something like a check box with a description like Choose Save As option to preserve element binding or similar? I wasn't thinking of adding it to this PR but I can very well expand its scope and continue working to add it here.

@Amoursol
Copy link
Contributor

@aparajit-pratap I too would err on the side of information here, and given the "Save As" button isn't as common as "Save" I'm OK with creating an additional dialog, but do agree that a more holistic approach would be ideal pending size of scope. @Jingyi-Wen thoughts?

@QilongTang
Copy link
Contributor

QilongTang commented Sep 11, 2023

Thank you @aparajit-pratap This is a good first stab while I do feel like informing every user about element binding at SaveAs with a dialog is a bit overkill. Given we are no longer implementing this for 2.19 release, could we implement something similar to the open dialog to give user an option to dump element binding data as alternative? So that user have an option to fallback to legacy SaveAS

@QilongTang what I understood by having a second SaveAs option, was in addition to the Save As we have today, we restore the legacy SaveAs as the second option, which does not modify node GUIDs, correct? If we are on the same page about that, are you suggesting I add that as an option to the File -> Save As dialog - something like a check box with a description like Choose Save As option to preserve element binding or similar? I wasn't thinking of adding it to this PR but I can very well expand its scope and continue working to add it here.

Yes, we are on the same page and that was my suggestion, with that user is still one dialog away from the SaveAs destination instead of two.

@aparajit-pratap
Copy link
Contributor Author

Thank you @aparajit-pratap This is a good first stab while I do feel like informing every user about element binding at SaveAs with a dialog is a bit overkill. Given we are no longer implementing this for 2.19 release, could we implement something similar to the open dialog to give user an option to dump element binding data as alternative? So that user have an option to fallback to legacy SaveAS

@QilongTang what I understood by having a second SaveAs option, was in addition to the Save As we have today, we restore the legacy SaveAs as the second option, which does not modify node GUIDs, correct? If we are on the same page about that, are you suggesting I add that as an option to the File -> Save As dialog - something like a check box with a description like Choose Save As option to preserve element binding or similar? I wasn't thinking of adding it to this PR but I can very well expand its scope and continue working to add it here.

Yes, we are on the same page and that was my suggestion, with that user is still one dialog away from the SaveAs destination instead of two.

Ok, thanks @QilongTang, @Amoursol, I think I'll create a separate task for the legacy saveas instead (as it needs some design) and merge this as-is.

@Amoursol
Copy link
Contributor

Ok, thanks @QilongTang, @Amoursol, I think I'll create a separate task for the legacy saveas instead (as it needs some design) and merge this as-is.

Can you please also add an AC around putting a keyboard shortcut too?

@QilongTang
Copy link
Contributor

@aparajit-pratap @Amoursol @Jingyi-Wen For this particular improvement, I would suggest showing this dialog ONLY when element binding data exists for the current workspace. Element binding for Dynamo sandbox users or non Revit/C3D integration users does not mean much, I believe showing this for every SaveAs as a DynamoCore function is an overkill. Please consider the final state of this feature in Dynamo 3.0

@Amoursol
Copy link
Contributor

@aparajit-pratap @Amoursol @Jingyi-Wen For this particular improvement, I would suggest showing this dialog ONLY when element binding data exists for the current workspace. Element binding for Dynamo sandbox users or non Revit/C3D integration users does not mean much, I believe showing this for every SaveAs as a DynamoCore function is an overkill. Please consider the final state of this feature in Dynamo 3.0

Seconded on this - would be not just annoying, but confusing for users of Dynamo without Element Binding.

@aparajit-pratap
Copy link
Contributor Author

aparajit-pratap commented Sep 11, 2023

@aparajit-pratap @Amoursol @Jingyi-Wen For this particular improvement, I would suggest showing this dialog ONLY when element binding data exists for the current workspace. Element binding for Dynamo sandbox users or non Revit/C3D integration users does not mean much, I believe showing this for every SaveAs as a DynamoCore function is an overkill. Please consider the final state of this feature in Dynamo 3.0

@QilongTang yes, that's exactly what I'm doing here. Only if the binding data is non-empty, will the dialog show up.

@QilongTang QilongTang added this to the 3.0 milestone Sep 12, 2023
@QilongTang
Copy link
Contributor

QilongTang commented Sep 12, 2023

@aparajit-pratap I forgot to mention that a Jira task was already created before for the SaveAs improvement see https://jira.autodesk.com/browse/DYN-6212

@aparajit-pratap aparajit-pratap merged commit dea73fb into DynamoDS:master Sep 13, 2023
@Jingyi-Wen
Copy link

@aparajit-pratap I too would err on the side of information here, and given the "Save As" button isn't as common as "Save" I'm OK with creating an additional dialog, but do agree that a more holistic approach would be ideal pending size of scope. @Jingyi-Wen thoughts?

Apologies for not seeing this until now. For the minimal - can we provide a tooltip for "Element binding"? Given this might not be something beginner/intermediate users would be familiar with, this could be too scary for them.

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.

5 participants