OnCancel - No indication of Undo versus Redo

[Fredosixx]

Current behavior of OnCancel: The flag argument does not make the distinction between Undo and Redo.

This is an old problem, which I hope should be overcome one day.

The benefit of OnCancel is that it fires BEFORE the Undo or Redo action is performed. So it's relatively easy to decide what to do with it, and possibly cancel it.

However, because you don't know whether it is related to an Undo or a Redo, you cannot use it in all cases. And if you let it go and then find out whether it's an undo or a redo (via Observers), it's a little bit late. And it creates complex side effects because any Undo or Redo would change the Ruby object references of the entities affected. So you have then to reconcile with using entityIDs.

REQUEST: Do something about it. Two suggestions below:

1. Creating 2 new methods (e.g. onUndo, onRedo), but making sure they fire BEFORE the action is performed.

2. Amending the current onCancel method with passing a 3rd argument (if arity of method is 3) indicating whether this is undo or redo. This way, this should limit the issues of backward compatibility.

[DanRathbun]

Do you find yourself trying to use the tool instance also as a model observer (attached in #activate, detached in #deactivate) with #onTransactionRedo and #onTransactionRedo callbacks ?

[Fredosixx]

Not really. The Tool instance is used to define and create geometry. Ctrl-Z and Ctrl-Y are natural ways to undo and redo for any Sketchup user, so I need to support their occurrence and react to it properly.

What I noted is that, just because onCancel() does not indicate whether this is Undo or Redo, it makes things always complex and cumbersome to implement. This is because #onTransactionUndo and #onTransactionRedo callbacks are fired AFTER the undo and redo is performed by Sketchup, and that whenever you have a change in the Undo/Redo stack you need to recompute the Ruby reference of objects (which means that you have to store their entityID beforehand).

You have example of that in FredoCorner and most of my other extensions creating geometry.

[DanRathbun]

so I need to support their occurrence and react to it properly.

No doubt. I'm not against improvements in this regard.

Amending the current onCancel method with passing a 3rd argument (if arity of method is 3) indicating whether this is undo or redo. This way, this should limit the issues of backward compatibility.

Adding arguments will ensure backward compatibility is broken ... unless all previous releases of SketchUp already ignore extra arguments in observer callback method definitions.

Instead the simplest and less intrusive means of updating is to leave a reason argument of 2 to mean an undo was done (as the documentation states) and implement a redo value of 3.

An alternate solution could be a redo bit flag of 4 means the geometry action was specifically a redo.

Which would you prefer ?

(I think the bit flag pattern would encourage the most backward compatible code.)

[Fredosixx]

Let's say that the tool need to react differently whether this is a True Cancel or an Undo / Redo.

The author could have written his code in 2 ways, noting that flag takes 2 value, 0 or 2 (1 is never used)

FORM 1

if flag == 0
   #this is a cancel
else
   #this is an Undo / Redo
end

FORM 2

if flag == 2
   #this is an Undo / Redo
else
   #this is a Cancel
end

If the new version of the onCancel() method use 2 or 3 for the flag for Undo or Redo, then the FORM 2 will create an issue.

With the first argument and check of arity, the flag value remains 2, but the 3rd argument tells whether this is undo / redo. This will only be used if the author makes a change to its extension to accommodate the new API form (which may imply to have 2 onCancel() methods depending on the Sketchup version. So it's a bit of artificial complexity.

This is why I would prefer 2 new methods: onUndo and onRedo.

Now, there has been several changes in the API that created potential incompatibility and which overall improved SketchUp. So if the update of flag is done, that should not be a big deal.

[DanRathbun]

With the first argument and check of arity, ...

This isn't backward compatibility, it's forward compatibility.

This is why I would prefer 2 new methods: onUndo and onRedo.

Probably best as this wouldn't effect older versions.
If present in an older version they just wouldn't get called.

[Fredosixx]

This isn't backward compatibility, it's forward compatibility.

For me backward compatibility means that it does not break the current versions extensions...

[DanRathbun]

"Backward" means looking back at older versions. The current version is the most recent older version, as opposed to future versions yet to be released. The method can be made agile in future API versions, but the older releases are "set in stone".
However, if your new code had a 3rd argument with a default value then in older versions there wouldn't be an ArgumentError (2 for 3) raised when the engine only passed 2 arguments. (But this is fragile as it requires coders to adopt a certain pattern for a specific API method.)

[thomthom]

Adding arguments will ensure backward compatibility is broken ... unless all previous releases of SketchUp already ignore extra arguments in observer callback method definitions.

We can check for arity from the SketchUp side and then call with "legacy" arguments if it only takes two argument.

We did that for getMenu in SU2015 to allow the method to pass mouse parameters.
https://ruby.sketchup.com/Sketchup/Tool.html#getMenu-instance_method

However for onCancel I'm not sure what this third parameter would be.

Maybe something on the tool class that dictate behaviour change?

# Allowing tools to return a feature flag option hash to switch behaviour.

# ON_CANCEL_LEGACY = 0b0
# ON_CANCEL_WITH_REDO = 0b1

# CANCEL_REASON_ESC = 0
# CANCEL_REASON_REACTIVATE = 1
# CANCEL_REASON_UNDO = 2 # Also given in ON_CANCEL_LEGACY mode.
# CANCEL_REASON_REDO = 3 # Given in ON_CANCEL_WITH_REDO mode.

class MyTool

  def tool_options
    {
       onCancel: ON_CANCEL_WITH_REDO
    }
  end

  def onCancel(reason, view)
    # Method signature remains the same. Only possible reason code changes
    # depending on `tool_options`.
  end

end

💡?

(This reminds me, we should have constants for the magic values.)

[Fredosixx]

The best for the future would be two new methods OnUndo and OnRedo (firing BEFORE the Undo or Redo happens).
OnCancel would not be changed and could still be used by old plugins.

[DanRathbun]

We can check for arity from the SketchUp side and then call with "legacy" arguments if it only takes two argument.

You're missing my point. I am not talking of legacy extension running in future SketchUp API version. I am speaking of new extension version (with say 3 onCancel arguments) running in old SketchUp API environment where there is no arity check happening.
In this case, the old API would still pass 2 arguments.

So the new extension version would need to write their onCancel callback in an argument agile way.

Dealing with variable arity can also be done by extension coders. They can code defensively by adding *extra to each callback method definition (be it tool or observer) so as to future proof their code (against ArgumentError) when new ("extra") arguments are passed when their "legacy" extension is running in newer API version environments.

Or ... (as said above) ... the coder can add extra argument(s) with default value(s).

[DanRathbun]

Maybe something on the tool class that dictate behaviour change?

Your idea would bring about a pattern like so ...

class MyTool

  ON_CANCEL_LEGACY = 0b0
  ON_CANCEL_WITH_REDO = 0b1

  CANCEL_REASON_ESC = 0
  CANCEL_REASON_REACTIVATE = 1
  CANCEL_REASON_UNDO = 2 # Also given in ON_CANCEL_LEGACY mode.
  CANCEL_REASON_REDO = 3 # Given in ON_CANCEL_WITH_REDO mode.

  def tool_options
    {
      onCancel: ON_CANCEL_WITH_REDO
    }
  end

  def onCancel(reason, view)
    # Method signature remains the same. 
    # Only possible reason code changes depending on `tool_options`.
    if reason == 0
      # This is a cancel in both modes.
      # Do CANCEL code ...
    elsif reason == 1
      # This is a reactivate in both modes. (not used?)
      # Do REACTIVATE code ...
    else
      # This is an Undo / Redo in both modes, so determine mode:
      if tool_options[:onCancel] == ON_CANCEL_WITH_REDO
        if reason == CANCEL_REASON_REDO
          # Do REDO code ...
        else
          # reason == CANCEL_REASON_UNDO
          # Do UNDO code ...
        end
      else
        # Legacy behavior where:
        #   tool_options[:onCancel] == ON_CANCEL_LEGACY
        # Do LEGACY code ...
      end
    end
  end

end

Now imagine what would happen when the above tool was loaded into a legacy SketchUp version.
It would not work correctly, ... it would always execute the "Do UNDO code" block.

There'd need to be a way for a tool object to properly (dynamically) set tool_options[:onCancel].

Normally we like to use ducktyping to check against behavior, but there doesn't seem to be anyway to do it, in this case. So we'd be stuck with a clunky version check ...

  def tool_options
    {
      onCancel: Sketchup.version.to_f >= 21.3 ? ON_CANCEL_WITH_REDO : ON_CANCEL_LEGACY
    }
  end

Unless, for the redo functionality release, y'all also define some constants we can check against. Ie ...

  def tool_options
    {
      onCancel: defined? TOOL_ON_CANCEL_WITH_REDO ? TOOL_ON_CANCEL_WITH_REDO : TOOL_ON_CANCEL_LEGACY
    }
  end

[DanRathbun]

---

OR ... perhaps simplify things, and just add an API features hash and into it a boolean flag value that tools can check. In this example, assume that a new module method Sketchup::features is implemented that gives access to a READ ONLY hashlike object, with key/value pairs that extensions can access for information or conditional branching, etc.*
...

class MyTool

  def onCancel(reason, view)
    # Method signature remains the same. 
    # Only possible reason code changes depending on redo support.
    if reason == 0
      # This is a cancel in both modes.
      # Do CANCEL code ...
    elsif reason == 1
      # This is a reactivate in both modes. (not used?)
      # Do REACTIVATE code ...
    else
      # This is an Undo or a Redo, so determine redo support:
      if Sketchup.features[:tools_support_redo_on_cancel] rescue nil
        if reason > 2
          # Do REDO code ...
        else # reason == 2
          # Do UNDO code ...
        end
      else # running a legacy API version
        # Do LEGACY code ...
      end
    end
  end

end # tool

And if your rubocop doesn't like rescue in modifier position (to react to NoMethodError,) then for the if condintion you can do ...

      if Sketchup.respond_to?(:features) && Sketchup.features[:tools_support_redo_on_cancel]

---

Note that a "features" hash would be informational only and would not allow setting or toggling features.

* Any application level adjustable settings should (as I have already requested) be exposed via a Sketchup::options module method which would return a reference to a Sketchup::OptionsManager instance object.

~

[Fredosixx]

I really think that the simplest and cleanest is to crate two new callback methods: onUndo and onRedo.
This way, we preserve the compatibility and makes things clearer in the API

[DanRathbun]

I know ...

Fredosixx said: This is why I would prefer 2 new methods: onUndo and onRedo.

DanRathbun said: Probably best as this wouldn't effect older versions.
If present in an older version they just wouldn't get called.

... I agree.

But I was responding to Thomas' musings. Ie... "throwing some ideas around".

Even with your idea ... In a newer API version, your onCancel callback would need some way to decide NOT to run the legacy clause that is probably going to be within it.

Unless you can figure out some nifty way to conditionally define the onCancel callback. If the devs defined some constants then they could be used ie ...

if defined? TOOL_ON_CANCEL_WITH_REDO
  def onCancel(reason, view)
    # REDO EDITION
  end
else
  def onCancel(reason, view)
    # LEGACY EDITION
  end
end

---

OR ... are you thinking that the onCancel callback would not get called for a Undo or a Redo in this future version ?

[Fredosixx]

onCancel should continue to behave as it does.

Up to the developer to take benefit of the 2 new methods onUndo and onRedo.

This ensures full compatibility and freedom.

[DanRathbun]

onCancel should continue to behave as it does.
Up to the developer to take benefit of the 2 new methods onUndo and onRedo.

I would say then that the new onUndo() and onRedo() should get called before onCancel() so that the coder can set a state variable that an undo/redo has been processed already.

This would allow the onCancel() to have a conditional that returns (or does nothing) when reason > 1 ...

class MyTool

  def onCancel(reason, view)
    # Method signature remains the same. 
    # Only possible reason code changes depending on redo support.
    if reason == 0
      # This is a cancel in both modes.
      # Do CANCEL code ...
    elsif reason == 1
      # This is a reactivate in both modes. (not used?)
      # Do REACTIVATE code ...
    else
      # This is an Undo or a Redo, so ...
      unless @redo_processed || @undo_processed
        # Neither have yet been handled so,
        # Do LEGACY code ...
      end
    end
    reset()
  end

  def onRedo(view)
    # Do REDO code ...
    @redo_processed = true
  end

  def onUndo(view)
    # Do UNDO code ...
    @undo_processed = true
  end

  def reset
    # ... other tool reinitialization of state ...
    @redo_processed = false
    @undo_processed = false
  end

end # tool

EDIT: Updated example above to show possible onRedo and onUndo tool callbacks as well as use of an internal reset() method.
~

[DanRathbun]

Another option (not mutually exclusive) would be for the SketchUp engine to take some direction from the onUndo and onRedo return values ...

• truthy subsequently calls onCancel
• falsely skips calling onCancel afterward
• If undefined will call onCancel as it does now (legacy mode)

For example, if legacy code always assumed an undo, updated code could leave onUndo undefined and continue handling an undo in the onCancel callback, but also define onRedo to especially handle a redo and have it return false to prevent a subsequent call to onCancel.

ADD EXAMPLE: An example (like that above) showing possible coding pattern if onRedo and onUndo callback return values can control whether onCancel is subsequently called in future API version to avoid legacy code evaluation.

class MyTool

  def onCancel(reason, view)
    # Method signature remains the same. 
    # Only possible reason code changes depending on redo support.
    if reason == 0
      # This is a cancel in both modes.
      # Do CANCEL code ...
    elsif reason == 1
      # This is a reactivate in both modes. (not used?)
      # Do REACTIVATE code ...
    else
      # This is an Undo or a Redo in legacy version, so ...
      # Do LEGACY code ...
    end
    reset()
  end

  def onRedo(view)
    # Do REDO code ...
    reset()
    false # Don't call onCancel
  end

  def onUndo(view)
    # Do UNDO code ...
    reset()
    false # Don't call onCancel
  end

  def reset
    # ... tool reinitialization of state ...
  end

end # tool

[DanRathbun]

NOTE-FYI-TWIMC: Updated last 2 posts and pseudo code examples.

[DanRathbun]

Another option (not mutually exclusive) would be for the SketchUp engine to take some direction from the onUndo and onRedo return values ...

• truthy subsequently calls onCancel
• falsely skips calling onCancel afterward
• If undefined will call onCancel as it does now (legacy mode)

BUT ... Julia has noticed what appears to be not well planned and undocumented behavior concerning the calling of Tool callbacks, viz:

@Eneroth3 in #176, said:

I just noticed that a truthy return value from onKeyDown also prevents onCancel from being called when pressing escape. I think this quirk really needs to be mentioned in the API, as it can be virtually impossible to make a tool reset on Esc without knowing this.

... which (the current behavior as described by Julia) is opposite of what I propose (above) and not intuitive.

Thomas mentions how JS callback work, which I thought use the "bubble up" concept. Ie, true to "bubble up" (the default?) and false for not.

[thomthom]

Side note @Fredosixx :

The benefit of OnCancel is that it fires BEFORE the Undo or Redo action is performed. So it's relatively easy to decide what to do with it, and possibly cancel it.

How do you cancel an upcoming undo/redo?

[Fredosixx]

How do you cancel an upcoming undo/redo?

You just start a FAKE transaction

model.start_transaction 'fake', true

[thomthom]

Huh! I wasn't aware of that. hm.. wonder why that blocks the upcoming operation. I have to dig into the source.

[Fredosixx]

I assume that Sketchup undo the Fake operation, leaving all what was previously done unchanged

[thomthom]

That - sounds very plausable!

[Fredosixx]

I hope this does not change...

This is a way to manage the Undo / Redo in an interactive tool, because you have to keep track of the object references, based on the map of their entityID (or persistentID).

[Eneroth3]

The easy way to handle undo/redo in an interactive tool is to commit small operations for every change the user is seeing, rather than having a long operation you commit at the end.

[DanRathbun]

How do you cancel an upcoming undo/redo?

You just start a FAKE transaction

model.start_transaction 'fake', true

I think you mean model.start_operation ;)

[Eneroth3]

Would adding ´onBeforeUndo´ and ´onBeforeRedo´ to the ModelObserver solve the use case in the original post? This could be used inside of a tool but also outside of it so it's a more general approach.

[Fredosixx]

It would help, but dedicated methods for Sketchup::Tool would be easier.

As you know, it is always painful to set observers for models, because you need to take care of reactivating them when the model is changed. Not to mention the Mac, where keeping track of observers is a total mess.

I always wondered why the onCancel method had a flag for Undo but not for Redo.

[DanRathbun]

I think Julia's double-duty callback idea has great merit. (You just know if tools got these callbacks, that other coders would be wanting them outside tools, for other situations.)

As you know, it is always painful to set observers for models, because you need to take care of reactivating them when the model is changed.

What about pattern like so ?

(Remember, that observer objects need only expose publicly available callback methods, which can be any kind of object. A module, a class instance, whatever.)

class MyTool

  def initialize(model)
    @model = model
    @active = false
  end

  def activate
    @model.add_obsever(self)
    @active = true
    puts 'Your tool has been activated.'
  end

  def deactivate(view)
    @active = false
    @model.remove_obsever(self)
    puts 'Your tool has been deactivated.'
  end

  def resume(view)
    @active = true
  end

  def suspend(view)
    @active = false
  end

  def onBeforeRedo(view)
    return false unless @active
    # reactive code
    false # Don't call onCancel
  end

  def onBeforeUndo(view)
    return false unless @active
    # reactive code
    false # Don't call onCancel
  end

end

# Usage ...
model = Sketchup.active_model
model.select_tool(MyTool.new(model))

Behind the scenes the API could do automatically what I show above in boilerplate.

Ie, as you know all observers are really abstract, so even if the callbacks are part of the ModelObserver paradigm the underlying feature can be used in multiple places.

So it could be possible that when ever a tool is active, that the SketchUp observer engine will poll the tool object for these callbacks. (On the C side of things, the same functionality can be exposed to the Ruby API in different ways and places.)

~

[Fredosixx]

Indeed. That's why I use with the current 'AfterUndo' observers.

Still, the BeforeUndo observer is provided for free with the OnCancel method and flag = 2. It's a pity that the same flag is used for both Undo and Redo, whereas it could have made the difference.

[Eneroth3]

Given what we already have now adding a redo flag to onCancel may seem like a small step but I suspect it is quite far from what the method was planned to be used for when implemented. Native tools seem to react identically to a change in the model history and pressing Esc, i.e. resetting to their initial state. Maybe there is some nuance to what the "initial state" is for tools using a pre-selection which could explain why the reason parameter exists to begin with, I haven't tested that.

I've done a text search through my own code to see if I'm using the reason parameter, and my component replacer uses it to drop the sampled component and go back to the sample state on Esc but not on model history. Same with layer painter. I also have a tool in Eneroth Townhouse System where Esc drops the last point the user has placed rather than going back all the way to the initial state.

I think we are more in observer territory than specific tool territory. The low-weight implementation for observers Dan shown above also helps a lot to make observers less cumbersome. Learning that patter was a but of a mindblower for me.

[Fredosixx]

As said, either or both would be an improvement.

I am always careful with observers, because you never know what you can do or not within the callback.

Very often, I cancel the Undo or Redo by starting a fake transaction ( with model.start_operation), usually when the Tool just draw things in the viewport without creating real geometry.

[DanRathbun]

... and then we have the fact that since v2016, observers cache transactions, but tool objects are immediate kinds of interfaces.

It is not an easy "nut to crack".