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

Use a Travertino-based interface for style operations. #311

Merged
merged 69 commits into from Dec 23, 2017

Conversation

freakboy3742
Copy link
Member

This PR removes the dependency on Colosseum, in favour of Travertino. Travertino contains only the primitive datatypes and operations for styles, which allows Toga to include a built-in simple style engine (called, for want of a better name, Flow).

Colosseum will remain a fully CSS compliant style engine, and will extend the interfaces and types defined by Travertino. Travertino and Colosseum will be "Toga independent", dealing purely with laying out boxes on a page. The process of applying that layout to an actual widget is the role of a new class called an "Applicator".

The "Flow" engine reduces layout to "boxes of boxes", where every box is a row of other boxes, or column of other boxes. Each box can have an intrinsic width and height (either "exactly X", or "at least X"), and may optionally specify an exact width or height as a style. Boxes can also specify a "flex" amount; all the flexes in a row/column are added, and after minimum space allocations have been met, any extra is allocated in proportion to the flex amounts. If a row/column requires more space than is available/allocated, the box will be expanded to the minimum possible size.

This patch makes one other notable change: Every widget is now expected to have a native object. This fixes the unusual handling of Box objects, which were widgets that didn't have a native object. As well as simplifying code, this has the added benefit of ensuring that methods like "set background color" will always have a widget that is appropriately scoped. The name container has been recycled to mean the "top level widget" (which is what it was really being used for anyway).

@freakboy3742
Copy link
Member Author

This is still a WIP; at present, tutorial 0, 1 and 2 work on Cocoa. There's also a big need for documentation, especially for the new Flow style engine.

@freakboy3742 freakboy3742 changed the title WIP: Use a Travertino-based interface for style operations. [WIP] Use a Travertino-based interface for style operations. Dec 13, 2017
@goanpeca
Copy link

goanpeca commented Dec 13, 2017

Colosseum will remain a fully CSS compliant style engine, and will extend the interfaces and types defined by Travertino. Travertino and Colosseum will be "Toga independent", dealing purely with laying out boxes on a page. The process of applying that layout to an actual widget is the role of a new class called an "Applicator".

Applicator :-p ? really?... just trolling, this awesome work :0)

@freakboy3742
Copy link
Member Author

@goanpeca I'm open to suggestions for better names... :-)

@goanpeca
Copy link

goanpeca commented Dec 13, 2017

LayoutConstructor, LayoutCreator, LayoutManager, LayoutHandler, LayoutBuilder, LayourApplicator XD... having the work Layout in it would really clarify the meaning from the start, don't you think?

@freakboy3742
Copy link
Member Author

Ok - except that it's not just about layout (at least, it won't be very soon). It also handles colors and fonts. It's about applying the style to the the objects in the layout. I dug through the Gang of Four "Patterns" book for a good name match, and didn't really find one; ended up using "makeup applicator" as the underlying idea.

@goanpeca
Copy link

Ok

Copy link
Member

@danyeaw danyeaw left a comment

Choose a reason for hiding this comment

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

This is looking really great! I like the change in name from Flow to Pack. I would say I made it through about 2/3 of the changes, I'll try to review the changes to the tests and some of the other platforms tomorrow.

It seems like there is an issue with the rehint / size allocation. Tutorial 4 doesn't display unless I resize the window.

One other minor thing: It doesn't look like the Cookcutter at examples/.template has been updated with the changes.


# Turn the autoresizing mask on the container widget
# into constraints. This makes the container fill the
# Turn the autoresizing mask on the widget widget
Copy link
Member

Choose a reason for hiding this comment

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

Is this supposed to say widget widget?

Copy link
Member Author

Choose a reason for hiding this comment

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

Widget? Widget! 😝


.. literalinclude:: /tutorial/source/tutorial-0.py
.. literalinclude:: /../examples/tutorial0/tutorial/app.py
Copy link
Member

Choose a reason for hiding this comment

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

Great change to get rid of duplicating the tutorial code!

@@ -12,7 +14,7 @@ def attr_impl(value, attr, factory):
# This will manifest any impl-specific attributes.
impl = getattr(value, attr, None)
try:
Copy link
Member

Choose a reason for hiding this comment

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

Renaming to bind really improved clarity here

@@ -9,7 +9,8 @@ def build(app):
box = toga.Box()

button = toga.Button('Hello world', on_press=button_handler)
Copy link
Member

Choose a reason for hiding this comment

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

I'm getting Gtk+ Warnings when running tutorial0: Allocating size to GtkButton 0x2e64180 without calling gtk_widget_get_preferred_width/height() and Negative content width -33 (allocation 1, extents 17x17) while allocating gadget (node button, owner GtkButton).

