Skip to content

Conversation

@jwiggins
Copy link
Member

This PR implements constraints-based layout of components in Enable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not 100% sure, but these feel like they maybe should live on CoordinateBox (or a subclass of CoordinateBox) which Component inherits.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted. Perhaps now is a good time to remove all the layout cruft from Component? CoordinateBox is a better place than Interactor...

@corranwebster
Copy link
Contributor

Looking this over, this is an excellent addition, and important. The things that I wonder about:

  • would it make sense to make all containers ConstraintsContainers, but have default behaviour that mimics the current fixed layout system (ie. strong equality to width and height)? In other words, should we make constraints layout the default behaviour?
  • how much of this code is duplicated from Enaml? Should duplicated code be pushed to Casuarius?
  • layout helpers! Waaaannt!
  • there is a stubbed out start of a composable layout system (see AbstractLayoutController, some unused traits on Container, etc.) that @pzwang started some years back. We should either think of fixing it up and using it, or just ripping it out and deleting it as part of this PR.

We want to get this right, as if we do it will have important consequences for Enable and Chaco.

@jwiggins
Copy link
Member Author

Responses to each of your points:

  • If we can come up with a good way of integrating this without breaking existing code too badly then it probably should be the default layout. I'm not sentimental.
  • LayoutManager is copied verbatim. LayoutBox is a slightly modified version of some code which is duplicated in the Enaml client code (one copy each for Wx and Qt). Since this copy makes three, it's time to stuff it somewhere convenient. The symbolic constraints that I added to LayoutBox will need to be moved to a more appropriate place.
  • I agree that layout helpers should be included. They weren't an early priority. Now that it works, they just got more important. There might be a problem sharing this code with Enaml since Enable has a bottom-left origin and Enaml is top-left.
  • Old, unfinished stuff should go... new_component.py and new_abstract_component.py have survived for far too long (unfinished feature work in the master branch? WAT?). I think the success of Enaml has shown us the best way to do layout so far is constraints. It's certainly better than anything we've had up until now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is a potential solution to the string problem here to have layout_box override __getattr__ for 'left', 'right', etc. to return layout_box.primitive('left'), etc. so we can say:

container.layout_box.left == one.layout_box.left

Not perfect, but it could be refined further, and would allow layout helpers to be added more easily.

- Implement contents constraints in the container
- Expand layout helpers in the user-specified constraints
This should help with the layout helper debugging...
This means that layout helpers can accept components directly instead of a
reference to the constraints namespace of the component.
We can revisit this once Components have a more robust way of supplying
their size preferences.
@jwiggins
Copy link
Member Author

jwiggins commented Mar 7, 2013

I feel like this is ready for a full code review now. It does all the things I think it should do for a first cut. I've created several layouts with it in a reasonably complex customer project and it seems to be holding up well.

@jwiggins
Copy link
Member Author

jwiggins commented Mar 8, 2013

I'm going to close this request and reopen with the target branch as "feature/layout-refactor" so that we can accumulate all these disruptive changes over time.

@jwiggins jwiggins closed this Mar 8, 2013
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