Convert Canvas Widget to Use a Drawing Stack

[danyeaw]

In #319 we discussed that it was desired to remove the callbacks created by using an on_draw handler, and instead push drawing commands to a stack to be drawn with.

PR Checklist:

• All new features have been tested
• All new features have been documented
• I have read the CONTRIBUTING.md file
• I will abide by the code of conduct

Current status: fully implemented

• Fix tests
• Further testing with other contexts besides the default one

[freakboy3742]

This looks like a good start in the general direction of what I was suggesting in #319; however, I'm not sure it's 100% there yet.

There are two particular aspects that concern me.

Firstly, the remove=True approach to removing drawing elements. Does that actually work? Does a lambda of a bound method have a reliable hash that allows for removal from a list? Regardless, it's not exactly a "natural" API - I would have expected canvas.remove(instruction), not canvas.draw_somthing(details, remove=True).

Secondly - once a drawing instruction has been added - is there any scope for modifying it's parameters? For example, consider a vector drawing app. I add a circle to my canvas, and then I want to drag to modify it's radius. Using this API, I'd need to remove then add each drawing instruction as I went, whereas all I really want is the ability to modify the radius of the circle instruction that is already there.

So - here's a proposal: Have an add() and remove() method on the interface layer. Those methods add and remove "drawing instruction objects". A drawing instruction object might look something like:

class Circle:
    def __init__(self, x, y, radius):
        self.x = x
        self.y = y
        self.radius = radius
    def __call__(self, impl):
        impl.arc(self.x, self.y, self.radius, 0, 0)

The interface level calls devolve into constructing an instruction object instance, adding it to the interface's list, and then returning that object to the user.

The user can modify the parameters of the instruction object - resulting in a changed visual appearance on next redraw. (There might be a need for an API endpoint to force a redraw after you've tweaked parameters)

Or, the user can use the instance returned as the instance that has to be removed from the instruction list. Or, if they won't need to modify or remove the drawn object, they can just ignore the returned value.

Drawing Contexts then become a special type of drawing instruction that can themselves hold drawing instructions. I'm sure there's some way we could also mange to make drawing contexts Python-level context objects (since they will be formal Python objects, after all)

Using this approach:

• The simple case of "draw a logo on the screen" is just a set of drawing instructions.

• A vector drawing app is a set of mouse click listeners to create objects, and drag listeners to modify the parameters of the object being displayed.

• A platformer game is a set of keyboard listeners to modify the x/y coordinate of a drawing context that draws each character on the canvas

Does that make sense? Have I missed anything obvious in this design?

[danyeaw]

@freakboy3742 Thanks again for the great feedback. Here I am stuck in my little bubble of what canvas APIs provide now, and you are thinking outside the box - mind blown!

You are spot on with the remove parameter, that won't work 😄

I noticed in your Circle example that you were moving a level of abstraction up, so instead of providing low level drawing operations like an arc, we are providing an interface to draw a circle. To follow those breadcrumbs, maybe we would want to provide more standard vector tool like drawing capabilities like Rectangle, Rounded Rectangle, Ellipse, and a Pen tool. We might be able to provide a few other common drawing elements like arrows for UML or other diagramming.

I think I understand the basic concept of making a character in a game a context and translating it around the screen, I'm not sure what API someone making a game like that would like though. So that sounds like an interesting thing for me to look in to a little more.

[freakboy3742]

The suggestion about "Circle" wasn't really meant as a specific API suggestion - it was meant more as an example of how you might have drawing instructions that abstract underlying drawing operations. Each of the primitives will almost certainly have a drawing instruction associated with it; but there might be other useful drawing instructions that encapsulate one or more underlying primitives, some with fixed values where the underlying primitive allows configuration.

That said - "Circle" is one of those time where we can try to educate everyone and say "a circle is just an arc with a start and end angle of 0"... or we can just provide a "draw circle" API :-) As a first pass, we don't have to capture all those use cases, though - just translating the primitives would be plenty.

Providing more complex objects like "Circle", "Rounded Rectangle" or "Arrow" as a proof of concept for more complex operations might be a nice to have, but could certainly wait until a later iteration.

Application-specific complex shapes like "UML Class" definitely sounds like something that should be left to end-users.

[DariusMontez]

