Code review of merged Link3 branch

Here's the place for discussion related to coding in FreeCAD, C++ or Python. Design, interfaces and structures.
ezzieyguywuf
Posts: 656
Joined: Tue May 19, 2015 1:11 am

Re: Code review of merged Link3 branch

Postby ezzieyguywuf » Wed Sep 04, 2019 6:05 pm

wmayer wrote:
Wed Sep 04, 2019 6:01 pm
Do I have to create an account just to look at this branch?
I accidentally had the repository set to "private".

You should be able to view it now.

edit: the interesting bits are in [https://gitlab.com/ezzieyguywuf_cpp_exa ... ingleton.h]ConsoleSingleton.h[/url]

edit: I added a CI configuration file to the gitlab repo, so you can see the code executing <this is gone, see below>.

edit2: I moved the repo, and accidentally deleted the CI stuff. I may add it back later (probably won't.)
Last edited by ezzieyguywuf on Mon Sep 09, 2019 4:51 pm, edited 2 times in total.
wmayer
Site Admin
Posts: 16639
Joined: Thu Feb 19, 2009 10:32 am

Re: Code review of merged Link3 branch

Postby wmayer » Wed Sep 04, 2019 8:27 pm

You should be able to view it now.
Looks quite nice and elegant.
ezzieyguywuf
Posts: 656
Joined: Tue May 19, 2015 1:11 am

Re: Code review of merged Link3 branch

Postby ezzieyguywuf » Wed Sep 04, 2019 8:47 pm

wmayer wrote:
Wed Sep 04, 2019 8:27 pm
Looks quite nice and elegant.
:oops: well then, I will begin work on a PR that incorporates these concepts.
realthunder
Posts: 1806
Joined: Tue Jan 03, 2017 10:55 am

Re: Code review of merged Link3 branch

Postby realthunder » Thu Sep 05, 2019 3:22 am

Forgot to mention, the linkDemo you posted is kind of outdated. I'll prepare some tests later. But it does expose problems in exception handling in PythonFeature, which is fixed here.
Try Assembly3 (latest version 0.11) along with my custom build of FreeCAD at here.
And if you'd like to show your support, you can donate through patreon, liberapay, or paypal
ezzieyguywuf
Posts: 656
Joined: Tue May 19, 2015 1:11 am

Re: Code review of merged Link3 branch

Postby ezzieyguywuf » Sun Sep 08, 2019 2:49 am

ezzieyguywuf wrote:
Wed Sep 04, 2019 8:47 pm
wmayer wrote:
Wed Sep 04, 2019 8:27 pm
Looks quite nice and elegant.
:oops: well then, I will begin work on a PR that incorporates these concepts.
So as an update - as I work on this PR, I find that I'm re-writing more and more of the Console.h code base. In general, it seems like we're "reinventing the wheel" a bit here. For example, spdlog appears to be a mature, open-source logging library written in C++.

Would I be better off finding a way to incorporate spdlog (or something similar) into the FreeCAD codebase, rather than "reinventing the wheel"?

In general, I'm an advocate of the Unix philosophy - in other words, each program should do one thing, and do it well. While I understand that this is difficult to do on a project as ambitious as FreeCAD, it should still be feasible to at least approach this sort of ideal.

If for whatever reason there is an issue with pulling in spdlog, then maybe it'd even be better to update/replace the Console.h stuff (specifically I'm looking at ConsoleObserver and ConsoleSingleton) with some sort of wrapper around python's logging module.

I have personal experience using python's logging module, and I can attest that it is simple, powerful, and gets the job done.
wmayer
Site Admin
Posts: 16639
Joined: Thu Feb 19, 2009 10:32 am

Re: Code review of merged Link3 branch

Postby wmayer » Sun Sep 08, 2019 2:10 pm

So as an update - as I work on this PR, I find that I'm re-writing more and more of the Console.h code base. In general, it seems like we're "reinventing the wheel" a bit here
I don't think there is a need to throw over board all our current logging mechanism. There might be more mature solutions out there but for our purposes our implementation always was sufficient.
If for whatever reason there is an issue with pulling in spdlog, then maybe it'd even be better to update/replace the Console.h stuff (specifically I'm looking at ConsoleObserver and ConsoleSingleton) with some sort of wrapper around python's logging module.
Especially a Python solution makes absolutely no sense because then you need a C++ wrapper which makes it very expensive to print anything from within C++ code. And since Python itself is almost nowhere thread-safe you always need to lock the GIL when using the Python API and this causes problems with multi-threading.
User avatar
Kunda1
Posts: 8702
Joined: Thu Jan 05, 2017 9:03 pm

