SketchUp / api-issue-tracker

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

Document abstract interface for DefinitionList#load_from_url #260

Open thomthom opened 5 years ago

thomthom commented 5 years ago

http://ruby.sketchup.com/Sketchup/DefinitionList.html#load_from_url-instance_method

Document this interface similar to the other abstract interfaces we have (See #257 et al)

DanRathbun commented 4 years ago

There are at least two errors in the LoadHandler class example.

1) The constructor call is missing from the example to instantiate the load_handler object. Need to add this statement:

    load_handler = LoadHandler::new

2) The onFailure callback does not call the error= setter method correctly. It should be:

    def onFailure(error_message)
      error = error_message
      Sketchup::set_status_text('')
    end

... or ...

    def onFailure(error_message)
      @error = error_message
      Sketchup::set_status_text('')
    end
(The report is that apparently, calling `self.error=` doesn't work for some reason.)
https://forums.sketchup.com/t/observer-on-file-creation-modification/27514/13
Eneroth3 commented 4 years ago

This class is all documented in a code example for a method and can't be given tags of its own. Should it be documented on its own and get a new page in the docs?

thomthom commented 4 years ago

We have other interfaces like this; Animation for instance: https://ruby.sketchup.com/Sketchup/Animation.html

We made YARD comments that documented the interface. (YARD created a new page per class/module)

Similarly the tool interface, entirely "virtual". We can follow the same pattern.

DanRathbun commented 4 years ago

YARD understands the idea of abstract classes and has an @abstract tag.

DanRathbun commented 1 year ago

This is my first edition of a YARD stub for LoadHandler:

module Sketchup

  # @abstract **This class is abstract**. The `Sketchup::LoadHandler` class cannot
  # be sub-classed because it is not defined by the API.
  #
  # An abstract class to watch over the download and definition import of a model
  # file using the {DefinitionList#load_from_url} method. The callbacks in such an
  # instance of a load handler are normally used to drive an interface to keep the
  # user informed and respond upon error or success.
  #
  # To handle the remote download and import of a `.skp` model file as a component
  # definition into the active model, create a custom class of whatever class name
  # is desired within your own namespace and extension submodule. Within the class,
  # implement the callback methods you need (as described below.)
  #
  # As this is an abstract class, you are free to define any instance variables,
  # constants, class methods or other internal instance methods you desire to make
  # a well working class with maintainable code. Ie, it might be beneficial to
  # pass references or data into the load handler object that could be used within
  # the callbacks.
  # @example Passing arguments into a load handler:
  #   class MyLoadHandler
  #
  #     # Pass the outer object that calls {DefinitionList#load_rom_url} and the
  #     # active model object references into the new load handler instance.
  #     def initialize(parent, model)
  #       # Assign the arguments to internal instance variables:
  #       @parent = parent
  #       @model = model
  #       # Initialize other variables:
  #       @start_time = Time.now
  #     end
  #
  #     # Upon success, ask the parent (that instantiated this handler,) for the
  #     # newly loaded definition object and start a repetitive instance insert.
  #     def onSuccess
  #       # This will start a new undo operation!
  #       @model.place_component(@parent.definition, true)
  #     end
  #
  #   end # class
  #
  # Code defensively. It is suggested that code keep track of the time spent on
  # the download. Some servers do not return the file size in the respose headers
  # before the download begins. This could mean {#onPercentChange} does not help
  # the handler know the progress of the download. Perhaps do not rely upon the
  # {#onFailure} callback to fire on a timeout condition.
  #
  # @see DefinitionList#load_from_url
  #
  class LoadHandler

    # @!method cancelled?
    #   Checked by SketchUp frequently to see if the handler object wants
    #   to cancel the download or continue.
    #   You could, for example, show a messagebox after X seconds asking if the
    #   user wants to cancel the download. If this method returns `true`, then
    #   the download cancels. Return `false` to continue downloading.
    #   @example
    #     def cancelled?
    #       Time.now - @start_time > 60.0 ? true : false
    #     end
    #   @return [Boolean] Whether to cancel the download or not.

    # @!method onFailure(error_message)
    #   Callback that fires when a download error occurs.
    #   @example
    #     def onFailure(error_message)
    #       Sketchup::set_status_text('')
    #       UI.messagebox(error_message)
    #     end
    #   @param [String] error_message The message text.
    #   @return [nil] SketchUp does not expect any particular return value.

    # @!method onPercentChange(percent)
    #   Callback that receives percent change updates from SketchUp. If the web
    #   server does not report the file size in response to the request, then
    #   the `precent` value may be `0` each time this method is called.
    #   @example
    #     def onPercentChange(percent)
    #       # Display the download progress on the status bar:
    #       Sketchup::set_status_text("loading: #{percent.round}%")
    #     end
    #   @param [Float] percent
    #   @return [nil] SketchUp does not expect any particular return value.

    # @!method onSuccess
    #   Callback that is fired upon completion of a successful download. This
    #   might be where the downloaded definition is used, or a method call is
    #   made to another object that notifies it that the new definition is now
    #   usable.
    #   @example
    #     # Call another class to notify that the download is complete.
    #     def onSuccess
    #       Sketchup::set_status_text('')
    #       Downloader.complete
    #     end
    #   @return [nil] SketchUp does not expect any particular return value.

  end # class

end # module Sketchup
MSP-Greg commented 1 year ago

Maybe

    # @!attribute [r] cancelled?

instead of:

    # @!method cancelled?

Always had a thing about declaring attributes,..

DanRathbun commented 1 year ago

No. It is not an attribute (instance variable.) LoadHandler is a native API interface and cancelled? is an optional callback.

This stub is only a means of documenting the interface as a abstract class on a YARD class page of it's own.

The SketchUp Extensibility Team would actually put the documentation into the C code comments so it might have a bit different form (RDoc I think.) The team uses a custom YARD template to read the C comments.

This Ruby stub would be for use by those who use the Stubs repository to generate their own YARD documentation.

MSP-Greg commented 1 year ago

No. It is not an attribute (instance variable.)

Whether an instance variable exists is an implementation issue. I would consider it an attribute predicate method, and doc it that way. Doc's aren't concerned with implementation, they're about the API.

I can't recall if YARD will recognize an @!attribute tag in c source...

DanRathbun commented 1 year ago

Greg, ... it is just NOT necessary as cancelled? is a callback method in which the coder can do anything they like as long as they know that the return value (truthy or falsey) will inform the SketchUp engine to either continue the download or cancel it respectively.

Documenting it this way lets the consumer know that they need to define a callback method if they wish to have a "bailout" feature in their LoadHandler interface.


Also, if I remember correctly, YARD will list attributes separately from instance methods.

But, for YOUR own Ruby stubs do whatever YOU like.

MSP-Greg commented 1 year ago

Dan,

From some YARD code, showing methods being tagged as attributes:

      # @!attribute [r] object
      # @return [CodeObjects::Base, nil] the object the parent docstring is
      #   attached to. May be nil.
      def object; parser.object end

      # @!attribute [r] handler
      # @return [Handlers::Base, nil] the handler object the docstring parser
      #   might be attached to. May be nil. Only available when parsing
      #   through {Parser::SourceParser}.
      def handler; parser.handler end

I've seen 'callback' often refer to methods called when an event occurs. Some of the code in question is more about implementing an interface, which isn't handled In Ruby as in some other languages.

Also, if I remember correctly, YARD will list attributes separately from instance methods.

Correct, which is the basis for my suggestion. But, as I also mentioned, I'm not sure if that can be done with c source.

DanRathbun commented 1 year ago

I've seen 'callback' often refer to methods called when an event occurs.

AFAIK ... the feature registers a temporary interface object that receives calls "back from the SketchUp engine's" load remote definition task, in order to control it and provide feedback so the extension can in turn give feedback to the end user.

It really sounds like "callback" is appropriate here.

What should they be called then if we and/or the API is using the incorrect term?

@DanRathbun Also, if I remember correctly, YARD will list attributes separately from instance methods.

Correct, which is the basis for my suggestion.

Right, so then this is not the correct thing to do. Attributes are getters or setters (or accessors) for state.

The cancelled? callback method is not a state getter method. Listing it this way will confuse and lead readers astray.

You've proven (by your comments) that it also was not named very well as it's name implies that it should be a state getter. The early API coders (no longer with the company) were very good at JavaScript but not very familiar with Ruby conventions.

The callback method name should really have been named cancel_download? (perhaps not with the question mark.) It really is SketchUp asking the interface the question of whether to cancel or continue (the download) in the present tense, not past tense.

The documentation also does not say how often or when the callback is called and whether it occurs before or after it calls the onPercentChange callback / event handler.


Anyway ... it's fine the way it is.

(I'm not sure that SketchUp's custom YARD template uses the attribute listing anywhere. It is not likely they'd introduce it now if it was never used in the past. The docs have enough open errors and improvements needed. We don't need to worry about this.)