@danyeaw Are you familiar with Tkinter's canvas? Docs link. It came to mind because it also uses a "drawing stack", so it might be interesting to glean from their API.

Tkinter does something quite curious for 2d coords: they use complex numbers! So (10px, 45px) would be represented by 10 + 45j. Complex number math works kinda like vector math, which means that you can just add to coords together: (10 + 45j) + (0 + 5j) == 10 + 50j. I'm not sure if that's the right move for Toga, but it is intriguing nonetheless.

The API has the idea of "canvas item" objects that represent lines, arcs, images, etc.. When you call canvas.create_line(...), a line instance is added to the draw stack, and the function returns its ID so you can modify/delete it later.

Personally, I favor our approach of dealing directly with classes such as Circle and Image instead of Tkinter's approach of passing around IDs to items and using methods to indirectly modify them, and Tkinter's system seems less extensible since you can't define your own subclasses of drawables.

Anyway, I though I'd bring it up as a possible source of inspiration. Great work so far @danyeaw !

[danyeaw]

@DariusMontez Thanks! I'll take a look at those Tkinter docs.

[freakboy3742]

@DariusMontez The "return an ID" approach used by Tkinter is a function of Tk itself - in Tk, everything is a string, so using an ID is pretty much the only way to refer to a complex object. Since we're using Python, there's no need to emulate that particular quirk :-)

I can see the analog between complex numbers and points, and using a builtin "point-ish" data type is definitely appealing; but I'm not sure I'm wild about leaning on that feature too hard. Consider the math of increasing the height of a widget by Y pixels - you'd need to use height + (y * j) if y is entered as an integer, but height + y if it was entered as a complex value. You could use height + abs(y) if it's ambiguous - but the abs() calculation could also accept a point offset, which would then return the wrong value. Or, you could use height + y.imag - but that then relies on you understanding the conceit that is being used. That's a source of potential error for every height calculation that is avoided if you just use a tuple.

[danyeaw]

@freakboy3742 Since for me a picture is worth a 1000 words, is this what you had in mind? I want to make sure the concept is concrete for me before I start hacking at some code. This would move the majority of the drawing methods to drawing object classes. The stack would then be a drawing_objects list on each context in the implementation level. What did you mean above by a Python Context Object? Do you mean a Python context manager (with statement) or something else?

[freakboy3742]

@danyeaw I think we're mostly on the same page.

The calls to implementation layer drawing methods would be on the drawing objects. The interface layer canvas would still have draw_line(...) et al, but it would just be a wrapper for constructing primitives:

    def draw_line(self, x1, y1, x2, y2):
        line = Line(x1, y1, x2, y2)
        self.root_context.add(line)
        return line

A DrawingContext would have the same wrappers - so you can draw on the canvas, or you can draw on a context; the difference being that a DrawingContext would act as a context manager, and any drawing operations on the DrawingContext would operate on the context, rather than the canvas directly.

The DrawingContext would also be a drawing instruction, whose role is to wrap other drawing instructions - that way a context can be put on the root context's list of drawing instructions.

So - usage would be something like this:

my_canvas = toga.Canvas()
my_canvas.draw_line(10, 20, 30, 40)
with my_canvas.context() as dc1:
    dc1.translate(5,15)
    dc1.draw_line(0, 0, 30, 40)
    with dc1.context() as dc2:
        dc2.draw_line(5, 10, 15, 20)

Looking at your use cases:

• A simple drawing tool would just use the shortcut primitives to draw directly on the canvas.

• A platform would construct a context for each character or element in the game, and retain handles to them. Moving the character is then just a matter of adjusting the context.

• A vector tool would construct individual drawing objects, and add them explicitly to the canvas.

Does that make sense?

[danyeaw]

Thanks @freakboy3742 for the feedback! Here is version 2 based on my updated understanding.

[freakboy3742]

@danyeaw Ok.. so... I've never found UML especially helpful as a communication tool (for Python especially) - but that diagram seems much further away from what got in my mind.

The drawing primitive objects are interface level objects - and even if they were at the level of the implementation, they wouldn't be "owned" in the way that diagram implies.

The role of the implementation is to provide a generic API that can map onto the native drawing APIs.

The role of the interface is to provide a mechanism to create and store a list of drawing instructions that, when needed, can be "executed", resulting in a bunch of implementation level drawing API calls (and, in turn, a bunch of native API calls)

