Model#start_operation taking optional code block

[Eneroth3]

A few times I've seen new developers indent code after start_operation, as if it started a new code block. In many ways this would make sense - start_operation correspond to a commit_operation and putting code in between has a special meaning from an API point of view.

I think the API would be more user friendly and allow for nicer code if start_operation, just as File.open, could be called with a block. When the block finishes the operation is committed, with no need for an extra method call. If an exception is raised within the block the operation could be aborted, with no need for a rescue and abort_operation call.

[thomthom]

I agree - block pattern should have been there from the beginning. And should have been the "default" way to do operations.

[MSP-Greg]

If an exception is raised within the block the operation could be aborted, with no need for a rescue and abort_operation call.

No. The block return should be used.

[Eneroth3]

No. The block return should be used.

I don't follow. The block return should be used instead of aborting the operation on an exception? Used for what?

[thomthom]

You mean true/false value to abort or not? I'm not convinced about that. I can easily see people forgetting to end the block with a true/false. And since everything in Ruby is truthy/falsy this could lead to unexpected weirdness since Ruby implicitly return the value of the last line.

Aborting an operation is not in the happy path - I think it makes sense to wire that up to any unhandled exceptions from the block.

[MSP-Greg]

Using exceptions for what's really a flag or status isn't really typical. Obviously, you could define a specific error/exception for it. If other errors were raised, what should SU do? Process and the re-raise? A few issues there.

Blocks are part of Ruby, If people can't get their returns straight, I don't know.

Finally, having seen many other 'forums' where feature requests are discussed, new API statements or functionality changes aren't often considered when a shim consisting of a few lines will match the desired functionality. Below example is not production grade...

def block_operation(blah)
  model.start_operation(blah)
  yield
  model.commit_operation
rescue OP_ERROR
  model.abort_operation
rescue
  # other issues?
end

[thomthom]

I don't see what you are referring to as a flag or status. The purpose of a block is to ensure the operation is committed. If there is an error it makes sense to abort it.

Having the return value of the block controlling whether to abort the operation after it's completed seems like an odd logic.

[MSP-Greg]

First of all, I've got testup-2 running from the console, so while afk, it can run as many loops as I set. It is not stable. Haven't worked enough to determine whether it's SU or the test code. If it is SU, the stability issues are bugs, and possibly hard for devs to repro. Also, many of the requests here are new functionality or things not easily implemented with a small shim. What would I rather have Trimble working on?

This would be an API addition, and would obviously not be backward compatible. Given that it can be done with a small amount of code, I don't think it's worth adding.

If there is an error it makes sense to abort it.

We haven't clarified whether this is an error raised in code or by Ruby itself. One or the other, or both?

So an error aborts. Then what? Is the error raised again? Does it add to the call stack? The idea of a silent Ruby failure is not a good idea.

I used to have a plugin that imported / exported to two proprietary formats, along with several dialogs to help parse extraneous entities from the model. While testing, on import errors, I'd prefer to have all ops committed as the model might show graphically what the issue was.

[DanRathbun]

While testing, on import errors, I'd prefer to have all ops committed as the model might show graphically what the issue was.

First of all. The block form would be optional. Complex code situations still can use the old (normal) syntax. (Example, Ruby Tools where the start and the commit are often fired from different event driven callback methods.) The addition of a block form operation method should not affect code that is "in the wild", nor coders who decide not to use the block form method.

Secondly, I originally (and formally) filed a request for the block form back in 2012 during the testing for the SketchUp 2013 version. So this is not a new idea. (@thomthom Please check the internal FR dB for that request number.) It was requested mainly as a simple means of indoctrinating newbs to Ruby (and the SketchUp API especially) into coding simple undo operations. (Later as their knowledge grew, they could learn the more complex forms, as their needs dictated.) But it could also be used by "old hands" doing simple tasks that did not need the more complex form.

Thirdly, my request was for 1 or 2 new methods, NOT changing the current methods. I believe that changing the current method would cause confusing documentation and be not easily duck-typed. IE, it is a no-go to duck-type the number of arguments for API methods implemented in C. But simple to test a model object if it responds to a method name. So, IMO, having new block form methods named operation() and/or perform() would be better all around. (See example below.)

---