Copy link
Member Author

Choose a reason for hiding this comment

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

Hrm. That's odd - I'm not seeing this on my ubuntu 16.04 VM.

Can you confirm you've got travertino 0.1.2? I think I remember seeing something like this with 0.1.1...

Failing that - what OS are you running?

@DariusMontez Can you confirm if you're seeing this error?

Copy link
Member

Choose a reason for hiding this comment

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

@freakboy3742 I'm running Ubuntu 17.10. I cloned Travertino and then installed it with pip3 install -e.

@@ -1,4 +1,5 @@
import toga
from toga.style.pack import *


def build(app):
Copy link
Member

Choose a reason for hiding this comment

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

Tutorials 1/2/3 are also getting Gtk+ warnings: Gtk+ Allocating size to GtkButton 0x2d40180 without calling gtk_widget_get_preferred_width/height().

else:
# If no width is specified, assume we're going to use all
# the available width. If there is an intrinsic width,
# use it to make user the width is at lea
Copy link
Member

Choose a reason for hiding this comment

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

Is part of the comment missing here?

Copy link
Member Author

Choose a reason for hiding this comment

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

What makes you sa

Copy link
Member Author

Choose a reason for hiding this comment

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

:-)


# Establish available height
if self.height:
# If width is specified, use it.
Copy link
Member

Choose a reason for hiding this comment

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

Did you mean if height is specified?


@property
def on_press(self):
""" The callable function for when the button is pressed
"""The handler to invoke when the button
Copy link
Member

Choose a reason for hiding this comment

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

Did you mean to delete "is pressed"?

@@ -4,35 +4,34 @@


class Button(Widget):
""" Button widget, a clickable button.
"""A clickable button widget.

Args:
label (str): Text to be shown on the button.
id (str): An identifier for this widget.
style (:class:`colosseum.CSSNode`): An optional style object. If no style is provided then
Copy link
Member

Choose a reason for hiding this comment

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

Should this be style (:obj:Style)? Please check for the other widgets as well.

@freakboy3742 freakboy3742 changed the title [WIP] Use a Travertino-based interface for style operations. Use a Travertino-based interface for style operations. Dec 22, 2017
@freakboy3742
Copy link
Member Author

At this point, I think this patch is feature complete. Barring bugs and design criticisms, I'm ready to merge it.

@@ -39,6 +39,7 @@ def set_enabled(self, value):
### APPLICATOR

def set_bounds(self, x, y, width, height):
# print("SET BOUNDS ON", self.interface, x, y, width, height)
Copy link

@goanpeca goanpeca Dec 22, 2017

Choose a reason for hiding this comment

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

@freakboy3742 would not it be better to start using the logging module for this, e.g.

log.debug(("SET BOUNDS ON", self.interface, x, y, width, height))

?

Copy link
Member Author

Choose a reason for hiding this comment

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

I get what you're driving at; however, I've got three reasons for not using logging.

  1. This is a set of messages that we really only care about during this initial debugging period for Pack. Once we've established that Pack is doing the right thing, we're unlikely to need to have debugging any more.

  2. This is a code path that needs to be fast. Python function calls are one of the slower operations the VM can perform, so even if a function is a no-op, it can have a perceivable time lag.

  3. I have a bit of a love-hate relationship with Log, mostly because it's really easy to use the wrong way. If it's used properly, it's great - but if it isn't... well, you spend your life trying to overcome the fact that some third party package is trying to wrest control of the logging configuration.

Signed-off-by: Dan Yeaw <dan@yeaw.me>
@danyeaw
Copy link
Member

danyeaw commented Dec 23, 2017

Attached are the changes made to fix the size_allocation problem, I also submitted a PR to your branch if that is easier 👍

Fix_size_allocation_errors_for_Gtk+.txt

I totally agree that this PR is ready to be merged.

Maybe for a future PR, how do you think the canvas widget should handle colors? Should we still allow a color passed in directly through fill_style and stroke_style? The style is applied to the widget, so these are more like per-method styles.

Fix size allocation errors for Gtk+
@freakboy3742
Copy link
Member Author

@danyeaw I'm not 100% sure what we should do with colors/styles on canvas. There's a certain simplicity to being able to just "draw a blue line"; but I can also see how being able to apply a "stylesheet" to a canvas and have all lines with a given ID or class be blue could also be useful. Definitely a topic for another PR discussion, though :-)

@freakboy3742 freakboy3742 merged commit dcf2db3 into beeware:master Dec 23, 2017
@freakboy3742 freakboy3742 deleted the travertino branch December 23, 2017 03:47
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.

None yet

4 participants