Re: Code review of merged Link3 branch

Postby Kunda1 » Sun Sep 08, 2019 2:37 pm

not reinventing the wheel VS extra dependency hell
not to sound flippant but the dependency list is getting ridiculous

https://github.com/FreeCAD/FreeCAD_Cond ... ing-status
Want to contribute back to FC? Checkout:
#lowhangingfruit | Use the Source, Luke. | How to Help FreeCAD | How to report FC bugs and features
ezzieyguywuf
Posts: 656
Joined: Tue May 19, 2015 1:11 am

Re: Code review of merged Link3 branch

Postby ezzieyguywuf » Sun Sep 08, 2019 11:33 pm

wmayer wrote:
Sun Sep 08, 2019 2:10 pm
Kunda1 wrote:
Sun Sep 08, 2019 2:37 pm
I've decided that this conversation warrants it's own thread. If you'd like to continue the discussion, please view the link I posted. Sorry for the ridiculous length....I guess you could say I'm passionate about the topic :-P
wmayer
Site Admin
Posts: 16639
Joined: Thu Feb 19, 2009 10:32 am

Re: Code review of merged Link3 branch

Postby wmayer » Mon Sep 09, 2019 10:41 pm

Another regression is in the tree view:
when double-clicking a Body or Part container then by default it got expanded when being activated or collapsed otherwise. This mechanism doesn't work any more.

In the ActiveObjectList class a user parameter is set to enable or disable the automatism and in the past we had this simple 2-liners:

Code: Select all

    ParameterGrp::handle hGrp = App::GetApplication().GetParameterGroupByPath("User parameter:BaseApp/Preferences/TreeView");
    bool autoExpand = hGrp->GetBool("TreeActiveAutoExpand", true);
where you can see in one second where to look in the parameter editor to check if it's possibly switched off. Now with all this macro combat like FC_TREEPARAM & co it is a pain in the ass to find the path of the user settings and it took me for sure 10 minutes to get into it. Ok, the path and parameter is the same.

But still, this is a nice example how to reduce readability of code to a minimum and make it almost impossible to debug it. And not to speak about how difficult it will be if something needs to be changed that you don't break anything else.
wmayer
Site Admin
Posts: 16639
Joined: Thu Feb 19, 2009 10:32 am

Re: Code review of merged Link3 branch

Postby wmayer » Fri Sep 13, 2019 9:16 am

git commit ff1d1cd3414360ebb66e656b6ee35dd102b72590
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Mon Jun 24 16:30:08 2019 +0800

App: add New APIs for future Link function
* getViewProviderNameOverride(), return a string of the view provider
type of this object. This allows Python class to override the view
provider of an object. This feature will be used by ViewProviderLink
which is designed to work with any object that has LinkBaseExtension.
Only for allowing Python objects to choose a custom view provider it's very dangerous to allow any kind of DocumentObject to define a ViewProvider other than it was designed for. For example, it doesn't make sense that a view provider for a pad feature must always ensure that its document object is a pad feature because before it just was not possible to inject the wrong document object type to a view provider (unless you do this intentionally in the code directly).

Thus, the vast majority of view provider classes do the faster static_cast instead of the more secure but slower dynamic_cast. Now by adding this possibility many view provider classes are all the sudden vulnerable for possible dangling pointers and thus opens a whole new can of worms.

An attacker can easily prepare a project file where the wrong view provider type is specified to make the application crash. So, this is a critical security issue.