SketchUp / api-issue-tracker

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

PagesObserver#onPageUpdate() observer callback #26

Open DanRathbun opened 6 years ago

DanRathbun commented 6 years ago

SketchUp Ruby API Feature Request

The API is missing an onPageUpdate() observer callback.

Tests show nothing firing when a page is updated. I attached a test observer to the page, the pages collection and the entities collection. Got no response.

1. Use-case:

One case is what is described in Eneroth3's FR: https://github.com/SketchUp/api-issue-tracker/issues/25 When a page update callback fired, she could check her extension's data to sync it with what the page is doing with the view's properties, ... camera, section planes, etc.

2. Suggested pattern / parameters:

One pattern of arguments passed into the callback could be: pages, page, old_flags, new_flags. It would make dealing with changes much better if the flags prior to the update could be compared with the new flags.

ADD: This would be a bulk callback pattern where 1 call would be made after a scene page update. If this pattern is chosen, the flags would be st be passed as hash arguments as described below in post 5 ...

def onPageUpdate(pages, page, old_flags, new_flags)
  if old_flags != new_flags
    # The page property set changed !
    changed = (old_flags - new_flags).abs
  end
end

OR, ...

... another pattern of arguments passed into the callback could be: pages, page, property, old_value, new_value.

This would be an individual callback pattern where a call would be made for each changed property, either after a scene page update ... or immediately after the actual change of an individual property or update flag.

In this scenario the callback name does not need to reflect "update" as there are several properties that can change independent of an update, which have no current observer firing (or that helps identify the page and property that changed. Ie PagesObserver#onContentsModified fires for some changes but does not pass the reference of the page whose property changed, nor the property that changed, making the firing almost worthless to coders. See list below for details.)

So given this (and the fact that EntityObserver#onChangeEntity does not fire at all for scene page entity objects (when it should have,) it might be better to name the callback "onPageChanged" ...

... and have it fire for changes to: :name, :description, :delay_time, :include_in_animation, :transition_time, etc., in addition to the update flags (and possibly the page's position in the collection as PagesObserver#onContentsModified fires once when a page's position is changed but doesn't give any info to determine which page was moved, and what other pages had their positions adjusted accordingly.)

def onPageChanged(pages, page, property, old_value, new_value)
  if property == :use_axes && new_value
    # The axes is being saved, so ...
    MyPlugins::AxesHistory.save_axes(page.name, page.axes.transformation)
  end
end

2. Suggested implementation / reasoning:

I think it best that this callback be added to the Sketchup::PagesObserver class documentation, as that is where coders will be looking for it. (Yes observers are abstract now, but this is about where coders "attach" observers, see below)

This is also where they are likely to wish to attach such an observer. (It is tedious to be forced to attach an observer to each item in a collection, so I'd vote against putting this in the Sketchup::EntityObserver class, which would force coders to attach to each and every page instance. Ie, SketchUp should poll the observers attached to the collection for a callback method, instead of the possibly many observers attached to the page entity itself. (Easier and faster for everyone!)

~

DanRathbun commented 6 years ago

I suppose however if the C-side did the heavy interrogation of the flags (and other properties: name, description, include_in_animation,) and told us what changed, that'd be nice also. :grin:

Perhaps an array of flag integers that changed, using -1 for name, -2 for description, and -3 for include_in_animation. (Those that did not change would not be in the array.)


Incidentally, changing the page description does not fire the PagesObserver#onContentsModified, but changing the page name or include_in_animation does.

However this is almost worthless, as this callback doesn't even tell you which page in the collection was changed ! Coder would be needing to cache the states of the entire collection. This is not efficient!

thomthom commented 6 years ago

Logged as SU-38573

DanRathbun commented 6 years ago

I suppose however if the C-side did the heavy interrogation of the flags (and other properties: name, description, include_in_animation,) and told us what changed, that'd be nice also. grin

Perhaps an array of flag integers that changed, using -1 for name, -2 for description, and -3 for include_in_animation. (Those that did not change would not be in the array.)

Just noticed that the Sketchup::ShadowInfoObserver#onShadowInfoChanged callback does a similar thing by returning an integer index of what property was changed.

The difference here would be that the onPageUpdate() would be a bulk change callback, whereas the shadow info changes are fired individually.

DanRathbun commented 4 years ago

@DanRathbun said: I suppose however if the C-side did the heavy interrogation of the flags (and other properties: name, description, include_in_animation,) and told us what changed, that'd be nice also. 😁

Perhaps an array of flag integers that changed, using -1 for name, -2 for description, and -3 for include_in_animation. (Those that did not change would not be in the array.)

Thinking a bit more ... it would be easier and less confusing for everyone if the 2 "flags" parameters were simply hashes whose keys are symbol names matching (as close as feasible) the flag query method names.

Ex 1:

def onPageUpdate(pages, page, old_flags, new_flags)
  if old_flags[:use_hidden_objects] != new_flags[:use_hidden_objects]
    # The page's use_hidden_objects property has changed ...
    MyPlugin::PageWizard.update_list(page.model, page, :use_hidden_objects, new_flags[:use_hidden_objects])
  end
end

Ex 2:

def onPageUpdate(pages, page, old_flags, new_flags)
  # Find all the flags that have changed:  
  changed = new_flags.select { |key,value| value != old_flags[key] }
  # Pass the new "changed" hash to an extension method:
  process_page_changes(page.model, page, changed) unless changed.empty?
end
DanRathbun commented 4 years ago

FYI

Massively edited the original issue post after more testing with Page changes and observers (and noticing other page properties that have no observer firing.)

DanRathbun commented 4 years ago

PagesObserver#onContentsModified ...

Does NOT fire for:


As noted, nothing fires for EntityObserver when attached to Sketchup::Page or Sketchup::Pages objects.


Testing ...

module PageSpy # Note: this is a hybrid singleton observer object.

  extend self

  def init(
    model = Sketchup.active_model
  )
    pages = model.pages
    # Attach as a PagesObserver to collection:
    pages.add_observer(self)
    # Attach as an EntityObserver to page entities:
    pages.each {|page| page.add_observer(self) }
  end

  def remove(
    model = Sketchup.active_model
  )
    pages = model.pages
    # Remove from the Pages collection:
    pages.remove_observer(self)
    # Remove as an EntityObserver from page entities:
    pages.each {|page| page.remove_observer(self) }
  end

  def onChangeEntity(page)
    puts "PageSpy: onChangeEntity: #{page}"
  end

  def onContentsModified(pages)
    puts "PageSpy: onContentsModified: #{pages}"
  end

  def onElementAdded(pages, page)
    puts "PageSpy: onElementAdded: page \"#{page.name}\""
    page.add_observer(self)
  end

  def onElementRemoved(pages, page)
    name = page.respond_to?(:name) ? page.name : "(Deleted)"
    puts "PageSpy: onElementRemoved: page \"#{name}\""
  end

end