Chart Widget #586

polarmutex · 2018-08-26T14:55:29Z

This PR allows the user to render a matplotib plot figure onto a toga canvas. I also added a clearAll function to the canvas to be able to clear the draw command from the canvas to render a new canvas.

There are also more to implement here to get all matplolib figures rendering correctly in the canvas widget. Such as images, hatch styles, and alpha. Text is currently displayed by converting to path commands, it looks good to me but the draw text in the canvas can be updated to change this.

I have some discussion topics related to this:

As currently implemented it is not interactive. Do you have future plans to have a mouse action API in core? It looks like you have a keyboard api in the constants. If not I could add the mouse handlers into the impl code.

Right now we have a closed_path type only. I left it as is and created a generic path type. As I read the path commands from matplotlib I just add them to the path class. Do we want to keep both?

Also the closed_path draw command on cocoa and gtk do not need the x,y coords should we change the API to drop them?

Refs #545

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

freakboy3742 · 2018-08-27T01:50:41Z

@bryall \o/ This is amazing - thanks!

Looks like you'll need to add something to the Beekeeper configuration to add matplotlib to the test environment. However, I don't want to add matplotlib (and thus numpy, and others) to install_requires as a dependency for all toga apps.

There are a couple of options here.

We could just add an EXTRA_REQUIRES line to the Beekeeper configuration; this would let the tests pass, but anyone who used the widget in production would see odd errors.

Another option would be to make chart an extra_requires configuration for charts. This would mean anyone who wanted to use the chart widget would pip install toga[chart] (or something close to that).

A third option, we could handle this by making chart an "external widget" - i.e., a separate repository that just has the chart widget in it. The console widget from Yorkshire4 gives an example of how this can be done. I'd be happy to maintain this new repository in BeeWare as an official repository; I just need someone to set up the initial repository.

goanpeca · 2018-08-27T03:15:19Z

I like option 3 :-p

polarmutex · 2018-08-27T03:57:42Z

I am good with option 3.

freakboy3742 · 2018-08-29T01:54:03Z

@bryall Awesome - I've just created a stub project https://github.com/pybee/toga_chart so you've got something to issue a PR against; if you can transfer this over to that repository (and fix up any other problems with the repo that you notice), I'll merge that in!

brutusthebee · 2018-09-11T03:24:11Z

src/core/toga/__init__.py

@@ -11,6 +11,7 @@
 from .widgets.box import Box
 from .widgets.button import Button
 from .widgets.canvas import Canvas
+from .widgets.chart import Chart
 from .widgets.detailedlist import DetailedList
 from .widgets.image import *


At column 1: (F403) 'from .widgets.image import *' used; unable to detect undefined names

brutusthebee · 2018-09-11T03:24:11Z

src/core/toga/__init__.py

@@ -11,6 +11,7 @@
 from .widgets.box import Box
 from .widgets.button import Button
 from .widgets.canvas import Canvas
+from .widgets.chart import Chart
 from .widgets.detailedlist import DetailedList
 from .widgets.image import *
 from .widgets.imageview import *


At column 1: (F403) 'from .widgets.imageview import *' used; unable to detect undefined names

freakboy3742

The code you've got here (along with beeware/toga-chart#1) clearly works; however, I'm not 100% clear on some of the design decisions you've made here. I've flagged the areas that are unclear.

It's entirely possible the fault lies with the existing path/line/closed path objects that were already there; if there is an underlying problem with those objects, I'd rather replace them than overload them with similar, but very slightly different APIs.

By way of guidance, the API we've got here was inspired by the HTML5 Canvas - but it's entirely possible we've missed some things in the translation.

freakboy3742 · 2018-09-17T10:31:40Z

src/core/toga/widgets/canvas.py

@@ -99,6 +99,12 @@ def remove(self, drawing_object):
        self.drawing_objects.remove(drawing_object)
        self.redraw()

+    def clearAll(self):


Why clearAll? Why not just clear?

will change, i think I was trying to differentiate between clearing a single draw command with clearing all the draw commands

freakboy3742 · 2018-09-17T10:31:54Z

src/cocoa/toga_cocoa/widgets/canvas.py

