PR #4000 dialog to edit Part's primitives

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!
User avatar
uwestoehr
Veteran
Posts: 4961
Joined: Sun Jan 27, 2019 3:21 am
Location: Germany
Contact:

PR #4000 dialog to edit Part's primitives

Post by uwestoehr »

This PR adds the feature to edit Part's primitives via its dialog: https://github.com/FreeCAD/FreeCAD/pull/4000

I had to change the primitives location dialog was not suitable to be used for editing (Gui::Location widget misses the angle information). The aim is that one can open the dialog to edit, and subsequently lose the dialog without any change and as result there should indeed nothing be changed. That was impossible with the old dialog.

To assure that I am on the right track, I implemented this now only for new primitives and the editing only for helices. My solution is so that I can easily extend it for all primitives.

What I could not yet achieve is a live preview of changes one make in the dialog. I would be grateful for a hint on how this could be/is done.

p.s. PR 4000 party! :lol:
User avatar
uwestoehr
Veteran
Posts: 4961
Joined: Sun Jan 27, 2019 3:21 am
Location: Germany
Contact:

Re: PR #4000 dialog to edit Part's primitives

Post by uwestoehr »

My PR was merged and Werner extended it. However I cannot compile FC anymore:

Code: Select all

2>DlgPrimitives.obj : error LNK2019: unresolved external symbol "public: static class Base::Type __cdecl Part::Ellipse::getClassTypeId(void)" (?getClassTypeId@Ellipse@Part@@SA?AVType@Base@@XZ) referenced in function "public: __cdecl PartGui::DlgPrimitives::DlgPrimitives(class QWidget *,class Part::Primitive *)" (??0DlgPrimitives@PartGui@@QEAA@PEAVQWidget@@PEAVPrimitive@Part@@@Z)
2>DlgPrimitives.obj : error LNK2019: unresolved external symbol "public: static class Base::Type __cdecl Part::Circle::getClassTypeId(void)" (?getClassTypeId@Circle@Part@@SA?AVType@Base@@XZ) referenced in function "public: __cdecl PartGui::DlgPrimitives::DlgPrimitives(class QWidget *,class Part::Primitive *)" (??0DlgPrimitives@PartGui@@QEAA@PEAVQWidget@@PEAVPrimitive@Part@@@Z)
2>D:\FreeCAD-build\Mod\Part\PartGui.pyd : fatal error LNK1120: 2 unresolved externals
2>Done building project "PartGui.vcxproj" -- FAILED.

Besides this I am a bit surprised that it went in because this PR was not yet finished. I knew that the preview part was missing and my plan was to extend it to other primitives as well. But before I continue I wanted to get feedback if I am on the right track. Thus I made a PR.
As a relative newbie I still have to learn about the FC code so a short discussion here about my mistakes, missing things etc. would have helped. Now I will of course see how Werner implemented it but still think getting feedback and improving the PR would be the better way to learn how it is done right.
wmayer wrote: .
wmayer
Founder
Posts: 20243
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: PR #4000 dialog to edit Part's primitives

Post by wmayer »

uwestoehr wrote: Sat Nov 07, 2020 2:23 am My PR was merged and Werner extended it. However I cannot compile FC anymore:
This should fix it: git commit f8bd2c81c
Besides this I am a bit surprised that it went in because this PR was not yet finished.
When making a PR then the feature should be finished. If you say it's not finished then please don't make a PR.
I knew that the preview part was missing and my plan was to extend it to other primitives as well. But before I continue I wanted to get feedback if I am on the right track. Thus I made a PR.
The better workflow is not make a PR and instead ask some other developers if the way you go is good or bad. After some discussions you may find some consensus that can be implemented and once finished opening a PR is the way to go.
As a relative newbie I still have to learn about the FC code so a short discussion here about my mistakes, missing things etc. would have helped. Now I will of course see how Werner implemented it but still think getting feedback and improving the PR would be the better way to learn how it is done right.
Here is a list of the issues:
  • I have added the class ViewProviderPrimitive and made the other view provider classes for primitives a sub-class. This way the editing logic needs to be implemented only once (actually three times because the view provider for helices and spirals does some code duplication because they inherit from ViewProviderSpline)
  • It's a bit weird to add a global variable to store the name of the passed document object. C++ in the first place is an object oriented language and thus at least a class member should be used. In general global variables should be avoided as much as possible.
    The second point is that the type of the global variable is a const char* instead of a std::string and if the user deleted the object before clicking OK it could become a dangling pointer. Furthermore it nowhere was checked inside the accept() method that the object still exists.

    To simplify this whole code I changed the constructor to directly pass the pointer to the primitive feature. Internally the pointer is handled inside a DocumentObjectWeakPtrT which has a similar behaviour to C++'s std::weak_ptr. The class DocumentObjectWeakPtrT will be notified if an object has been destroyed in the meantime and thus returns a null pointer and not a dangling pointer.

    As a consequence also the code to get and set the properties could be simplified a lot.
  • When creating new primitives it's OK to add them to the active document. This means if two documents exist and the user changed the active document after opening the dialog it's the desired behaviour to add further primitives to the currently active document.

    BUT: when editing an existing primitive it's wrong behaviour to rely on the active document. The changes the user made must be applied to the document where the primitive belongs to. Also this could be easily implemented by directly passing the pointer of the primitive feature.
  • The code to manipulate the feature was also used as commit message. This of course is wrong and a message "Edit <feature name>" should be used.
  • In the location dialog there was a button "3D view" which allowed to click inside the 3d view to fetch the position data of the placement. By removing this button a warning was printed each time the dialog is opened.
  • The location dialog had two nested group boxes which did not only look bad but it also made the dialog unnecessarily so high that when using it a scroll bar on a 27" monitor was shown.
