[BUG] Cancel does not work with Datum Planes

About the development of the Part Design module/workbench. PLEASE DO NOT POST HELP REQUESTS HERE!
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
PCR
Posts: 44
Joined: Wed Apr 21, 2021 1:07 pm

[BUG] Cancel does not work with Datum Planes

Post by PCR »

OS: Windows 10 (10.0)
Word size of OS: 64-bit
Word size of FreeCAD: 64-bit
Version: 0.19.24291 (Git)
Build type: Release
Branch: (HEAD detached at 0.19.2)
Hash: 7b5e18a0759de778b74d3a5c17eba9cb815035ac
Python version: 3.8.8
Qt version: 5.12.9
Coin version: 4.0.0
OCC version: 7.4.0
Locale: German/Germany (de_DE)

when i click cancel the datum plane is created..
PCR
Posts: 44
Joined: Wed Apr 21, 2021 1:07 pm

Re: [BUG] Cancel does not work with Datum Planes

Post by PCR »

chrisb
Veteran
Posts: 53785
Joined: Tue Mar 17, 2015 9:14 am

Re: [BUG] Cancel does not work with Datum Planes

Post by chrisb »

It is a bit sophisticated, but it seems that creating and editing are two things. So the DatumPlane is created and then you enter immediately editing mode. You can cancel the latter, and no editing will be done, which means in your case that the DatumPlane remains attached to the face which was selected when creating the DatumPlane.

You can after canceling use undo to make the creation undone as well.
A Sketcher Lecture with in-depth information is available in English, auf Deutsch, en français, en español.
User avatar
jnxd
Posts: 951
Joined: Mon Mar 30, 2015 2:30 pm
Contact:

Re: [BUG] Cancel does not work with Datum Planes

Post by jnxd »

chrisb wrote: Mon May 03, 2021 6:01 pm It is a bit sophisticated, but it seems that creating and editing are two things. So the DatunmPlane is created and then you enter immediately editing mode. You can cancel the latter, and no editing will be done, which means in your case that the DatumPlane remains attached to the face which was selected when creating the DatumPlane.

You can after canceling use undo to make the creation undone as well.
It is particularly odd to see this behavior only for datum objects. I observed this too (mistakenly re-reported here) and took a look at the source a couple days ago. From what I can tell, the setEdit() command is also used for the solid features like pad or pocket, but they are destroyed when the edit is canceled...
My latest (or last) project: B-spline Construction Project.
openBrain
Veteran
Posts: 9031
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: [BUG] Cancel does not work with Datum Planes

Post by openBrain »

I had this tab opened for a pretty long time.
I had a quick look now and absolutely can't find what commits the undo transaction in the case of the Datum objects but not in the case of a Pad/Pocket/... :?
Will try to have another look later on.
troyp76
Posts: 32
Joined: Thu May 06, 2021 8:05 am

Re: [BUG] Cancel does not work with Datum Planes

Post by troyp76 »

Recently used Datum planes for a model and came across this issue.

My understanding after reviewing the code is there are two commands being added to the undo list when a DatumPlane is created:
- Create DatumPlane
- Edit Attachment

When the user presses the Cancel button (linked to the reject method in the code) only the currently active command "Edit Attachment" is aborted.
The "Create DatumPlane" had already been committed so it is left untouched, thus the object remains in the tree.
As chrisb suggests a manual undo on the GUI will then remove the DatumPlane object.

Note: this applies to any of the Datum objects not just planes.

To address this an additional undo command can be used to clean up the object iff the parameter editor was opened when creating a new object vs editing an existing object .

I have created a branch that demonstrates this does fix the issue.
Not sure if it's the right approach thus haven't submitted a PR yet.

Rather I suspect it would be better to redirect the context menu / double click events to the UnifiedDatumCommand() method that distinguishes between the create and edit cases. (eg "Edit DatumPlane" vs "Create DatumPlane" undo entry).

Have a look at the branch, if there is general agreement that it's acceptable as is I will clean it up and submit a PR.

Otherwise I can look at using UnifiedDatumCommand when I get some more time.
openBrain
Veteran
Posts: 9031
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: [BUG] Cancel does not work with Datum Planes

Post by openBrain »

troyp76 wrote: Wed Sep 29, 2021 8:10 am I have created a branch that demonstrates this does fix the issue.
Not sure if it's the right approach thus haven't submitted a PR yet.

Have a look at the branch, if there is general agreement that it's acceptable as is I will clean it up and submit a PR.
There is a general agreement (with myself :) ) that your branch looks like an horrible dirty patch. :lol:

We discussed a global solution already on this thread : https://forum.freecadweb.org/viewtopic.php?f=10&t=62063
I'm currently listing the impacted classes before doing a draft PR : the PR will be very fast, but testing for regression everywhere (and eventually fixing) will be a huge task -- and I'll probably request for some help from the community --.

As would say GitHub, "You can safely delete your branch" :lol:
troyp76
Posts: 32
Joined: Thu May 06, 2021 8:05 am

Re: [BUG] Cancel does not work with Datum Planes

Post by troyp76 »

For the record the now deleted branch was definitely a "horrible dirty patch" intended to get the discussion going again. :lol:

At least it wasn't a complete waste of time as there is a link to the PR and other discussion thread now. ;)
openBrain
Veteran
Posts: 9031
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: [BUG] Cancel does not work with Datum Planes

Post by openBrain »

troyp76 wrote: Wed Sep 29, 2021 11:03 am At least it wasn't a complete waste of time as there is a link to the PR and other discussion thread now. ;)
Sure, and maybe you'll be interested in testing regressions in the coming PR.
Just so you see amount of work, all classes in the picture below linked to the right of ViewProviderGeometryObject (grey box) shall be checked for regression (and eventually fixed). :twisted:
VPGeomObj_inh.png
VPGeomObj_inh.png (874.93 KiB) Viewed 5016 times
troyp76
Posts: 32
Joined: Thu May 06, 2021 8:05 am

Re: [BUG] Cancel does not work with Datum Planes

Post by troyp76 »

openBrain wrote: Wed Sep 29, 2021 2:38 pm Sure, and maybe you'll be interested in testing regressions in the coming PR.
Are you buying the pizza and beers? :D

Let me know when the PR is ready and then we can chat about how to go about it.
Post Reply