PR #4000 dialog to edit Part's primitives
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
Be nice to others! Respect the FreeCAD code of conduct!
PR #4000 dialog to edit Part's primitives
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!
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!
Re: PR #4000 dialog to edit Part's primitives
My PR was merged and Werner extended it. However I cannot compile FC anymore:
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.
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: .
Re: PR #4000 dialog to edit Part's primitives
This should fix it: git commit f8bd2c81c
When making a PR then the feature should be finished. If you say it's not finished then please don't make a PR.Besides this I am a bit surprised that it went in because this PR was not yet finished.
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.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.
Here is a list of the issues: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.
- 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.
Re: PR #4000 dialog to edit Part's primitives
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.
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.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.
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.
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.[*]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.
Good to know. Up to now I never stumbled over App::DocumentObjectWeakPtrT and did not know about it existence.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.
Thanks as well, I should have known thisBUT: 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.
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()?[*]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.
My mistake. Of course the 3D button should not have gone.[*]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.
That is weird because I test all dialogs also on a smaller 15'' laptop monitor and could not see what you observed.[*]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.
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?
Re: PR #4000 dialog to edit Part's primitives
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.What about changing our policy then. I find it convenient to present code and ideas via a PR.
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.
Therefore you don't need a PR. Creating a branch should be enough.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
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.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?
Re: PR #4000 dialog to edit Part's primitives
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.
Many thanks! I will have a look.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.
Re: PR #4000 dialog to edit Part's primitives
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
Edit: git commit dabf65cc72a
Re: PR #4000 dialog to edit Part's primitives
Thanks. I will read to learn about QSignalMapper.
I could extend your solution to the other primitives if you give me a go.
Re: PR #4000 dialog to edit Part's primitives
Too lateI could extend your solution to the other primitives if you give me a go.
I leave this for you.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.