@@ -46,7 +46,8 @@ def create(self):
        self.add_constraints()

    def redraw(self):
-        pass
+        # pass


Minor housekeeping - no need to have the commented out pass.

This one looks like it's still current

freakboy3742 · 2018-09-17T10:35:17Z

src/core/toga/widgets/canvas.py

@@ -459,6 +493,27 @@ def color(self, value):
            self._color = parse_color(value)


+class Path(Context):
+    """A user-created :class:`Path <Path>` drawing object for a
+    path context.


I'm not sure I see what Path is adding here - the implementation of _draw() is the only significant functionality, and the implementation is identical to Context (which is the direct descendent). How is Path any different to Context?

I agree, did not look at Context to closely, this class does not add anything, will fix this

freakboy3742 · 2018-09-17T10:37:28Z

src/core/toga/widgets/canvas.py

@@ -591,6 +646,32 @@ def _draw(self, impl, *args, **kwargs):
        impl.move_to(self.x, self.y, *args, **kwargs)


+class CloseTo:
+    """A user-created :class:`CloseTo <CloseTo>` drawing object which closes
+     the polyline vack to the starting point


I'm a little unclear about why CloseTo is needed if we also have ClosePath. Is there really a need for two different ways to draw a closed path?

with close path you have to know before adding the path segments that it is closed. matplotlib gives an array of draw commands. When I loop over the commands, I do not know it is closed till that last command telling me to close the segment. With the two implemented platforms so far. To close the segment does not require passing it the beginning point to close to. So adding this type makes it easier for me to just loop over the draw commands and add each one to the drawing context.

Ok; that makes some sense; I missed that this isn't a context, it's a drawing primitive.

I think I understand the use case; however, I'm still not 100% sure if a new primitive is needed here.

As I understand it, the problem is that at the time you are drawing the primitives, you don't know if the path is closed or open, so you need to be able to put a "close" command at the end of sequence of drawing commands.

My countersuggestion would be that you could use the ClosedPath context manager in a different way to acheive the same goal.

ClosedPath is a context manager, so the default way of using it is:

with canvas.closed_path(x,y) as path: path.line_to(x+10, y+10) path.line_to(x-10, y+10)

but that's no the only way it can be used. A context manager is just an object that has an enter/exit lifecycle; you can create instances of the object and call the underlying primitives on it directly. So, if you don't know if the path is closed until some boolean is evaluated, you could do something like:

draw_objs = [ LineTo(x+10, y+10), LineTo(x-10, y+10), ] if is_closed: target = canvas.closed_path(x,y) else: target = canvas for obj in draw_objs: target.add_draw_obj(obj)

That last part could be simplified by adding an "add_draw_objs()" method, which seems like a likely use case.

The underlying idea here is that closing a path isn't an operation; a closed path is an entity that is constructed from other operations. Does that make sense?

(Also, small typo in the docstring - vack)

polarmutex · 2019-04-13T03:02:18Z

Sorry I was busy and left this partially done.

updated the code based on comments. Once I get these changes into core, I will move to toga-chart

freakboy3742

This looks like it's heading in the right direction; I've flagged one architectural question, and some minor cosmetic bits.

freakboy3742 · 2019-04-13T08:52:14Z

src/cocoa/toga_cocoa/widgets/canvas.py

@@ -46,7 +46,8 @@ def create(self):
        self.add_constraints()

    def redraw(self):
-        pass
+        # pass


This one looks like it's still current

freakboy3742 · 2019-04-13T08:52:54Z

src/core/toga/widgets/canvas.py

@@ -458,7 +478,6 @@ def color(self, value):
        else:
            self._color = parse_color(value)

-


It's a minor style thing, but convention is 2 blank lines between class declarations (not sure why this didn't get flagged by Beefore...)

freakboy3742 · 2019-04-13T10:07:48Z

src/core/toga/widgets/canvas.py

@@ -591,6 +646,32 @@ def _draw(self, impl, *args, **kwargs):
        impl.move_to(self.x, self.y, *args, **kwargs)