Does that help any more? I feel like I'm not being especially effective in communicating what I've got in mind, but I'm not sure where the message is getting lost...

[danyeaw]

@freakboy3742. Thanks for your help getting the concept clear for me, I really appreciate it. I think what we really need is to be able to huddle around a whiteboard 😄

The calls to implementation layer drawing methods would be on the drawing objects.

Sorry, my fault, I think I originally read that and thought you meant that the drawing objects (classes) need to be on the implementation layer. Am I getting warmer?

[freakboy3742]

@danyeaw That looks more like it, yes. The implementation layer doesn't need to have any concept of adding and removing drawing objects - the stack of drawing objects only needs to exist at the Interface layer. There might be some context handling operations on the implementation, but that's purely a GTK implementation detail - creating and destroying GTK drawing contexts (assuming that's the easiest way to render them).

[danyeaw]

@freakboy3742 Thanks! I have updated the diagram. I moved the stack to the interface layer and removed the add and remove drawing objects, you are right those shouldn't be needed on the implementation. I kept a draw_contexts method on the implementation because I think that will be needed for Gtk+, but might not be for other implementations.

Looks like it is time for me to start coding. Thanks again for all of your reviews and help!

[danyeaw]

@freakboy3742 I have finished initial implementation of the new design and the widget is drawing successfully using the updated Tutorial 4 example. To make this ready to merge I still have work to do on updating the tests. Would you mind taking a look?

[danyeaw]

Thanks again for all your comments and time reviewing 👍

After taking another look at the children_contexts list, I determined that holding all the contexts in a tree wasn't required in order to pass the canvas down to the next context. The updated PR now just passes the canvas down when a context is created. I removed the children_contexts list, so thanks for helping me find an area that could be simplified. 🍾

I fixed and updated the majority of your other comments.

1. For optimizing the redraw method, I recommend tackling that as part of Add Canvas Support for Cocoa #319.
2. For moving native_context to the first argument, I'm not sure that is feasible. I am calling the implementation methods from the interface with impl.drawing_object(self, ...), so the impl gets passed in as the self.
3. I think the modify methods are also needed as I explained above. But, please let me know if you think I'm missing something.
4. I am also not sure how to make the contexts draw themselves also explained more above. This is a limitation of needing to call the drawing objects from the implementation layer. If you think this is possible, would you mind please explaining more about your concept?

[freakboy3742]

1. On reflection, the redraw has a natural fix - the enter and exit methods of the python context manager provide a point at which we can suspend and resume updates. It shouldn't be that difficult to achieve.
2. When I said "first argument", I meant the first one after self :-)
3. I've left a comment about modify above
4. The interface contexts don't need to draw themselves; the contexts need to know how to draw on a GTK context when provided one.

• The impl layer widget has a member method:

def gtk_draw_callback(self, gtk_context):
    self.interface.root(self, draw_context=gtk_context)

• This callback method is registered as self.native.connect("draw", self.gtk_draw_callback)
• The "root" object on the interface widget is a drawing object object only job is to be a container for other drawing objects. It might also be the only drawing object that exposes the canvas-level scale/skew options. It has a __call__ method that is essentially:

def __call__(self, impl, draw_context):
    for obj in self.drawing_objects:
        obj(impl, draw_context)

• A "simple" drawing object (something like line_to) has a __call__ method that is something like:

def __call__(self, impl, draw_context):
    impl.line_to(draw_context, self.x, self.y)    

• A more complex drawing object (something like a path) is a combination of the two styles, wrapping the "contained" drawing objects with the open/close pair of calls on the implementation that the python context represents.

def __call__(self, impl, draw_context):
    impl.start_path(draw_context, self.x, self.y) 
    for obj in self.drawing_objects:
        obj(impl, **kwargs)
    impl.end_path(draw_context)    

