PR #1452 : Gui improvement : Drags and drops in tree view.

[plgarcia]

I always rebase on upstream master, what generally suffices when no conflict.
This time I have had to solve the conflicts.
When done I push on the right branch for the pull request (origin/GUIImprovement for this change). This triggers the different compilations and test done by travis and appveyor.

By the way compilations on mac-os do no since the beginning of summer!

At this stage the pull request can be automatically merged, as stated here:
https://github.com/FreeCAD/FreeCAD/pull/1452

Do I do something wrong?


Regards
Pascal Garcia

[NormandC]

Do I do something wrong?

Maybe you should request this topic be moved to the Developers corner. I think some of the FC devs never read the Open discussion forum, or they don't expect to discuss development in this forum.

[wmayer]

Hi all,

I had a deeper look at the PR and the basic idea is fine and the attempts to fix current inconsistencies.

There are small issues, like, code of adding intermediate compound does not belong to the core, so it should be handled elsewhere. BTW, my creation of Link is perfect for sharing object with different boolean features.

This is absolutely true. The compound creation function has nothing lost in the core system. FYI, the core system of FreeCAD is:

• FreeCADBase provides foundation classes, integration of the Python interpreter, serialization and its own type system
• FreeCADApp provides the document, feature classes, properties
• FreeCADGui visual representation of features, user interface

In the core system everything is kept as abstract as possible to achieve highest possible flexibility. This means that any functionality offered by modules (Part, PartDesign, Mesh, ...) must not be directly accessed there (e.g. using specific feature class names). Instead, everything must be accessed via abstract interfaces.

Please keep in mind that there are many switches in cmake where you can switch off any module and create a build with only a few modules. In this case the system must still work as expected.
So, this means that FreeCAD can be built without Part (and thus without PartDesign, Sketcher, Draft, ...). In this case the compound function will raise an exception which is not even handled.

Please remove this function from the TreeWidget class. In Part there is already a function to create a compound. if this doesn't provide all possibilities you need then just extend this one.
And if you want to access in the tree then adjust the Workbench class in PartGui accordingly.

Also, the implemented compound function causes new inconsistencies as you can add a mesh to it.

Calling dropTarget->hasInInListRecursive(dragObj) is inefficient. You can obtain dragObj->outListRecursive() once at the beginning of dragging, and check if any dropTarget is in it.

Yes, the function looks very similar to _isInInListRecursive and I don't see why a second method is needed.

I changed core.autocrlf to true. I hope this will solve the problem.

Doesn't seem so. There are some files where on github everything seems to have changed but only a single line has changed.

Some more issues:
StdCmdForceRefresh replaces StdCmdRefresh because it uses the same name command Std_Refresh. But the current refresh command must not be replaced as it's much faster. Doing a force recompute can take a very long time to finish.

The LinkAction causes the same problems as the compound function that you can add an unsupported object type (e.g. Mesh) to boolean operation or compound.

The implementation of replaceOneValue looks very weird and is potentially fragile. Please always use curly braces after for-loops as if anyone who makes changes there can easily break it. Also, to avoid compiler warnings on many platform use std::size_t instead of int for .size() of STL containers.
Then there is no need to write

Code: Select all

.operator[](i)

, just write

Code: Select all

[i]

.

Then some general words about coding styles:
You can pretty much follow the guidelines here: https://wiki.qt.io/Qt_Coding_Style
I personally do a few things differently and follow the Stroustrup style from here: http://astyle.sourceforge.net/astyle.html

The most important thing at least is to avoid tabs and use spaces (preferred is four spaces for indentation).

@realthunder
Will this cause a serious problem for your fork when I merge this PR after fixing the open issues?

[realthunder]

@realthunder
Will this cause a serious problem for your fork when I merge this PR after fixing the open issues?

I wouldn't say serious problem, but I do have to rewrite quite a lot of the code changes in this PR to fit in my fork.

[plgarcia]

Hi all,

I had a deeper look at the PR and the basic idea is fine and the attempts to fix current inconsistencies.

There are small issues, like, code of adding intermediate compound does not belong to the core, so it should be handled elsewhere. BTW, my creation of Link is perfect for sharing object with different boolean features.

This is absolutely true. The compound creation function has nothing lost in the core system. FYI, the core system of FreeCAD is:

• FreeCADBase provides foundation classes, integration of the Python interpreter, serialization and its own type system
• FreeCADApp provides the document, feature classes, properties
• FreeCADGui visual representation of features, user interface

In the core system everything is kept as abstract as possible to achieve highest possible flexibility. This means that any functionality offered by modules (Part, PartDesign, Mesh, ...) must not be directly accessed there (e.g. using specific feature class names). Instead, everything must be accessed via abstract interfaces.

Please keep in mind that there are many switches in cmake where you can switch off any module and create a build with only a few modules. In this case the system must still work as expected.
So, this means that FreeCAD can be built without Part (and thus without PartDesign, Sketcher, Draft, ...). In this case the compound function will raise an exception which is not even handled.

Please remove this function from the TreeWidget class. In Part there is already a function to create a compound. if this doesn't provide all possibilities you need then just extend this one.
And if you want to access in the tree then adjust the Workbench class in PartGui accordingly.

Ok I use this function quite often when I have to change one object of a fusion for example.
This is only practical. It is done in one function that can moved wherever you want. I do not see where o move.

The most important is to solve the drag and drops that are not working in the current version.

Forget this function if it causes problems.

Also, the implemented compound function causes new inconsistencies as you can add a mesh to it.