+class CloseTo:
+    """A user-created :class:`CloseTo <CloseTo>` drawing object which closes
+     the polyline vack to the starting point


Ok; that makes some sense; I missed that this isn't a context, it's a drawing primitive.

I think I understand the use case; however, I'm still not 100% sure if a new primitive is needed here.

As I understand it, the problem is that at the time you are drawing the primitives, you don't know if the path is closed or open, so you need to be able to put a "close" command at the end of sequence of drawing commands.

My countersuggestion would be that you could use the ClosedPath context manager in a different way to acheive the same goal.

ClosedPath is a context manager, so the default way of using it is:

with canvas.closed_path(x,y) as path: path.line_to(x+10, y+10) path.line_to(x-10, y+10)

but that's no the only way it can be used. A context manager is just an object that has an enter/exit lifecycle; you can create instances of the object and call the underlying primitives on it directly. So, if you don't know if the path is closed until some boolean is evaluated, you could do something like:

draw_objs = [ LineTo(x+10, y+10), LineTo(x-10, y+10), ] if is_closed: target = canvas.closed_path(x,y) else: target = canvas for obj in draw_objs: target.add_draw_obj(obj)

That last part could be simplified by adding an "add_draw_objs()" method, which seems like a likely use case.

The underlying idea here is that closing a path isn't an operation; a closed path is an entity that is constructed from other operations. Does that make sense?

(Also, small typo in the docstring - vack)

polarmutex · 2019-04-13T15:29:30Z

I have made the changes, and I think this is good to go

freakboy3742

Looks great! Thanks for the contribution!

initial chart widget impl

3ef78a9

removed chart widget to be moved into its own repo

3365fe6

brutusthebee reviewed Sep 11, 2018

View reviewed changes

forgot to remove chart from the __init__.py

5a0a489

freakboy3742 mentioned this pull request Sep 17, 2018

initial chart widget beeware/toga-chart#1

Merged

freakboy3742 reviewed Sep 17, 2018

View reviewed changes

Brian Ryall added 2 commits April 11, 2019 21:43

Merge branch 'master' into chart-widget

602509d

changes from peer review

a8830ca

freakboy3742 requested changes Apr 13, 2019

View reviewed changes

Brian Ryall added 2 commits April 13, 2019 11:16

removed comment, remove not needed CloseTo

6c4ad91

added back in newline that was accidently deleted

5e3b5a7

freakboy3742 approved these changes Apr 13, 2019

View reviewed changes

freakboy3742 merged commit 0e540d5 into beeware:master Apr 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chart Widget #586

Chart Widget #586

polarmutex commented Aug 26, 2018 •

edited

Loading

freakboy3742 commented Aug 27, 2018

goanpeca commented Aug 27, 2018

polarmutex commented Aug 27, 2018

freakboy3742 commented Aug 29, 2018

brutusthebee Sep 11, 2018

brutusthebee Sep 11, 2018

freakboy3742 left a comment

freakboy3742 Sep 17, 2018

polarmutex Sep 22, 2018

freakboy3742 Sep 17, 2018

freakboy3742 Apr 13, 2019

freakboy3742 Sep 17, 2018

polarmutex Sep 22, 2018

freakboy3742 Sep 17, 2018

polarmutex Sep 22, 2018

freakboy3742 Apr 13, 2019

polarmutex commented Apr 13, 2019

freakboy3742 left a comment

freakboy3742 Apr 13, 2019

freakboy3742 Apr 13, 2019

freakboy3742 Apr 13, 2019

polarmutex commented Apr 13, 2019

freakboy3742 left a comment

		@@ -458,7 +478,6 @@ def color(self, value):
		else:
		self._color = parse_color(value)

Chart Widget #586

Chart Widget #586

Conversation

polarmutex commented Aug 26, 2018 • edited Loading

PR Checklist:

freakboy3742 commented Aug 27, 2018

goanpeca commented Aug 27, 2018

polarmutex commented Aug 27, 2018

freakboy3742 commented Aug 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freakboy3742 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

polarmutex commented Apr 13, 2019

freakboy3742 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

polarmutex commented Apr 13, 2019

freakboy3742 left a comment

Choose a reason for hiding this comment

polarmutex commented Aug 26, 2018 •

edited

Loading