Part: allow installing attacher extension on demand

[realthunder]

Forum thread: https://forum.freecadweb.org/viewtopic.php?f=3&t=49464&sid=240cd565d4d4b079ad7297af6cf1b531

[DeepSOIC]

I will test it before stating for sure, but i'm afraid there will be recompute issues because positionBySupport may not be called by object's execute routine.

Also, there is a bunch of unreachable code left by this change in TaskAttachmentEditor.py, soon after the code that adds the extension, where a message pops up saying the object is not attachable and thus will not be parametric. Basically, the whole if block can be eliminated as a result of this change.

[DeepSOIC]

When attachengine is installed on objects that don't expect it, attachment is not updating properly.
Example.

1. make Part Cube, Cylinder and Sphere
2. fuse cube and sphere
3. select fusion, menu Part->Attachment. Attach to Cylinder in some way. OK.
4. move cylinder
   ->fusion does not follow. Though it snaps into place as soon as I touch any attachment-related property.

[realthunder]

Yeah, that's because Fusion does not call it's parent execute(), so no triggering of extensionExecute() in AttachExtension, like you said.

The way I see it. Unless there is real harm, like crash or something, I don't see any problem enabling AttachExtension on demand. If the user finds out that some object with the extension does not working and report it, then we can fix it. Like the Fusion here. If some object is really not meant to have AttachExtension, then we can filter it in TaskAttachment.py here. Or maybe make ExtensionContainer::registerExtension() virtual, and let each object decide.

[DeepSOIC]

Thanks!
How about the now-redundant if in TaskAttachmentEditor.py?

[realthunder]

I added code to handle exception when calling addExtension(), just in case an object can choose to reject the extension in the future. So those if checks still have their use.

[wwmayer]

Some remarks:

• The AttachExtensionPython is always added if the selected objects lacks of the AttachExtension. Since this is done automatically and affects any kind of object you selected wouldn't it be nicer to pop-up a dialog and ask the user beforehand?
  In case the user does this on purpose and don't want this dialog to pop-up each time a check mark could be added to not show the dialog any more.

• Currently we have two implementations of the attachment task panel. One is written in Python and is accessible as command within the Part wb. The other one is written in C++ and is accessible via the property editor. When using the latter then the attachment extension is not added

• Originally the execute() command was not designed to call the execute() method of its parent class. Now sometimes a class invokes the method of its parent class and in most other classes it doesn't. IMO, this will lead to quite some confusion and inconsistent behaviour.
  Currently in FEM and TechDraw there are many classes that invokes the parent's function and there it's obviously on purpose which is OK.
  In Part it's done for the PrimitiveFeature classes but it only hides the actual bug. IMO when executing a feature then the document must make sure to also always execute its extensions. It shouldn't depend on how the execute method of a feature is implemented.
  That's why I recommend to move the code from DocumentObject::execute() to DocumentObject::recompute()

[DeepSOIC]

* Originally the execute() command was not designed to call the execute() method of its parent class. Now sometimes a class invokes the method of its parent class and in most other classes it doesn't. IMO, this will lead to quite some confusion and inconsistent behaviour.
  Currently in FEM and TechDraw there are many classes that invokes the parent's function and there it's obviously on purpose which is OK.
  In Part it's done for the PrimitiveFeature classes but it only hides the actual bug. IMO when executing a feature then the document must make sure to also always execute its extensions. It shouldn't depend on how the execute method of a feature is implemented.
  That's why I recommend to move the code from DocumentObject::execute() to DocumentObject::recompute()

There is one little problem with it: should extensions be executed before or after object's execute? What if the object needs it in the middle?
There aren't many cases where it matters now: the only case I know of is Sketcher, which requires attachment be executed before recomputing the sketch, because up-to-date placement is required for external geometry projection. Current implementation requires calling extensions explicitly, which offers all the flexibility, but doesn't support dynamic extensions well. The new system, implemented naively, will not offer any flexibility. I guess it can still be implemented to support order override... i'm just raising awareness.

[realthunder]

• The AttachExtensionPython is always added if the selected objects lacks of the AttachExtension. Since this is done automatically and affects any kind of object you selected wouldn't it be nicer to pop-up a dialog and ask the user beforehand?
  In case the user does this on purpose and don't want this dialog to pop-up each time a check mark could be added to not show the dialog any more.

It feels like a bit redundant. Because the intention of clicking that command is to use the attacher, the user will almost always say yes, unless we put some scary warning message in the dialog. A better way would be to make it possible to undo extension insertion, but that would be another topic. Anway, for attach extension in particular, I don't see any harm in adding it. Because if it has any ill effect, the user can always turn it off completely.

• Currently we have two implementations of the attachment task panel. One is written in Python and is accessible as command within the Part wb. The other one is written in C++ and is accessible via the property editor. When using the latter then the attachment extension is not added

But the one in property editor is only available if the object has already installed attacher extension? I am not sure though. Looks like PropertyEnumAttacherItem is only used by AttachExtension.

• Originally the execute() command was not designed to call the execute() method of its parent class. Now sometimes a class invokes the method of its parent class and in most other classes it doesn't. IMO, this will lead to quite some confusion and inconsistent behaviour.

Yeah, I have the same concern as DeepSOIC. No idea how to solve it yet.

[wwmayer]

There is one little problem with it: should extensions be executed before or after object's execute? What if the object needs it in the middle?

In the ExtensionContainer class we have the same situation that the extension methods are executed either before or after the methods of the base class -- so not much flexibility. But so far we never had a problem this way.

There aren't many cases where it matters now: the only case I know of is Sketcher, which requires attachment be executed before recomputing the sketch, because up-to-date placement is required for external geometry projection. Current implementation requires calling extensions explicitly, which offers all the flexibility, but doesn't support dynamic extensions well.

First of all, do we have consensus about to by default always call the extensionExecute() methods when recomputing a feature?
The problem we have is that this PR only addresses Part features but by automatically adding the attachment object to each object (e.g .mesh, fem, point features) their behaviour is inconsistent because the extensions are not executed.

The new system, implemented naively, will not offer any flexibility.

The flexibility you want is very easy to achieve: the method that invokes execute() is called recompute() and this is also a virtual method. So, e.g. the SketchObject class only has to override recompute() and first calls extensionExecute() and afterwards its actual execute() method.
It's even possible that a feature can completely ignore its extensions if needed or call their extensionExecute() within its execute() method.

But the one in property editor is only available if the object has already installed attacher extension? I am not sure though. Looks like PropertyEnumAttacherItem is only used by AttachExtension.

Hmm, good question. I recently tested your PR with the example uploaded in the forum. By using the attachment dialog via property editor I never got it working as expected and it took me a while until I figured out to call the command from the menu. I will have a look again...

[DeepSOIC]

In the ExtensionContainer class we have the same situation that the extension methods are executed either before or after the methods of the base class -- so not much flexibility.

One can:

• not call DocumentObject::execute, and have full control (and responsibility) to execute extensions.
• call DocumentObject::execute before actually doing things to pre-execute all extensions
• call DocumentObject::execute after actually doing things to post-execute all extensions

the only inflexibility i see now is if there is a need to call superclass's execute but not let it execute extensions, there is no way to do it other than copy-pasting super's execute code with slight edits. Doesn't sound like a very big deal.

The flexibility you want is very easy to achieve

Okay =)

[wwmayer]

Merged.

The first commit has been added as it is: c5ce992
The second and fourth commit plus a needed fix have been squashed to: ea2e46d
The third commit has been replaced with: a30cf5b

This way we don't have to care about implementation details of the execute() function of subclasses (e.g. SketchObject).