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

ModelObserver#onDeleteModel is not called when closing a model #717

Open
merwaaan opened this issue Oct 6, 2021 · 8 comments
Open

ModelObserver#onDeleteModel is not called when closing a model #717

merwaaan opened this issue Oct 6, 2021 · 8 comments

Comments

@merwaaan
Copy link

merwaaan commented Oct 6, 2021

  1. SketchUp/LayOut Version: SU 2017, 2021
  2. OS Platform: Mac

Hi,
We're unable to use ModelObserver#onDeleteModel in our extension to react to a model being closed.

From the documentation, I expected this callback to be executed when closing a model (might have misunderstood the doc; the difference between "delete" and "close" is unclear to me in that context).

Here's a reproducer:

class MyModelObserver < Sketchup::ModelObserver
  def onDeleteModel(model)
    puts "MODEL DELETED" # not called?
  end
end

Sketchup.active_model.add_observer(MyModelObserver.new)

Our use case

We need to detect the model closing on Mac to:

  • free up the memory used by our extension in this model (quite a lot of geometry, can be heavy)
  • most importantly, close our extension's dialogs when the associated model is closed

Right now, the dialogs are not closed because the aforementionned callback cannot be used to do so. Naturally this leads to many errors down the line because the model and the entities that are referenced in the code backing those dialogs do not exist anymore.

Do you know of another way to detect if a model has been closed without polling its .valid? method in every spot where the model/its entities are referenced?

Thanks!

@DanRathbun
Copy link

DanRathbun commented Oct 6, 2021

I do NOT see either the delete nor erase callback being called on Windows.
I have a recollection that one of them would fire when a model object was destroyed.
At some point though, the API started reusing the same model object for all models in a session on Windows.

My test:

class ModelSpy < Sketchup::ModelObserver
  def self.spy
    Sketchup.active_model.add_observer(new)
  end

  def onDeleteModel(model)
    puts "#{self.class.name}: #{__method__} called"
  end
  def onEraseAll(model)
    puts "#{self.class.name}: #{__method__} called"
  end
  def respond_to?(meth)
    puts "SketchUp is polling #{self.class.name} for a #{meth} callback."
    super
  end
end

@DanRathbun
Copy link

For Mac, there is the AppObserver#onActivateModel callback.
It is meant to tell an extension when the user switches between model document windows.

On Windows, since there is only ever one model object open at a time, the AppObserver#onNewModel and AppObserver#onOpenModel callbacks would signify that the old model had been closed, and that data would need to be refreshed for the extension.

So I could suggest that you attach your extension submodule to the SketchUp application as an AppObserver object*. And then whenever the above mentioned 3 callbacks fire, you do the validity test.

* An observer object can be any type of object as long as the callback methods are publicly accessible. So a module can serve well as the AppObserver object because there is only ever 1 application.
Meaning you do not need to write a class and instantiate it. Either make the callbacks module methods by prefixing with self. or put a extend self call near the top of the extension submodule definition.

@merwaaan
Copy link
Author

merwaaan commented Oct 7, 2021

@DanRathbun Yes, we use onActivateModel, onNewModel and onOpenModel. For instance, we have some kind of global dialog that displays data related to the currently active model, and those callbacks help us update its contents when appropriate.

However, we're still missing a way to cleanly detect the closure of a model, which can happen easily on Mac. Unfortunately, it seems that onDeleteModel cannot help us in achieving that.

For the moment, we ended up with a UI timer constantly polling model.valid? + many defensive checks (imagine return if @model.valid? lines scattered everywhere) but that's cumbersome and doesn't seem super robust.

@thomthom
Copy link
Member

thomthom commented Oct 19, 2021

Yea, onDeleteModel is not working well, never has been. close vs delete is indeed confusing. Looking at the underlying logic that trigger the event it's not really suited for public API. I think we'd be better off with a new event that works reliably on both platform: onCloseModel

@DanRathbun
Copy link

DanRathbun commented Oct 19, 2021

Also if the model has modifications, when calling Sketchup::Model#close if the user answers No to the save model message box query, then no observer callbacks at all will fire. So there is no way to know that the edits were discarded by extensions that may need to know.

If the user answers Yes then we get the usual procession of observer calls ...

Sketchup.active_model.close

... output from the above ModelSpy ...

SketchUp is polling ModelSpy for a onPreSaveModel callback.
SketchUp is polling ModelSpy for a onTransactionStart callback.
SketchUp is polling ModelSpy for a onTransactionEmpty callback.
SketchUp is polling ModelSpy for a onSaveModel callback.
SketchUp is polling ModelSpy for a onPostSaveModel callback.

@DanRathbun
Copy link

I still see no calling of Sketchup::ModelObserver#onEraseAll even though I call ...

Sketchup.active_model.entities.clear!

At the same time it looks like Sketchup::EntitiesObserver is lacking in that it has an #onEraseEntities callback, but it provides no information that is useful and appears to not be getting called. Viz:

class ModelSpy < Sketchup::ModelObserver
  def self.spy
    ispy = new()
    Sketchup.active_model.add_observer(ispy)
    Sketchup.active_model.entities.add_observer(ispy)
  end

  def onDeleteModel(model)
    puts "#{self.class.name}: #{__method__} called"
  end
  def onEraseAll(model)
    puts "#{self.class.name}: #{__method__} called"
  end
  def onEraseEntities(entities)
    puts "#{self.class.name}: #{__method__} called"
  end
  def respond_to?(meth)
    puts "SketchUp is polling #{self.class.name} for a #{meth} callback."
    super
  end
end

We get this weird output with a new model and only a Sumele instance ...

Sketchup.active_model.entities.clear!
SketchUp is polling ModelSpy for a onTransactionStart callback.
SketchUp is polling ModelSpy for a onTransactionCommit callback.
SketchUp is polling ModelSpy for a onElementAdded callback.
SketchUp is polling ModelSpy for a onElementRemoved callback.
SketchUp is polling ModelSpy for a onElementModified callback.

Perhaps the #onEraseAll callback should be implemented for the Sketchup::EntitiesObserver abstract class ?

... and deprecated for the Sketchup::ModelObserver ?

@DanRathbun
Copy link

DanRathbun commented Oct 19, 2021

It would be helpful for the documentation to explain just when and why these two observer callbacks will or won't fire.

@sketchupbot
Copy link

Logged as: SKEXT-3255

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants