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

EMSUSD-189 detect invalid use of undo item #3227

Merged
merged 6 commits into from
Jul 19, 2023

Conversation

pierrebai-adsk
Copy link
Collaborator

The OpUndoItem must eventually be kept in a OpUndoList or deleted. This is usually done by using either a OpUndoItemRecorder (to keep them) or OpUndoItemMuting (to delete them). Failure to do so would leak undo items and is a symptom of a programming error where no recorder was created when they should.

Add code to detect such errors in non-release builds. Fixed problems found by the validator.

Adjust the CMake project:

  • Add a CMake flag to validate undo item called CMAKE_WANT_VALIDATE_UNDO_ITEM.
  • It is on by default in non-release builds.
  • It is off by default in release builds to avoid emitting errors that users can't do anything about.
  • Add a WANT_VALIDATE_UNDO_ITEM C++ symbol when enabled.

Add the validator:

  • Add OpUndoItemValidator class to do the validation.
  • Add details about orphaned undo items when the validator fails.
  • Add return value to the validator in case calling code want to know the result.
  • Don't trigger validation errors when an explicit undo item list is used instead of the global one.
  • Print a stack trace of where the validation error are found to more easily find the root cause.

Update related undo item classes:

  • Make the recorder and muting classes derive from the validator.
  • Make muting code non-inlined.
  • Add utility functions to the undo item list class to enable validation and testing.
  • Add Python wrapper for the new functionality.

Unit tests and fixes for problems found during testing:

  • Add a unit test to validate that the OpUndoItemList works.
  • The unit test also indirectly exercises the validator in non-release builds.
  • Fix the Python wrapper to not lose the undo items and allow undo and redo.
  • Fix the prim updater manager proxy-changed callback to not leave orphaned undo items.
  • Add undo item muting in multiple callbacks.
  • Correctly mute undo item during the undo and redo of function-based items.
  • Fix all tests that did not properly use a OpUndoItemList to capture the undo item of functions. In particular, the edit-as-Maya, merge-to-USD and duplicate functions.

The OpUndoItem must eventually be kept in a OpUndoList or deleted. This is usually done by using either a OpUndoItemRecorder (to keep them) or OpUndoItemMuting (to delete them). Failure to do so would leak undo items and is a symptom of a programming error where no recorder was created when they should.

Add code to detect such errors in non-release builds. Fixed problems found by the validator.

Adjust the CMake project:
- Add a CMake flag to validate undo item called CMAKE_WANT_VALIDATE_UNDO_ITEM.
- It is on by default in non-release builds.
- It is off by default in release builds to avoid emitting errors that users can't do anything about.
- Add a WANT_VALIDATE_UNDO_ITEM C++ symbol when enabled.

Add the validator:
- Add OpUndoItemValidator class to do the validation.
- Add details about orphaned undo items when the validator fails.
- Add return value to the validator in case calling code want to know the result.
- Don't trigger validation errors when an explicit undo item list is used instead of the global one.
- Print a stack trace of where the validation error are found to more easily find the root cause.

Update related undo item classes:
- Make the recorder and muting classes derive from the validator.
- Make muting code non-inlined.
- Add utility functions to the undo item list class to enable validation and testing.
- Add Python wrapper for the new functionality.

Unit tests and fixes for problems found during testing:
- Add a unit test to validate that the OpUndoItemList works.
- The unit test also indirectly exercises the validator in non-release builds.
- Fix the Python wrapper to not lose the undo items and allow undo and redo.
- Fix the prim updater manager proxy-changed callback to not leave orphaned undo items.
- Add undo item muting in multiple callbacks.
- Correctly mute undo item during the undo and redo of function-based items.
- Fix all tests that did not properly use a OpUndoItemList to capture the undo item of functions. In particular, the edit-as-Maya, merge-to-USd and duplicate functions.
@pierrebai-adsk pierrebai-adsk added enhancement New feature or request adsk Related to Autodesk plugin labels Jul 12, 2023
@pierrebai-adsk
Copy link
Collaborator Author

pierrebai-adsk commented Jul 13, 2023

The only PF failure is due to the known random failures in an export test failing to rename a temporary file.

@pierrebai-adsk pierrebai-adsk added ready-for-merge Development process is finished, PR is ready for merge and removed ready-for-merge Development process is finished, PR is ready for merge labels Jul 17, 2023
@pierrebai-adsk pierrebai-adsk added the ready-for-merge Development process is finished, PR is ready for merge label Jul 17, 2023
Copy link
Collaborator

@seando-adsk seando-adsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please look at your option() in CMakeLists.txt

@seando-adsk seando-adsk removed the ready-for-merge Development process is finished, PR is ready for merge label Jul 18, 2023
- Make the new CMake flag off by default.
- Fix include statement syntax.
@pierrebai-adsk
Copy link
Collaborator Author

The only PF failure is one known random export failure we've been having recently.

@pierrebai-adsk pierrebai-adsk added the ready-for-merge Development process is finished, PR is ready for merge label Jul 19, 2023
@seando-adsk seando-adsk merged commit 99df05d into dev Jul 19, 2023
@seando-adsk seando-adsk deleted the bailp/EMSUSD-189/improve-undo-item branch July 19, 2023 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
adsk Related to Autodesk plugin enhancement New feature or request ready-for-merge Development process is finished, PR is ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants