#5480 Sketcher: copy,paste,export as xml geometries with ctrl-c/x ctrl-v

Post here if you have re-based and finalised code to integrate into master, which was discussed, agreed to and tested in other forums. You can also submit your PR directly on github.
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
Post Reply
User avatar
paddle
Veteran
Posts: 1391
Joined: Mon Feb 03, 2020 4:47 pm

#5480 Sketcher: copy,paste,export as xml geometries with ctrl-c/x ctrl-v

Post by paddle »

Please test and review :
https://github.com/FreeCAD/FreeCAD/pull/5480
And if someone can merge :D
User avatar
jonasb
Posts: 162
Joined: Tue Dec 22, 2020 7:57 pm

Re: #5480 Sketcher: copy,paste,export as xml geometries with ctrl-c/x ctrl-v

Post by jonasb »

This is certainly a cool feature, thank you!
Regarding the code I have a few comments and suggestions, though.

The first thing that caught my eye: there are no unit tests. I think new features should be covered by automated tests right from the beginning (Ideally you start with the tests: no failing tests means nothing to do ;-)). IIRC @chennes recently added tests for Copy/Paste in the spread sheet module, so there should be some code to look at already.

Then I suggest to break the long methods into smaller, dedicated methods. A single method with 120 lines and 8-9 levels of indentations are too much for my taste. You already wrote comments over each "section". Those give nice method names. Overall this makes the code easier to read (also for your future self).

You also put FreeCAD's XML fragment as "text/plain" into the clipboard. Followed by some string magic to check if it is that XML when reading it back. I suggest to use a dedicated media type instead and check for that directly when pasting.
Does anyone know whether we have a media type for FreeCAD's xml format already defined somewhere? Something like "application/vnd.freecad.fcstd-document+xml" or here specifically "application/vnd.freecad.fcstd-document-fragment+xml"?
(One could even think of adding the same data in different formats, for different purpose. i.e. one for pasting between/within FreeCAD, one as "image/svg+xml" for direct pasting in inkscape, a human readable version as "test/plain", ...)
User avatar
paddle
Veteran
Posts: 1391
Joined: Mon Feb 03, 2020 4:47 pm

Re: #5480 Sketcher: copy,paste,export as xml geometries with ctrl-c/x ctrl-v

Post by paddle »

jonasb wrote: Wed Feb 02, 2022 9:14 pm This is certainly a cool feature, thank you!
Regarding the code I have a few comments and suggestions, though.
Thanks for your feedback !

Regarding unit tests I have no clue what they are and how they work. I'm not sure I can handle another layer of complexity tbh. I'm already struggling all the way to understand the code structure. Having to do that again for something non-vital makes me want to stop.

Personally I prefer standing function. But I can see reason that it's a bad habit and that smaller functions enable more code reusability and probably better readability. So I should try to split it.

This sounds like a good idea to have data in different formats to be compatible with inkscape and others. It would be awesome. Though I'm not sure to be up to the task right now as I have a few other features that need to be finished.
User avatar
jonasb
Posts: 162
Joined: Tue Dec 22, 2020 7:57 pm

Re: #5480 Sketcher: copy,paste,export as xml geometries with ctrl-c/x ctrl-v

Post by jonasb »

Let's continue the discussion in github directly at your PR. That's better than having it scattered over different places.

@0penBrain already pointed you to the standard copy/paste functions. The mime type thingy I mentioned above is already used there.
And just for the record, directly above MainWindow::createMimeDataFromSelection() the currently used mime types for document objects in the clipboard are defined: https://github.com/FreeCAD/FreeCAD/blob ... .cpp#L1640
Post Reply