Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Elements to support adding elements with different laydats #29

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tom-varga
Copy link
Contributor

This one may be controversial, but I hope not.
I will be committing a Cell.find() method that is a general purpose mechanism to find elements in a cell that match various requirements such as laydat and various boolean comparisons with a specified set of elements.
An example might be to search for all elements that are contained within a specified region and add them to an Elements object.
By definition, this could be a list of Elements on different laydats.
Therefore, it is bad that the current Elements object immediately converts all elements to the same laydat.
This update prevents this from occurring resulting in an enhancement that Elements now supports list of elements on different laydats.
However, the layer,datatype, and laydat setters continue to operate on all elements.

@hohlraum
Copy link
Owner

I'll have to think about this. I see why you want to do this: find() will return many objects, and you need a container to hold them. Allow me to think aloud. What are the options:

  1. A list.
    Pros: easy, lightweight
    Cons: lists are dumb, no access to useful methods like show(), bounding_box(), etc...

  2. Elements.
    Pros: lightweight, ostensibly this is what Elements are meant to be: a "smart" list, except...
    Cons: by design, elements are meant to share common datatype/layer

  3. Cell.
    Pros: no outstanding expectation that elements are of the same laydat.
    Cons: allowing references is more complication than needed here.

  4. A new class.
    Pros: could be tailored to do exactly what we want.
    Cons: seems redundant.

On the balance I think your solution is the best one.

However, we need a way of handling the getters for layer, datatype and laydat in cases where there are a mixture of layers, datatypes or laydats within the Elements. For instance, if there are several different layers in the list is it better to return a) a single value (which may not be among those in the list); b) multiple values, one for each of the layers actually present, or c) None which stands as a warning that there is an internal mixture of layers. It seems that one way or another this is going to break the correspondence between the interface for Elements and Path/Boundary. Can you see a way to reconcile this?

BTW, I've come to dislike the class name Elements, mainly because it hard to distinguish between the class itself and the drawing objects its intended to contain. Now might be a good time to start transitioning to a better name.

@hohlraum hohlraum closed this Mar 11, 2015
@hohlraum hohlraum reopened this Mar 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants