SketchUp / api-issue-tracker

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

Set scene visibility for bulk layers #380

Open DanRathbun opened 4 years ago

DanRathbun commented 4 years ago

SketchUp API Feature Request

Bulk layer visibility setters for Sketchup::Page scene objects.

@thomthom said (in Issue #376: Set scene visibility for entity): _I would have thought there should be hidden_entities= and layers=_

Bulk setters certainly would be welcomed by coders.

I'd say that hidden_layers for the getter is more descriptive. Perhaps the former could be implemented as an alias ... and layers deprecated?

@thomthom, I need to remind you that "=" named setters are not normally used in Ruby when the argument is not a single object. Please remember how the Sketchup::View#camera= method always confuses coders when they wish to use a transition. They always make a mistake and try to pass 2 parameters instead of wrapping them inside 1 array.

So, when the argument(s) will be an array or list of variable size, ... then a non "=" method beginning with "set_" is probably more appropriate. (An exception to the rule would be when the argument is a fixed size array. For example a setter that expects a point but can also take an array of 3 numerics.)

I also would think that any layers not specified should be left unchanged. Ie, it'd modify the hidden layer set, but not overwrite it. Unless anyone can think of a good reason to allow an initial boolean argument that decides whether or not to overwrite the entire set?

The alternative would be to have 2 methods set_hidden_layers (overwrites and resets the set of a page's hidden layers to only the argument list,) ... and add_hidden_layers (modifies the current set of a page's hidden layers.

Test ...

module PageRefinement

  refine Sketchup::Page do

    alias_method(:hidden_layers,:layers)  

    def add_hidden_layers(*ary)
      bulk_set_layers(ary.flatten,false)
    end

    def clear_hidden_layers()
      self.hidden_layers.each { |layer|
        self.set_visibility( layer, true )
      }
    end

    def remove_hidden_layers(*ary)
      bulk_set_layers(ary.flatten,true)
    end

    def set_hidden_layers(*ary)
      clear_hidden_layers()
      bulk_set_layers(ary.flatten,false)
    end

    private

    def bulk_set_layers(ary,flag)
      ary.each { |layer|
        case layer
        when Sketchup::Layer
          self.set_visibility( layer, flag ) if self.parent.include?(layer)
        when String
          self.set_visibility( self.parent[layer], flag ) if self.parent[layer]
        end
      }
    end

  end

end
Eneroth3 commented 4 years ago

I'd say that hidden_layers for the getter is more descriptive.

I have a vague memory the implementation is a bit more complicated here. If I'm not mistaken there is a list of layers and an additional boolean value telling whether these are specified to be shown or to be hidden. I can't remember where I have seen this though and it could be a misunderstanding.

Found it! Each Layer has a property saying what default state it has for scenes, and scenes have a list of layers that differ from their default setting. Quite convoluted.

https://ruby.sketchup.com/Sketchup/Layer.html#page_behavior-instance_method

DanRathbun commented 4 years ago

Okay, the docs say "hidden layers" ... what is a better adjective to use ?

Eneroth3 commented 4 years ago

"Rebel layers"? Nah. I see why they wrapped this logic and made a custom setter though.

DanRathbun commented 4 years ago

Hmmm ... an adjective that indicates states are stored ...

Eneroth3 commented 4 years ago

I think it is best described as a full sentence. I love expressing things with just that right word, and it is great to do that for naming variables, labels, titles etc, but here it is really just a weird behavior that makes sense to wrap in more pedagogical interface.

DanRathbun commented 4 years ago

Thinking out loud ...

Eneroth3 commented 4 years ago

Are we talking about names for the existing method? The convention for the SketchUp Ruby API seems to be not to correct the API when it comes to naming and inconsistencies but to accept and live with it. Renaming a method but keeping a deprecated alias for some time is otherwise quite common, but given how many old and no longer maintained extensions people still use, I don't find that path very likely here.

DanRathbun commented 4 years ago

Alias is what I'm talking. (There are quite a few methods in the API that have aliases.)

I believe things can be fixed for the better.