Skip to content

Named undo blocks#279

Open
vincentullmann wants to merge 2 commits into
ynput:developfrom
straylondon:named_undo_blocks
Open

Named undo blocks#279
vincentullmann wants to merge 2 commits into
ynput:developfrom
straylondon:named_undo_blocks

Conversation

@vincentullmann

Copy link
Copy Markdown
Contributor

Changelog Description

wrap load, update and delete into named undo steps

Additional review information

This PR adds a new context manager which can be used to wrap functions into a single named Undo-Step.
This improves readability when users look at their undo/redo steps

Note:

For now I've only updated the LoadClip loader. Once the general Idea/Implementation is approved I'm happy to roll it out to other loaders too

load
image

update
image

delete
image

Testing notes:

  1. load, update and remove a product (using the LoadClip loader)
  2. check the undo history

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

best to view this without whitespace.
Most of the lines only have an change in indentation

@BigRoy BigRoy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice, I like it.

Is there a reason why you only updated load_clip.py and not the other loaders?



@contextlib.contextmanager
def undo_step(name: str = ""):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Where does the step terminology come from?

If just something you came up with - can we make it chunk instead? Only reason is then at least it matches with ayon-maya and ayon-fusion, ayon-silhouette, ayon-cinema4d

@@ -180,59 +180,59 @@ def load(self, context, name, namespace, options):
inpanel=False
)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Previously this also stopped the active viewer. Is it safe without that? @jakubjezek001

name = container.get("name") or read_node.name()
name = f"Update: {name} v{old_version} -> v{version_name}"
nuke.Undo.name(name)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change

Comment on lines +440 to +443
if version := container.get("version"):
undo_name = f"Remove: {name} v{version}"
else:
undo_name = f"Remove: {name}"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's simplify this - I really would like to remove version as container data in Nuke in time anyway - it's not standard practice to have that imprinted. So let's avoid using it here.

Let's name it purely by the name here.

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.

2 participants