SketchUp / api-issue-tracker

Public issue tracker for the SketchUp and LayOut's APIs
https://developer.sketchup.com/
38 stars 10 forks source link

Hybrid observers #267

Open Eneroth3 opened 5 years ago

Eneroth3 commented 5 years ago

Today using a single object as multiple observers was brought up in the forum. SketchUp doesn't do any type checking in add_observer, but allow any object of any class to be passed.

The documentation still appears to suggest you should subclass the observer class matching the object you want to observer, and if I'm not mistaking, allowing any class is something of a side effect from the observer overhaul some years ago. Before that I suspect you'd get a NoMethodError if you didn't either subclass the right class or implement every single one of its callback methods, as there was empty callback methods defined in the API observer classes.

To get to the question, can we use a single class, that doesn't subclass any of the API observer classes, as multiple observers at the same time? Is it something that just works by accident and may not work in the future, or could it be considered a good practice in certain cases to avoid boilerplate? If so it would be useful if the documentation explained this.

# Hybrid observer detecting both selection changes and when drawing context changes.
class MyHybridObserver
  # @!group Sketchup::ApplicationObserver

  def onNewModel(model)
    puts "onNewModel: #{model}"
    model.add_observer(self)
    model.selection.add_observer(self)
  end

  def onOpenModel(model)
    puts "onOpenModel: #{model}"
    model.add_observer(self)
    model.selection.add_observer(self)
  end

  def expectsStartupModelNotifications
    true
  end

  # @!group Sketchup::ModelObserver

  def onActivePathChanged(model)
    puts "onActivePathChanged: #{model}"
  end

  # @!group Sketchup::SelectionObserver

  def onSelectionBulkChange(selection)
    puts "onSelectionBulkChange: #{selection}"
  end
end

Sketchup.add_observer(MyHybridObserver.new)
thomthom commented 5 years ago

To get to the question, can we use a single class, that doesn't subclass any of the API observer classes, as multiple observers at the same time?

For the most part, yes. I think there is one or two where you need to subclass for correct behaviour - dimension observer... I'll have to dig around.

But generally the interface is actually calling the methods on the "observer" if they respond to them.

DanRathbun commented 5 years ago

I think there is one or two where you need to subclass for correct behavior - dimension observer

Not the DimensionObserver. I just checked and a normal class (default subclass of Object) works fine. The Sketchup::Dimension#add_observer method does no typechecking to force a subclass of DimensionObserver.

However there is a weird thing in that the DimensionObserver's callback is still defined as an empty method in the superclass, and must not have been "abstracted" as the other observers were in the SU2016 overhaul. Perhaps you (Thomas) refer to that the core does not poll DimensionObservers for a #onTextChanged callback method before making the method call?

It serves no functional purpose to create a subclass of the DimensionObserver and not override the #onTextChanged callback method, (as it is the only callback the observer currently has,) which means there is no need to have the superclass' #onTextChanged callback method because missing_method would never wind up calling super().

I'd like to know which (other) observer classes haven't had their frivolous callbacks removed from the superclass, and which of them are not yet polled for callbacks before calling the callbacks. (Issues should be opened for such observers.)


All that aside, the MAIN reason for having superclasses is to provide common functionality to pass down the inheritance chain so subclasses can benefit from "genetics". But Sketchup API observer classes do not YET provide this hereditary functionality for any subclasses.

One MAJOR drawback to this superclass/subclassing is that coders can instantiate instances of the superclass, which was never intended.

This is why in my testing (of common inherited observer functionality) ... I've chosen instead to create a Sketchup::Observer mixin module, which can be included into classes, or extended into module or singleton objects. As mixin modules act like a pseudo-superclass, appearing in the inheritance chain, there "mixing in" can be tested with #is_a? and various class logical operators, etc. But the best part is that a module can not be instantiated, so using a mixin library module as the repository of observer common functionality, would force this feature to be used correctly.

Eneroth3 commented 5 years ago

The question isn't as much 'can it be done?' as 'can it be recommend to do?'. I'm not as much interested in the current implementation as I am in any possible future one of this API.

thomthom commented 5 years ago

I don't really see any harm in it. I kind of like the flexible pattern. But - there is the caveat that there might be one or two observers that rely on the subclass to be there to correctly handle it.

DanRathbun commented 5 years ago

But - there is the caveat that there might be one or two observers that rely on the subclass to be there to correctly handle it.

Again, I'd like to know WHICH (and add note in docs) and as to WHY it is necessary. As I said above I tested the #add_observer method and it still does no type checking even in the case of adding observers to dimension objects.

DanRathbun commented 5 years ago

The question isn't as much 'can it be done?' as 'can it be recommend to do?'. I'm not as much interested in the current implementation as I am in any possible future one of this API.

And I attempted to answer this both above and in several forum threads on the subject of observer "subclassing".

Let me pare it down to succinct ideas ...


Further reading ...

https://en.wikipedia.org/wiki/Inheritance_(object-oriented_programming)

https://en.wikipedia.org/wiki/Composition_over_inheritance


Now one pattern I'd likely never recommend would be to make a watched object a singleton observer that watches itself, if it is something that the SketchUp engine can destroy at anytime.

An example of a bad singleton observer would be defining singleton callbacks to a particular Face object and then assigning itself as the observer of itself. ie ... face.add_observer(face)

thomthom commented 5 years ago

Again, I'd like to know WHICH (and add note in docs) and as to WHY it is necessary.

I don't remember, and I've not had time to dig further into it. There isn't always time to drop into investigation right away.

But we've made no plans for enforcing sub-classing. The only case I think made it intro production was because of necessity in terms of differentiation - where multiple observers types could be hooked up. We'll find the details when we get to process this issue.