Add optional destination page to Shape.copy

[stevensultana]

There are two possible approaches. Either the one proposed in the PR where there is a single method with an optional parameter.

Or else we can keep the existent Shape.copy() as it is and add a new method, called Shape.copy_to(), which requires a Page as a parameter.

[stevensultana]

There seems to be some issue with calculating the ID of a new Shape, specifically when copying to a page which already has a Shape inside it, but which is not the originating page.

I added more checks to the existent test to show this issue. Will go through the code to see what I can find.

[dave-howard]

Hi Steven

The new tests are calling VisioFile.copy_shape() rather than the updated Shape.copy() method.

See how test.py::test_vis_copy_shape() calls page.set_max_ids() before calling vis.copy_shape()...

This is dependency is wrapped up in Shape.copy() - I would expect users of the package to interact with Page and Shape methods.

Note also in Shape.copy() - set_max_ids() needs to be called on the destination page (rather than source Shape's page) - that might be a cause of the test failures?

More generally: I am thinking that Shape.copy() should take a destination arg of either type Page or Shape, and where that arg is None it should create in same shape parent as source Shape. Taking a Page arg is good step in that direction.

[stevensultana]

The new tests are calling VisioFile.copy_shape() rather than the updated Shape.copy() method.

See how test.py::test_vis_copy_shape() calls page.set_max_ids() before calling vis.copy_shape()...

This is dependency is wrapped up in Shape.copy() - I would expect users of the package to interact with Page and Shape methods.

Agreed that users would interact with Pages and Shapes. In the meantime, I still tackled the problem from the VisioFile.copy_shape(), with the rationale that it would also resolve it for Shape.copy(). Does that make sense?

Note also in Shape.copy() - set_max_ids() needs to be called on the destination page (rather than source Shape's page) - that might be a cause of the test failures?

This was indeed part of the problem, but not the only one, which is why at that point I only pushed the test file, to make a note that something was the matter.

More generally: I am thinking that Shape.copy() should take a destination arg of either type Page or Shape, and where that arg is None it should create in same shape parent as source Shape. Taking a Page arg is good step in that direction.

I imagine this also relates to your later comment on parent_xml.

I'll push my current progress, which at least seems to work. There were a few issues scattered around, not least in the test that I created itself.

But I'll review the code again with your comments in mind... at a more decent time :)

[stevensultana]

Hello! I fixed the parent_xml thing. I think there is a similar issue in Page.shapes(), right? I didn't mess with this before confirming my understanding.

I also discovered the ET.find(), Element.find() and .findall() functions which we might look into employing to simplify some of the code - I'll be happy to do some trudge work if you wish :)

Let me know also about the max_id thing: shall I revert to having Shape.copy() take care of the page's max_id? Or shall I leave it to VisioFile.copy_shape()?

I still need to look into have a Shape be the destination arg to Shape.copy(). What would that mean, that the new shape is placed as a sub-shape to the argument? So: shape_c = shape_a.copy(shape_b) would result in shape_c being a sub-shape of shape_b?

[dave-howard]

Hi Steven - that's great, I'll look to merge in.

Regards find, findall functions - I had a quick look at these when I started this and they (much more likely me!) seemed to struggle with XML namespaces - but yes there must be a better way of processing XML.

ref shape_c = shape_a.copy(shape_b) yes that's the idea in my head, though I would like to be able to pass in Shape, Shape which is actually a group type, or Page as the destination

[stevensultana]

Regarding the find functions, yes I have a bit of a surprise, but after some messing around I figured out that shapes_xml = xml.find(namespace+'Shapes') works. But maybe a better forum to discuss this would be an issue?