(the exact calls in this last example aren't quite right for Path, but hopefully it's sufficiently clear what I'm aiming at)

This also possibly negates my argument about the ordering of the context argument in draw operations, because it might make sense to pass around **kwargs at the interface level to decouple the specific drawing context arguments that a backend will require.

Does that make any more sense?

[freakboy3742]

Is this a stray search and replace? It doesn't seem right...

[danyeaw]

Yes, I'm not sure how I did that, thanks for catching my error!

[danyeaw]

@freakboy3742 I think I understand what you described, thanks for the examples!

1. I changed the redraw events so that they only happen when exiting a context or when drawing directly to the canvas. This reduced redraw events for the Tiberius example from 55 to 12.
2. I did not reorder the contexts arguments based on you saying your argument was negated by the more complex drawing object above.
3. I removed all the modify methods, all changes are now done directly to the object properties.
4. I implemented the design based on your examples above. One thing I did change is that I realized after I implemented the root drawing object, that I could directly call the Canvas instead. This simplified the design further, while still maintaining clarity.

Thanks again for all of your great feedback.

[freakboy3742]

This is looking really good. I've flagged a couple of smallish things inline.

There are three "structural" things left that I can see.

The first issue is around the "transformation" operations. As I understand it, these operations can really only be applied at the "top" level of the canvas - it's a call directly on the GTK drawing context, affecting how the canvas as a whole is drawn. However, as implemented, you could be N levels deep in context managers, and apply a "scale". If I did that I'd probably expect it to apply across just the "local" context, but AIUI, it would apply globally; and if I had multiple scales, the last one added would be the winner.

Have I understood this correctly? If so, would it make sense to move the transformation operations away from the ContextMixin, and onto the actual Canvas itself? That way, it can only be applied to the actual canvas, not at arbitrary levels down a context chain.

The second issue is the one that would become apparent from the "platformer" use case - the example where moving a "parent" causes children to move as well. AFAICT, all the drawing objects are drawing in absolute canvas coordiates. If I draw Tiberius as a character, and I wanted to move him 20 pixels to the right, I'd need to update every x coordinate for every object in his drawing chain. That's a lot more complex than a single "move Tiberius" call.

The implementation you've got here is most of the way to supporting this - all that is missing is the concept of a drawing origin being propegated down the drawing heirarchy. Two things are needed: a Python context object (Object? SubCanvas?) that stores an "offset from origin" and a list of child objects; and to pass the "origin" down the drawing chain, so that any x/y drawing instruction is adjusted relative to the origin (rather than the "implied (0,0) origin" that is currently being used). Then, the "Object/Subcanvas" adds it's origin offset to the origin it's been drawn with, so all the children are relative to it's own offset.

Does that make sense? If it doesn't, don't worry about it too much; I'm fairly certain it can be added in later - it will just be a change to the drawing method (which isn't public API anyway).

