Code review of merged Link3 branch

[ezzieyguywuf]

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.)

[wmayer]

You should be able to view it now.

Looks quite nice and elegant.

[ezzieyguywuf]

Looks quite nice and elegant.

well then, I will begin work on a PR that incorporates these concepts.

[realthunder]

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.

[ezzieyguywuf]

Looks quite nice and elegant.

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]

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.

[Kunda1]

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

[ezzieyguywuf]

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

[wmayer]

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]

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.