User avatar
uwestoehr
Veteran
Posts: 4961
Joined: Sun Jan 27, 2019 3:21 am
Location: Germany
Contact:

Re: PR #4000 dialog to edit Part's primitives

Post by uwestoehr »

wmayer wrote: Sat Nov 07, 2020 9:53 am When making a PR then the feature should be finished. If you say it's not finished then please don't make a PR.
What about changing our policy then. I find it convenient to present code and ideas via a PR. This way one can easily try it out and comment in Github. It also gets the necessary audience. I find this PR is a good example: https://github.com/FreeCAD/FreeCAD/pull/3775
A new idea is proposed, I can try it out and comment there. The forum thread is linked to a brief discussion can be done there. The author of the PR can subsequently push changes to Github that can in turn be tried out again.

And at the end not every PR has to be merged, it can be postponed or just closed or stay open for some months as reference.

The better workflow is not make a PR and instead ask some other developers if the way you go is good or bad. After some discussions you may find some consensus that can be implemented and once finished opening a PR is the way to go.
Some time ago you told me that you are on your time limit. So I think by posting a PR to Github every interested person can have a look if he likes and there is then no need to contact certain developers.
For example at work I need a lot of time just to read the many mails I get daily. Other colleagues reported the same and we now set up a ticket system just for tasks. This reduced the amounts of mails and one sees at one look what might be interesting of affecting its own work.
I see a Github PR as a ticked in a tracker with the advantage that one can easily compile the proposed PRs.

[*]It's a bit weird to add a global variable to store the name of the passed document object. C++ in the first place is an object oriented language and thus at least a class member should be used. In general global variables should be avoided as much as possible.
The second point is that the type of the global variable is a const char* instead of a std::string and if the user deleted the object before clicking OK it could become a dangling pointer. Furthermore it nowhere was checked inside the accept() method that the object still exists.
Yes, I was unhappy with the global variable as well. I could just not find a better solution to address the currently selected feature. Thanks also for the hint with the const char* pointer, I have overseen this.

To simplify this whole code I changed the constructor to directly pass the pointer to the primitive feature. Internally the pointer is handled inside a DocumentObjectWeakPtrT which has a similar behaviour to C++'s std::weak_ptr. The class DocumentObjectWeakPtrT will be notified if an object has been destroyed in the meantime and thus returns a null pointer and not a dangling pointer.
Good to know. Up to now I never stumbled over App::DocumentObjectWeakPtrT and did not know about it existence.

BUT: when editing an existing primitive it's wrong behaviour to rely on the active document. The changes the user made must be applied to the document where the primitive belongs to.
Thanks as well, I should have known this :(

[*]The code to manipulate the feature was also used as commit message. This of course is wrong and a message "Edit <feature name>" should be used.
I did not understand how the undo code commit worked. Therefore I took existing code. To understand it, what is the difference between commitCommand() and commitTransaction()?

[*]In the location dialog there was a button "3D view" which allowed to click inside the 3d view to fetch the position data of the placement. By removing this button a warning was printed each time the dialog is opened.
My mistake. Of course the 3D button should not have gone.

[*]The location dialog had two nested group boxes which did not only look bad but it also made the dialog unnecessarily so high that when using it a scroll bar on a 27" monitor was shown.
That is weird because I test all dialogs also on a smaller 15'' laptop monitor and could not see what you observed.


Last but not least, even important :) , where in the code can I have a look to understand how I can add the now missing last piece of a live preview while the primitive is edited?
wmayer
Founder
Posts: 20243
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: PR #4000 dialog to edit Part's primitives