Just after adding the intermediate compound the scene is still consistent, as the compound contains only one part.
It is possible to create the same scene without using this function.
If further changes cause problem it means that more controls must be done independently of this change.

But again if it cause problems forget this "add intermediate compound" function.

Calling dropTarget->hasInInListRecursive(dragObj) is inefficient. You can obtain dragObj->outListRecursive() once at the beginning of dragging, and check if any dropTarget is in it.

Yes, the function looks very similar to _isInInListRecursive and I don't see why a second method is needed.

Ok let us go for it.

I changed core.autocrlf to true. I hope this will solve the problem.

Doesn't seem so. There are some files where on github everything seems to have changed but only a single line has changed.

I will have a look may be linked to the tabs as stated further.

Some more issues:
StdCmdForceRefresh replaces StdCmdRefresh because it uses the same name command Std_Refresh. But the current refresh command must not be replaced as it's much faster. Doing a force recompute can take a very long time to finish.

^F5 is a minor change. Do not take if you think it causes problems.

The LinkAction causes the same problems as the compound function that you can add an unsupported object type (e.g. Mesh) to boolean operation or compound.

More checks to be implemented.
I use the link of qtree but the idea is to replace the target by the part dragged (basically the first function of the openscad

Capture.PNG (1.86 KiB) Viewed 1256 times

that not easy to use and buggy to my mind.

The implementation of replaceOneValue looks very weird and is potentially fragile. Please always use curly braces after for-loops as if anyone who makes changes there can easily break it. Also, to avoid compiler warnings on many platform use std::size_t instead of int for .size() of STL containers.
Then there is no need to write

Code: Select all

.operator[](i)

, just write

Code: Select all

[i]

.

The idea is to replace one object by another in the tree. To do that we need the parent the part to replace and the part that will replace.

Then some general words about coding styles:
You can pretty much follow the guidelines here: https://wiki.qt.io/Qt_Coding_Style
I personally do a few things differently and follow the Stroustrup style from here: http://astyle.sourceforge.net/astyle.html
The most important thing at least is to avoid tabs and use spaces (preferred is four spaces for indentation).

I will have a look to that and parameterize my editor for tabs.

@realthunder
Will this cause a serious problem for your fork when I merge this PR after fixing the open issues?

Can I do something to help?

Pascal Garcia

[wmayer]

I wouldn't say serious problem, but I do have to rewrite quite a lot of the code changes in this PR to fit in my fork.

Well, the PR handles several cases to prevent the creation of a non-DAG of the document structure.

Ok I use this function quite often when I have to change one object of a fusion for example.
This is only practical. It is done in one function that can moved wherever you want. I do not see where o move.

For me it's only a matter that the creation of Part::Compound does not belong to FreeCADGui. As explained above the core system must not make any assumptions about availability of functionalities in extension modules. So, if you want to keep this function where it is then implement it as a subclass of Command somewhere in PartGui. In the Workbench class of PartGui you have the possibility to add commands to the context-menu of the tree view. So, from a user point of view nothing changes.

^F5 is a minor change. Do not take if you think it causes problems.

The point is that the normal Recompute command is not accessible any more.

The idea is to replace one object by another in the tree. To do that we need the parent the part to replace and the part that will replace.

I understand its semantic. What I am complaining about is how it's implemented. In general I consider it very bad coding style to omit the curly braces in for-loops as there is always a certain risk that later changes unintentionally break the logic. Also debugging this kind of code is a nightmare because the cursor always jumps outside the for loop and with next iteration back to it. This makes it often very hard to verify the correct behaviour.
Then I want our code base as clean as possible and therefore I regularly look at the build logs for gcc and clang on travis. All warnings reported by the compilers need to be fixed. (Currently, there are tons of warnings inside smesh or OCCT but this I think must be fixed upstream.)

Can I do something to help?

To sum up:

• Move creation of intermediate compound to PartGui as explained above
• Fix the LinkAction to avoid to insert objects of incompatible types in compounds, ... You can use the view provider's canDropObject therefore.
• Change the command name of StdCmdForceRefresh. (But actually there is no real need to implement it as C++ command as it's possible to write it as a macro)
• Fix replaceOneValue to avoid compiler warnings, use curly braces
• Check if hasInInListRecursive is really needed or if it can be replaced with isInInListRecursive or the way mentioned by realthunder

[Jee-Bee]

^F5 is a minor change. Do not take if you think it causes problems.

F5 for refreshing is a typical windows thing... Both OSX and linux use cmd/ ctrl + r
Is it possible to keep this in mind?

[realthunder]

I wouldn't say serious problem, but I do have to rewrite quite a lot of the code changes in this PR to fit in my fork.

Well, the PR handles several cases to prevent the creation of a non-DAG of the document structure.

I am not against merging this PR by any means. And I like the general idea of using Qt::supportedAction() as well.

Can I do something to help?

The difficult part is that in my fork, I introduce a relative link concept. It means a PropertyLink can link to a parent object, and then refer to some (grand)child using some text references. To be able to create such type of links, I added the API dropObjectEx(). The relative link also makes it much more difficult to implement your proposed replaceObject() API. Checkout my implementation of "replace with link" and "unlink" at here. In theory your replaceObject() operation can be done by "replace with link", re-point the link, and then "unlink", but there are probably other subtle problems.

[plgarcia]

May be the solution is to extend the QTreeview to use an extra key (the window key on PC, and what other key on apple).

I do not know enough the make that extension.

Would it make sense I investigate this solution?

Regards
Pascal Garcia