SketchUp / api-issue-tracker

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

Can't freeze Sketchup::Color #826

Open linying1991 opened 2 years ago

linying1991 commented 2 years ago

Bug Reports

  1. SketchUp/LayOut Version: 2022
  2. OS Platform: Windows 11

Sketchup::Color can't freeze

The correct result should be to throw an exception 1

Geom::Point3d can freeze

1

thomthom commented 2 years ago

hm.. I don't think anything in the SU API checks the frozen flag...

DanRathbun commented 2 years ago

I do not agree that this is a bug - It is an implementation oversight (like how clone does not work for many API objects.)

The main difference here is that Geom classes are virtual geometric helper classes that do not exist in the model database.

A color object is part of a material object held in the model's collection data. Having it frozen (freezable) makes no good sense.

What happens when a user manually edits a material in the GUI using the frozen color object? Should a dialog pop up saying "Sorry this color is frozen! Move along to something else ..." ? (No of course not.)

Objects held by the model and editable by the GUI, should have their #freeze, #taint and #untrust methods undefined.

Entity and it's subclass Drawingelement

(I think we talked about this back in the v7 or v8 days on SketchUcation.)

class Entity

  undef_method(:freeze)

  undef_method(:taint)

  # Scheduled to be removed in Ruby 3.2:
  undef_method(:untrust) if Object.public_methods.include?(:untrust)

end

Collection and some other classes that are direct Object subclasses should also not be freezable:

[
  # None of these classes should be freezable, etc.:
  Sketchup::Axes,
  Sketchup::Classifications,
  Sketchup::Color,
  Sketchup::Console,
  Sketchup::Entities,
  Sketchup::EntitiesBuilder,
  Sketchup::ExtensionsManager,
  Sketchup::ImageRep,
  Sketchup::InputPoint,
  Sketchup::InstancePath,
  Sketchup::Menu,
  Sketchup::Model,
  Sketchup::PickHelper,
  Sketchup::Selection,
  Sketchup::Set,
  Sketchup::TextureWriter,
  Sketchup::Tools,
  Sketchup::UVHelper,
  Sketchup::View
  #
].each do |klass|
  klass.class_eval {
    undef_method(:freeze)
    undef_method(:taint)
    # Scheduled to be removed in Ruby 3.2:
    undef_method(:untrust) if ::Object.public_methods.include?(:untrust)
  }
end

UI classes

It may be that coders may wish to have their Command, Toolbar, etc. objects frozen to prevent nefarious manipulation. But this might also prevent the status bar text property from being updated.

Freezing a HtmlDialog might prevent the callbacks from being reattached if it is reopened, so freezing a dialog is likely not desirable in most cases.


After undefining attempts to freeze something looks like this:

Sketchup.active_model.materials.freeze
#=> Error: #<NoMethodError: undefined method `freeze' for #<Sketchup::Materials:0x0000029699df30b8>>

~

thomthom commented 2 years ago

What happens when a user manually edits a material in the GUI using the frozen color object?

The SketchUp API returns a copy of the color object. All Sketchup::Color objects are standalone, owned by the Ruby object itself. Color, Point3d/2d, Vector3d/2d - they are all plain data objects that doesn't share ownership.

For Entity objects et. al. you're right, freezing them would not work well.

DanRathbun commented 2 years ago

All Sketchup::Color objects are standalone, owned by the Ruby object itself.

Do you mean they are just transient data objects as well .. ie, Ruby wrapped up C++ data separate from the C++ Material objects ?

thomthom commented 2 years ago

Yes. Whenever you get a point/vector or color you are getting a copy of the value. Thus, if you access the same vertex.position a lot, caching might yield performance boost.

DanRathbun commented 2 years ago

Okay thanks. I still do not see much good reason to freeze an extension's colors, points or vectors as the extension is in control of it's own objects. It is not likely that any other extension is going to try to modify another extension's objects.

thomthom commented 2 years ago

I find it useful to freeze objects used for constants, because it's easy inadvertently pass that object along in your code flow and accidentally mutate it. Freezing it ensures you get an error instead of silently carrying on.

MSP-Greg commented 2 years ago

I find it useful to freeze objects used for constants

There are many reasons to freeze objects. I've seen frozen objects that are passed to yield blocks, where the object is passed for its data, but it shouldn't be modified. Obviously, there are many ways to do things in Ruby...

Ideally, the API should freeze them or raise an error if they can't be.

sketchup[bot] commented 2 years ago

Logged as: SKEXT-3504