Post by wmayer »

What about changing our policy then. I find it convenient to present code and ideas via a PR.
I disagree! Look at the list of pending PRs that should be handled at some point, too. If we allowed to use PRs only for presenting the code the list would grow even faster and it will become a big mess very quickly.
It might be OK to present some half finished stuff as PR in some exceptional cases but it shouldn't be the standard.

If you want to present your code then open a forum thread and put there the link to your branch. This way a lot more people will be aware of it and can give you advises if needed.
This way one can easily try it out and comment in Github. It also gets the necessary audience. I find this PR is a good example: https://github.com/FreeCAD/FreeCAD/pull/3775
Therefore you don't need a PR. Creating a branch should be enough.
Last but not least, even important :) , where in the code can I have a look to understand how I can add the now missing last piece of a live preview while the primitive is edited?
Look at the class TaskBoxPrimitives of the PartDesign module. For a live preview you basically have to create a slot function for each spin box where you end up in adding a few dozens of new functions.
User avatar
uwestoehr
Veteran
Posts: 4961
Joined: Sun Jan 27, 2019 3:21 am
Location: Germany
Contact:

Re: PR #4000 dialog to edit Part's primitives

Post by uwestoehr »

wmayer wrote: Sat Nov 07, 2020 7:26 pm If we allowed to use PRs only for presenting the code the list would grow even faster and it will become a big mess very quickly.
Maintainers like you could mark them as no ready to be merged or the PR authors do so.
The problem with your proposal to put it into a local branch and/or forum is daily/real life trouble. For example I don't have the time to regularily step to all forum threads to read what is going on. Often I don't have time for FC for a full week and then it is really hard to stay again up to date. The same is with local branches. How can I know that there is an interesting new PR proposal e.g. in Vocx-fc's local branch without reading many forum threads?

The other way round works well for me. After a longer break of coding I came back and looked at open PRs. This way I found PR 3775 gave it a try because it affects my work with FC and thus interested me. Then I commented in the forum since I knew where to discuss. I don't see a problem, of e.g. mark a PR as "[proposal]" or "[under development]" as prefix.

Sure, we would then have quickly several 100 PRs, but one can easily filter those without prefix to get only those from which the authors think they are ready to be merged. One can also set up a rule that PRs, which have not been updated for 6 months, will be closed. I think this will limit the pen PRs to a reasonable amount.

Please give this idea a thought before you deny it. I can imagine that it sound weird to you at the first glance. However, I really believe this would be an improvement, also to relieve maintainers like you from work of having the need of reading so many forum threads to be up to date.

Look at the class TaskBoxPrimitives of the PartDesign module. For a live preview you basically have to create a slot function for each spin box where you end up in adding a few dozens of new functions.
Many thanks! I will have a look.
wmayer
Founder
Posts: 20243
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: PR #4000 dialog to edit Part's primitives

Post by wmayer »

To reduce the number of new slots the QSignalMapper can be used. Then only a new slot for every feature type must be implemented. I will prepare this for the Box and Cylinder feature.

Edit: git commit dabf65cc72a
User avatar
uwestoehr
Veteran
Posts: 4961
Joined: Sun Jan 27, 2019 3:21 am
Location: Germany
Contact:

Re: PR #4000 dialog to edit Part's primitives

Post by uwestoehr »

wmayer wrote: Sun Nov 08, 2020 9:00 am To reduce the number of new slots the QSignalMapper can be used. Then only a new slot for every feature type must be implemented. I will prepare this for the Box and Cylinder feature.
Thanks. I will read to learn about QSignalMapper.
I could extend your solution to the other primitives if you give me a go.
User avatar
uwestoehr
Veteran
Posts: 4961
Joined: Sun Jan 27, 2019 3:21 am
Location: Germany
Contact:

Re: PR #4000 dialog to edit Part's primitives

Post by uwestoehr »

uwestoehr wrote: Sun Nov 08, 2020 1:57 pm I could extend your solution to the other primitives if you give me a go.
To learn, I would like to do this and also implement the live preview for the location change. So I am now trying this and make a PR.
wmayer
Founder
Posts: 20243
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: PR #4000 dialog to edit Part's primitives

Post by wmayer »

I could extend your solution to the other primitives if you give me a go.
Too late :D
To learn, I would like to do this and also implement the live preview for the location change. So I am now trying this and make a PR.
I leave this for you.
Post Reply