api-issue-tracker icon indicating copy to clipboard operation
api-issue-tracker copied to clipboard

Document observers as abstract?

Open Eneroth3 opened this issue 5 years ago • 5 comments

SketchUp defines empty classes for observers that are expected to be subclassed into custom observers. However it can be easier to think of the observers as interfaces (abstract classes). One benefit is that you can implement several observer interfaces in a single class and reduce the boilerplate needed to re-attach an observer, e.g. when a new model is opened. An empty class in itself can also be confusing. If i remember correctly these classes originally defined empty methods that you could override yourself, but that these were removed for optimization. Documenting this as an interface/abstract class is also consistent to how tools and importers work.

To complicate things further, I think there is one specific observer that the superclass matters for, but I can't remember which.

We could document all observers (except the exceptions) as abstract classes. If we do this we also need to change the examples to not inherit from a superclass, and the phrasing to not mentioning overriding methods.

Eneroth3 avatar Oct 26 '20 13:10 Eneroth3

SketchUp defines empty classes for observers that are expected to be subclassed into custom observers.

... because ...

If I remember correctly these classes originally defined empty methods that you could override yourself, but that these were removed for optimization.

Correct. It was the v2016 observer overhaul that did this.

Documenting this as an interface/abstract class is also consistent to how tools and importers work.

Agreed.

However, these abstract objects in use, need only publicly respond to the appropriate callback method calls.

They do not need to be specifically a class, a subclass, nor an instance object. Since this is Ruby, and everything is an object, these observers (and other interfaces like Sketchup::Animation,) can be ANY type of object.

For example, they can be a module, and I often myself use the extension submodule itself as the toplevel AppObserver object that "watches" model objects as they are loaded, created and destroyed. So the module itself attaches other observer objects (or sometimes itself again) to these model objects as needed.

In addition, abstract observer objects can be any other kind of object, where a coder might need to temporarily have an object act as an observer. Code only needs to attach singleton callback observer method(s) to such an object.

# Where DICTNAME is a predefined local String constant:
str = "Insertion was observed."
def str.onComponentInstanceAdded(cdef,inst)
  inst.set_attribute(DICTNAME, "tagged_as", self)
end
my_definition.add_observer(str)

The example is likely not useful, but it is only shown here to make the point that any kind of object can serve as an observer.

The reason that this is so, is because Sketchup::Entity#add_observer (and any other particular class' definition of an #add_observer method) in the API, does not do any type checking to exclude or enforce any particular kind(s) of objects from being the argument observer object.

This paradigm agility is a good thing!

However, there are also scenarios where the class and instancing pattern is the best and cleanest form for extensions. Especially on Mac where there can be multiple model objects open, and the observer objects hold some state values that are and need to be specific to a particular model object, then instancing a Hybrid observer class object for each model is the best way. It helps prevent "crossed wires", where code can get confused over what objects go with what data. (And, they can automatically get detached and garbage collected when the model object is destroyed.)

A hybrid observer is one that contains callbacks for multiple API observer classes all in one class. It doesn't need to be a subclass of any particular API observer superclass, as there currently is no ancestral functionality to pass on down to descendant subclasses. So the API observer classes are now just places to document abstract callback methods, in places that are (it is hoped) easily discoverable by API coders. In practice I've most often just defined such a hybrid class as a ModelObserver subclass, because it's instances basically get attached to all of a model's collection objects. The class contains all of the callbacks for all of the collection class observers.


Anyway .. I thought we already had an open issue for this? I know we've discussed this in the past.

I boilerplated up an idea script that created some common observer functionality, that all observers could "enjoy". Such an implementation needs to be encapsulated in an object that cannot be instantiated, because we would not want code to create instances of the ancestor object. So, I wrapped it up in an test Sketchup::Observer mixin module. And then mixed it into all API observer classes. This injects the Sketchup::Observer mixin module into their ancestry chain so that the following becomes true:

Sketchup::ModelObserver < Sketchup::Observer
#=> true

... and ...

# Where my_model_spy is an instance of a custom subclass of Sketchup::ModelObserver:
my_model_spy.is_a?(Sketchup::Observer)
#=> true

This can be powerful if you are testing and need to get an array of all the various observer objects. You don't have to do multiple ObjectSpace calls with all the different observer class identifiers to get the list. "One call gets them all".

It also allows devs to write observer classes that would not inherit the mixin, by not defining them as a subclass. Or they could themselves decide not to have any particular API superclass, and mixin the Sketchup::Observer module themselves.

Again, paradigm agility.

~

DanRathbun avatar Oct 27 '20 00:10 DanRathbun

Anyway .. I thought we already had an open issue for this? I know we've discussed this in the past.

Okay, yes ... you yourself opened (in MAY 2019) this discussion thread: Hybrid observers #267

To complicate things further, I think there is one specific observer that the superclass matters for, but I can't remember which.

Yes I explained the situation in the hybrid observers thread, in the 3rd post.

If i remember correctly these classes originally defined empty methods that you could override yourself, but that these were removed for optimization.

The observer classes needed empty superclass methods because the original API writers didn't think to poll observer objects to determine whether they wanted to respond to a particular callback. The core just "brute force" called all callbacks for all observers (until the v2016 overhaul.)

I explain more in detail in the hybrid observer thread's 7th post.

Also, in that issue thread, I annunciate that all observers need to be checked to see if they were missed when the observers were overhauled. (Ie, they haven't had their empty methods removed and the core switched over to polling their observer subclass instance objects.) That issue is tagged as a doc and question issue, so probably need a new investigatory issue for checking observers.

~

DanRathbun avatar Oct 27 '20 01:10 DanRathbun

To complicate things further, I think there is one specific observer that the superclass matters for, but I can't remember which.

https://ruby.sketchup.com/Sketchup/Dimension.html#add_observer-instance_method

Though, this makes me wonder how https://ruby.sketchup.com/Sketchup/ComponentInstance.html#add_observer-instance_method works. Can you not add an EntityObserver to a component instance? (Or component definition/group/definition list.) Some stuff that needs to be looked closer at.

thomthom avatar Oct 30 '20 10:10 thomthom

As said previously, in testing, it seems that none of the #add_observer methods do any type checking (which allows us to create hybrid observer objects.)

DanRathbun avatar Oct 30 '20 22:10 DanRathbun

@Eneroth3 To complicate things further, I think there is one specific observer that the superclass matters for, but I can't remember which.

@thomthom Sketchup::Dimension#add_observer

I just noticed this, response and that it does not really "fit" the question. The linked method is not checking nor does it care what the observer argument's superclass is. It is duck-typing by checking if the argument respond_to?(:onTextChanged) and if so putting the object in the queue for dimension text changes.

This does not however prevent a coder from also attaching a dedicated EntityObserver object to the dimension object that purposefully does not have a #onTextChanged callback defined.

DanRathbun avatar Jun 11 '23 15:06 DanRathbun