Lastly, I've got a minor concern about the public API surface of canvas. The "draw" operation is being performed by __call__ - which is neat; but it also means that the main canvas object is "callable" - but that isn't (or shouldn't be) part of the public API surface.

Previously, this was less of a concern, because the Canvas "had a" root drawing object rather than "was a" drawing object. It was still a minor issue, however, because the interface drawing objects all appeared to be callable, even though that was something no user should ever do.

My thought is whether it might be desirable to replace the use of __call__() with a _draw() method. That way, the method is explicitly flagged as an internal detail, not for public invocation. You could even argue it's marginally clearer what it is you're actually doing with each drawing object - while it's cute that drawing objects are drawn by invoking them, it's a little opaque.

[freakboy3742]

I'm still unclear why this is a closure/inner function, rather than just a method on the impl canvas.

[danyeaw]

I thought that the callback function could only have two arguments (canvas and context), so I put a closure around it to inject the instance of the class. I did more testing and it does work with a third argument for the class instance. I updated the code to remove the closure. 👍

[freakboy3742]

This could be simplified to just use the property directly (i.e., self.color = color)

[danyeaw]

Updated!

[freakboy3742]

Would it make sense to subclass Context directly? The behavior of __call__, and everything in __init__ about setting up the drawing objects, is exactly the same - you just have to add the actual impl widget underneath it all.

[danyeaw]

Yes, great idea, that helped simplify the class hierarchy by removing the Mixin class.

[freakboy3742]

ping this one in case it's been missed.

[freakboy3742]

Why add_canvas_to_child, and not @canvas.setter?

[danyeaw]

Thanks, that is much nicer. Fixed.

[danyeaw]

Thanks again for all your great feedback!

1. I moved the transformation methods to the Canvas class.
2. I understand your concept of propagating an origin offset, but if you don't mind I would prefer to open up a separate PR for this once I detail out how I want the API to look.
3. I replaced the __call__ methods with new draw methods.

[freakboy3742]

By jove, I think we've done it! Two really small fixes, and I think this is good to be merged. Accepting on the basis that I trust you to make those last two changes before hitting the magic button!

As for the origin/offset thing - I agree that can be a followup PR.

[freakboy3742]

Is this double up needed? The path for the setter doesn't have any dependency on a pre-existing value of _color AFAICT.

[danyeaw]

I was trying to avoid my linter giving instance variables defined outside __init__ warnings, but I agree in this case it is a little wonky because we want to use the setter on initialization of the instance. I went ahead and removed the extra line.

[freakboy3742]

Should this be _draw, rather than draw? It isn't a public API that you should be calling.

[danyeaw]

@freakboy3742 I am trying to understand best practice for using public, private, and protected methods. I want to make sure this is clear for me since the canvas has multiple classes, and I want to be consistent across the methods. I think there is almost two use cases:

1. Public for the API that we present to the user.
2. Public for other classes to call.

I originally tried to make all the non-API facing methods protected, and then their properties private if I needed to use getters/setters. I found that it ended up being pretty confusing. I went and looked at what other projects like Requests were doing as a data point, and it looked like they were just excluding methods from the API documentation. This is the approach I ended up taking including documenting the "Main Interface" separate from "Lower-Level Classes".

I see the drawbacks of making the draw method protected are that I get linter warnings for accessing it outside the class it belongs to (which I could ignore I suppose), and they aren't really "protected" for use case 2 above.

If making draw protected is the right answer, then I will make the canvas and add_drawing_object methods protected also for consistency.

Any guidance you could provide would be greatly appreciated, thanks!

[freakboy3742]

Which linter is giving you the error? Does it give you any reference for the "bug" it found? Because that seems like an error in the linter, rather than a bug in the code. PEP8 describes the single underscore prefix as a 'weak "internal use" indicator'; import * won't import a symbol that starts with underscore, but the name is otherwise preserved and available. Using an "internal" symbol across modules should be perfectly acceptable usage, AIUI.

I agree that there are two interpretations of "public" here, but it's the first one that you mention that is most important from my perspective. We're in charge of the internals (and the internal usage), so we can expect a certain amount of cooperation and compliance in the second case; but the first case is where we put things in the hands of the user base at large.

I'm not a huge fan of the Requests approach to API safety. While documentation is an important part of the picture, a big feature of Python as a language is discoverability. I can drop into a prompt at any point, run dir(), and find out what is there for me to poke. Seeing _draw() implies there's a method that can be invoked, but it's probably not intended for me to use. Seeing draw() implies there's a method that I can invoke; if the only way to verify that I shouldn't be invoking it is to check if it isn't mentioned in a piece of documentation that isn't related to the code I'm currently looking at... that's not a very discoverable control.

In this case, draw() is very clearly an internal method. It's entirely appropriate for the impl layer to invoke it (after all, that's what the method exists for), but end users shouldn't even know it exists. We can't completely hide it, but we can make it obvious from naming that they shouldn't be touching it.

canvas and add_drawing_object(), on the other hand, aren't really intended for public consumption, but it doesn't really matter if the user does access them. There in the category of "well, you probably shouldn't be poking here, but it doesn't really matter if you do".

canvas will be an interface level object, so accessing it isn't a big problem. Modifying it might lead to some entertaining results, but I think they'd at least be predictable entertaining results (generally objects disappearing from one canvas, appearing on another).

As for add_drawing_object() - while I agree end users generally shouldn't need to invoke this one, it shouldn't be inherently dangerous to invoke it. I'd even suggest renaming the method as add(). While we don't necessarily want to encourage it's use, but it's not harmful to put another drawing object onto a context, as long as that object adheres to the interface requirements of a drawing object.

[danyeaw]

@freakboy3742 So my summary of what you said is that we should really only be making methods protected if access to the method from the public API could be harmful, correct?

I completed the refactoring by changing draw to _draw, and renaming add_drawing_object to add_draw_obj. Although I agree that calling it just add() would be nice, the Widget class already has an add method, and the Canvas is inheriting from it.

[freakboy3742]

Correct - that's my reading of the general intent of PEP8; and if that isn't the intent... that's what I'm running with in Toga's API design :-P