spakin / SimpInkScr

Simple Inkscape Scripting
https://inkscape.org/~pakin/%E2%98%85simple-inkscape-scripting
GNU General Public License v3.0
320 stars 31 forks source link

Improve repr() output of SimpleObject? #121

Closed user202729 closed 8 months ago

user202729 commented 8 months ago

Currently, if the user want to execute something like

print(all_shapes())

the output will be rather non-descriptive, consisting of things like

<simpinkscr.simple_inkscape_scripting.SimplePathObject object at 0x7fdb42497150>

It would be much better if __repr__ of SimpleObject could return something like

path([Move(62.95748, 11.816248), Line(77.990866, 45.520338), Line(124.99147, 42.548885), Line(99.204925, 19.009635), ZoneClose()], stroke='#800080', stroke_width=3, stroke_linecap='round', stroke_dasharray='none')

What's your opinion on implementing this?

As another advantage, this would allow the user to quickly tell what's the simple interface for some figure, simply by drawing the figure with Inkscape GUI, run selected_shapes(), then copy the output.

Note that most of the code is already there in the SVG to Python conversion code. (SimpInkScr/simpinkscr/svg_to_simp_ink_script.py)

Some notes to look out for while implementing this:

spakin commented 8 months ago

What's your opinion on implementing this?

I think it's reasonable for Simple Inkscape Scripting to present a more descriptive representation of a SimpleObject. However, I don't like the idea of the representation being an arbitrarily long string, especially in the case of a path object. The representation should be just enough for the user to recognize the object. Perhaps this could comprise

What do you think of that representation?

user202729 commented 8 months ago

Normally, the Python guide is that "str should be human readable, repr should be unambiguous". But up to you.

Currently, str is already used for url(#id) which is because of the implementation quirk, but I think it doesn't need to be that way -- with the intention of it, looks like it's mostly passed to _construct_style(base_style, new_style)_python_to_svg_str, but there's no reason why we can't have a special svg_str() method on each object.

If the plan is to make repr some short but still reproducible output, maybe we make each object just output the literal string consist of its id, and populate the global scope with the objects such that each object is its id? And maybe hide the "complete" description in some method. I think the code would be cleaner to make all the convert_path etc. a property on the corresponding Path etc. class, instead of a huge switch case (but that would need a major refactoring effort at this point; another point is that it would need to output Statement object instead of just a str?)

That is, make variables like path26 visible in global scope, and repr(path26) just return path26. (or path(path26) if you want to include the shape)

For bounding box, maybe we can just add a method like <object>.select() that selects the object on the canvas, which allow the user to find where it is.

For comparison: SageMath represents a variable as just its variable name (without the method how to construct it), but usually the variable is visible in the global scope with the same name. The function sage_input() gives a sequence of commands that reproduce the value.

(context: I think using print(<object>) is a more convenient way to see what the object is than navigating through Inkscape menu and save the current canvas to a file. Especially if you only want to view the currently selected object.

I don't think there's a particularly easy way to get such a list of coordinates like this without drawing the shape with the GUI and export it to SimpInkScr .py at the moment:

head = circle((0, 0), 25, stroke='black', fill=fill_color)
r_eye = circle((-9.2, -9.8), 5.4)
l_eye = circle((9.2, -9.8), 5.4)
mouth = path([Move(16.2, 4.2),
              Curve(17.6, 10.3, 9.0, 19.4, 0.2, 19.4),
              Curve(-9.0, 19.4, -17.8, 10.2, -16.2, 4.2),
              Curve(-8.7, 9.4, 6.3, 9.4, 16.2, 4.2),
              ZoneClose()])

and using print(all_shapes()) (or print([shape.complete_repr() for shape in all_shapes()]) assume the method is to be named .complete_repr()) would be easier than navigating through the GUI.)

spakin commented 8 months ago

I've been pondering your comments. Here's what I'm currently thinking:

user202729 commented 8 months ago

For the first point I mentioned:

Currently, str is already used for url(#id) which is because of the implementation quirk, but I think it doesn't need to be that way -- with the intention of it, looks like it's mostly passed to _construct_style(base_style, new_style)_python_to_svg_str, but there's no reason why we can't have a special svg_str() method on each object.

It already did a lot of checking anyway:

https://github.com/spakin/SimpInkScr/blob/master/simpinkscr/simple_inkscape_scripting.py#L87-L116

For the last point I mentioned:

(context: I think using print(<object>) is a more convenient way to see what the object is than navigating through Inkscape menu and save the current canvas to a file. Especially if you only want to view the currently selected object.)

(We do already have a feature to export the current canvas to a Python file that reproduces the canvas.)

The rest, up to you.

spakin commented 8 months ago

All right, I made a temporary branch of Simple Inkscape Scripting called repr-output. The code on this branch defines __repr__ to output the object's Simple Inkscape Scripting object type (e.g., SimpleGroup), the underlying inkex tag (e.g., g), the center of the object's bounding box (e.g., (824.45, 280.346)), and the ID of the underlying SVG object (e.g., #g4).

I don't know if this new repr output can be improved substantially for str so for now I removed the __str__ method. Consequently, str(obj) produces the same output as repr(obj). Because str no longer produces an SVG URL, I added more cases to _python_to_svg_str—as you point out above, there already were a lot of cases—to handle Simple Inkscape Scripting objects explicitly.

Please try repr-output and post your thoughts here.

user202729 commented 8 months ago

Looks good to me.

Technically, using str() on inkex object (as was used before the change) won't work out, I think. So this one is an improvement I suppose.

Minor point: User might get slightly confused if the name SimpleObject was never introduced in the tutorial but appear in the printed output though (?) but I think the other parts of the name can clarify it.

SimpleGuide repr is not yet implemented yet.

For some strange reason, SimpleTextObject bounding box isn't correctly computed (it only measure the lower left corner I think...?), although I'd consider this an issue with the underlying inkex library.

Using same str and repr might be reasonable for most object -- if I recalled correctly, except for str type, str() and repr() is the same -- which makes sense because the other objects can appear unambiguously when appear as an element in e.g. a list.

spakin commented 8 months ago

Looks good to me.

Technically, using str() on inkex object (as was used before the change) won't work out, I think. So this one is an improvement I suppose.

Great!

Minor point: User might get slightly confused if the name SimpleObject was never introduced in the tutorial but appear in the printed output though (?) but I think the other parts of the name can clarify it.

I probably ought to write a page for the wiki that at least mentions the various Simple Inkscape Scripting classes.

SimpleGuide repr is not yet implemented yet.

As of 00d7f50a7eb71626eb2958dfb07dbd3ff2d635a8, __repr__ should be implemented for all Simple Inkscape Scripting classes.

For some strange reason, SimpleTextObject bounding box isn't correctly computed (it only measure the lower left corner I think...?), although I'd consider this an issue with the underlying inkex library.

That's accurate. According to the documentation of TextElement.shape_box,

Returns a horrible bounding box that just contains the coord points of the text without width or height (which is impossible to calculate)

Thanks for reviewing Simple Inkscape Scripting's new __repr__ support. When I get a chance I'll merge repr-output into master and close this issue.

spakin commented 8 months ago

Resolved by 9d4e4acf3589a88cc9330b445cf504355474dfd7.