I gave privatley (in Sept. 2014,) a test only example to the SketchUp Development Team that defined two block form methods that handled exceptions differently (I think.) This example defined the methods within the Sketchup::Model class, and was labeled for testing only. For those interested here it is (in it's original 2014 state.) Again it is for TESTING ONLY. Model_operation_instance_method.zip They said that they liked the ideas, but didn't have time to implement them that cycle, nor the next. Also by the 2014 cycle, they were well into planning the observer overhaul and were reluctant to implement until after that.

I also posted (in Oct. 2014,) a public example at my BitBucket SAE repo, that also explored the idea of a reusable SAE::Operation class. https://bitbucket.org/SketchUp_Projects/sae-operation-class/src/5a337651d035e439551d4bd82f9c67041284b0b9/SAE/Operation.rb?at=default&fileviewer=file-view-default

Both of these examples predated the internal SketchUp undo stack / observer overhaul that occured 3 years later, for the SketchUp 2016 version release. So, these examples may no longer work as they did in earlier versions. (Additional testing would be required.)

Since the observer overhaul was completed, attention has been on the C API implementation and parity with the Ruby API, which had mostly put the brakes on all new Ruby API feature implementations. Now that C API / Ruby API parity is mostly complete, new features can be implemented again.

Lastly, just because us Rubyists know how to write a block shim does not mean that newbs will know, nor that an API will not benefit from block form methods. (We've recently also discussed temporary group block form methods.)

[kengey]

For me it would make more sense if we could retrieve the name of the last operation. If I recognize the name I could f.e. make my current transaction transparent.

Maybe, the way rivaling products handle this could be used for inspiration as well: Revit transactions

[DanRathbun]

For me it would make more sense if we could retrieve the name of the last operation.

I also had requested this back then. But as of yet no real access to the undo stack has been exposed.

[Eneroth3]

Great mind thinks alike I presume.

The differences in these requests are just cosmetic. I would however argue it is better to use an optional code block with the existing method than adding a separate method. This way users never risk mixing them up or be confused which one is which. The information whether a block should be used or not is already defined by the presence of said block, and it would be redundant to specify it once more by spelling the method name slightly differently. Also it is more consistent o File.open.

[kengey]

You mean something like this?

class Blockeration

    def self.start(name, &block)
        # perhaps also allow for disable ui etc...
        model = Sketchup.active_model 
        model.start_operation name
            begin
                block.call()
            rescue
                model.abort_operation
            else
                model.commit_operation
            end
    end #def
end #class

Blockeration.start("hello") do
    Sketchup.active_model.entities.clear!
end

It still allows for the same troubles: other plugins (or even your own) reacting to observers being called while being in your in-operation-block and creating their own operations.

[MSP-Greg]

@Eneroth3

I would however argue it is better to use an optional code block with the existing method than adding a separate method.

Which breaks compatibility with older SU versions. Hence, I assume many plugin devs would not use the optional block.

Back to what I said before. Trimble has plenty of things to fix/add before they work on breaking API changes. Especially one that can be done easily in Ruby...

[Eneroth3]

@MSP-Greg

This wouldn't break any compatibility; older plugins would work just as they used to. However, like all new features, it would of course not be used by developers that want their extensions to be backward compatible, just as with HtmlDialog, InstancePath, ImageRep etc.

Fun fact: I'm right now working on my first extension that relies on HtmlDialog. I was very happy when it was announced but didn't use it much for the first few years as I want my extensions to work in a few SU versions back. I'm still 140% HtmlDialogs was worth adding though.

That said this would be a somewhat cosmetic edition only, so I would understand if it doesn't get prioritized.

[kengey]

I also had requested this back then. But as of yet no real access to the undo stack has been exposed.

It would make much sense for me if one could:

• Get the name of the last operation.
• Simple not being able to perform changes to the model when not in an operation
• Know if there currently is an open operation
• Perhaps not being able to abort/commit an operation started by another extension. model.start_operation could return a token, which in return is needed to abort/commit the operation. This could work perfectly with also allowing for a code-block, so that devvies don't have to care about tokens etc... for example:

class Blockeration

    def self.start(name, &block)
        # perhaps also allow for disable ui etc...
        model = Sketchup.active_model 
        token = model.start_operation(name)
        begin
            block.call()
        rescue
            model.abort_operation(token)
        else
            model.commit_operation(token)
        end
    end #def
end #class

Blockeration.start("hello") do
    Sketchup.active_model.entities.clear!
end

[DanRathbun]

I would however argue it is better to use an optional code block with the existing method than adding a separate method.

It is not better by any means.

A coder would need to decide whether to use the "old" pattern, or the new block form, using a branching statement. Ie, they cannot use the block form for old versions as the block would just be ignored.

So, in most cases, the coder must determine whether the current running API supports the block form (so they can use it and NOT use a pattern that uses #commit_operation.)

Using complex boolean expressions involving the testing of version numbers is not desirable for branching decisions. Instead, we should seek to "duck-type" or test for the existence of functionality.

As I said above, for Ruby methods that are implemented in C (ie, the entire SketchUp API,) asking the #start_operation method what it's arity is will always return -1 as it takes a variable number of methods.

Secondly, for a method implemented in C, that takes an optional block, the #parameters method returns no information that a block is accepted. (For Ruby 2.0, it only gives block argument information if a method is implemented in Ruby with a &name parameter. Not sure if this has been improved in later versions.) Old SketchUp versions running Ruby 1.8, do not have the Method#parameters method, so that would further complicate support branching, if one relies upon Method#parameters.

Thirdly, the name "start_operation" indicates the start of something, not the whole of something, so a better named method is desirable.

Fourthly, there is a problematic parameter next_transparent that is discouraged (and deprecated,) and implementing a new method gives a opportunity to drop this parameter; and also to reorder the argument list, and make the disable_ui parameter have a true default instead of the #start_operation method's false default.

Fifthly, a user would be looking for a method named "operation" to do an operation and not something beginning "start_...". (This has been said by new coders in the past, where they just don't notice the current methods. They might have, if they'd been all 3 been named "operation_start", "operation_commit", "operation_abort".)

Sixthly, the API uses YARD to generate docs. And it has been a struggle to get overloaded methods to display in a non-confusing manner.

---

It just makes better sense to have a new method, and branch ...

opname = "Do Stuff"
if model.respond_to?(:operation)
  model.operation(opname) do
    do_this()
    do_that()
    do_more()
  end
else # use old pattern
  begin
    model.start_operation(opname,true)
      do_this()
      do_that()
      do_more()
    model.commit_operation()
  rescue
    model.abort_operation
  end
end

~

[MSP-Greg]

@Eneroth3

just as with HtmlDialog, InstancePath, ImageRep etc.

The distinction I'm making is that the above are new features that fix large issues (HtmlDialog), add new functionality (ImageRep), or add features that would require a fair bit of code (InstancePath).

This issue doesn't really fit into any of the above categories. Given that, why add something that very likely would be similar to your use of HTMLDialogs, in that many dev's wouldn't even use the new API for a few versions of SU? Especially when they can write their own utility function that works exactly as they require?

And, you're right, by the most commonly used definition of 'breaking', this isn't, but it would be considered easily 'shimable'...

[Eneroth3]

A coder would need to decide whether to use the "old" pattern, or the new block form, using a branching statement. Ie, they cannot use the block form for old versions as the block would just be ignored.

No one would use a branching statement for a cosmetic update like this. You use the old pattern until you decide to drop support for the SU versions not supporting the new one.

Thirdly, the name "start_operation" indicates the start of something, not the whole of something, so a better named method is desirable.

The Ruby Core re-use File.open for blocks, not a new File.do or File.use so I'd say it's more consistent, and therefore more expected, to use the same method.

Fourthly, there is a problematic parameter next_transparent that is discouraged (and deprecated,) and implementing a new method gives a opportunity to drop this parameter; and also to reorder the argument list, and make the disable_ui parameter have a true default instead of the #start_operation method's false default.

If a new method is added or the existing re-used, the arguments MUST be identical. Doing anything else for such similar methods would be extremely confusing. It would be impossible to keep track on what method uses what arguments without knowing the history behind them.

Fifthly, a user would be looking for a method named "operation" to do an operation and not something beginning "start_...". (This has been said by new coders in the past, where they just don't notice the current methods. They might have, if they'd been all 3 been named "operation_start", "operation_commit", "operation_abort".)

This I can somewhat agree on. It's too late to change the existing methods, but it is an argument for having a separate method named simply operation.

[DanRathbun]

The differences in these requests are just cosmetic.

Incorrect. The differences are fundamental.

This wouldn't break any compatibility; older plugins would work just as they used to.

It is about new code working on both older SketchUp versions and newer versions going forward. Your stance is about what you "think", whereas I have spent the time to write the test code (above) and test it. For me it was and is, not just "think", ... it is experience and knowing.

So I challenge you then, Julia, to write a test override of the #start_operation method that takes a block, and then show us a tested single working pattern that uses an operation for old SketchUp versions (with no block) and new SketchUp versions (that would recognize and use the block.)

[Eneroth3]

The difference is indeed cosmetic. I will not write a method testing whether a block is accepted as I know such a method is redundant. For code to work on old and new SU versions you should stick to the old syntax. There's no reason whatsoever to write the same code with 2 syntaxes, and on top of that write a piece of code to determine what syntax to use.

[thomthom]

Logged as SU-40227

[DanRathbun]

Just to revive this topic a bit.

I've recently (several times) added the following shim to some posted examples.

    def with_operation(op_name, disable_ui = true)
      return unless block_given?
      model = Sketchup.active_model
      model.start_operation(op_name, disable_ui)
        #
        result = yield # call the block
        #
      model.commit_operation
    rescue => err
      puts err.inspect
      model.abort_operation if UI.messagebox(
        "There was an error:\n#{err.message}\n\nDo you wish to Undo ?", MB_YESNO
      ) == IDYES
    else
      return result
    end

NOTE: This example must not use transparency to other operations. Otherwise it could abort the previous operation. (Issue #335)

Up above, I think Greg made the point that it's is difficult to tell what kinds of exceptions that would need to be aborted. I second this. Mainly because custom Sketchup::Model::GeomError exceptions are not raised. Instead various factory methods raise a mismash of core Ruby exceptions.

---

Usage ...

menu = UI.menu("Extensions")
menu.add_item("Nifty Command") {
  with_operation("Niftiness") do
    nifty_command()
  end
}

Of course it can be used anywhere in the code that needs to create a separate operation.

---

And also this might be a good topic for a example / blog article.