Code review of merged Link3 branch

[wmayer]

Here is a first review of the individual commits. I went through them in chronological order and for some comments it's possible that things have been improved in later commits.

git commit 50cefc5104d62fc2d1ba87c7df79a28ff6a6ef5c
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Fri Jun 21 10:13:46 2019 +0800

Console: improve console logging facility

Why is the if-check in ConsoleSingleton::Log reversed? When the output is NOT verbose then it should report less information and not more.
Calling now directly the functions NotifyMessage, ... inside the Python wrapper is a regression because it doesn't work properly any more to write messages in a script that runs in a worker thread. That was the whole point by adding the capability to defer posting messages.

git commit aa7d780f5d6ea492d8c379d810fff420f3ff8aed
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Fri Jun 21 10:21:44 2019 +0800

Base::FileInfo: fix left overs in transient directory

Setting ReadWrite permssion removes executable permission, causing
error when removing directory on Linux.

At least on Windows it will fail to delete files from a transient directory if it's marked read-only.

git commit 59417068f5a34d3bf61c2c2c95dc9a081c18fbad
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Fri Jun 21 10:46:43 2019 +0800

Base: improve exception

For better FC and Python exception mapping.

Not clear to me why exactly AbortException has the TYPESYSTEM_HEADER. Just wondering because for exceptions we have the producer class ExceptionProducer.

git commit 3fcbf71fb52802a52b05a73635e45bcdacbedeac
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Sat Jul 6 17:10:17 2019 +0800

Base: misc patches

int Matrix4D::hasScale(double tol) const: Inside the function "if(!tol)" is done. This is not proper C++ code: see -Wfloat-conversion (clang)
Vector3d Rotation::multVec(const Vector3d & src) const: IMO, such a function is superfluous and should be avoided because it internally creates a local object and needs to call the copy constructor.
bool Rotation::isSame(const Rotation& q, double tol) const: This implementation is clearly wrong!
Example:

Code: Select all

r=App.Rotation(App.Vector(0,0,1),90)
r.multVec(App.Vector(0,0,1))
s=App.Rotation(App.Vector(0,0,1),0)
s.multVec(App.Vector(0,0,1))

Both return the vector (0,0,1) but the quaternions are completely different. It's not sufficient to only check for one direction.

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

Code: Select all

const char *viewType = reader.hasAttribute("ViewType")?reader.getAttribute("ViewType"):0;

Using const char* is very fragile because its content changes when the XML parser continues to the next element. So for safety reasons a std::string is preferred.

git commit f4205130aeb45f2291910092e9b892ecdf84701e
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Fri Jun 28 10:16:42 2019 +0800

App: Property related API changes

git commit 1cb2e190b42156c1fe1512193b02a5c7d9fec425
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Fri Jun 28 17:45:27 2019 +0800

App: transaction related API changes

AutoTransaction should go into its own header and source files.

git commit e85bf9cd0eb1282686261059a064c17014cbefb8
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Sat Jun 29 17:24:14 2019 +0800

PropertyLinks: refactor property link API

git commit 93e60caa35ce793a0c0eabdd2cb20898543bd07b
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Sat Jun 29 17:30:51 2019 +0800

PropertyExpressionEngine: convert to link type property

git commit 34ed8a8e00cfc2c18a5810428d1cc10c959d6f55
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Sat Jun 29 17:36:37 2019 +0800

Spreadsheet: convert PropertySheet to link type property

git commit 161ad0578a8fb8aa7b73937e5d3b950cac7ad282
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Thu Jul 4 17:25:55 2019 +0800

DocumentObject: add allowDuplicateLabel/onBeforeChangeLabel()

Design-wise that's a bad idea and creates quite a lot of spaghetti code. The property classes are not designed to be used only for DocumentObject but should be usable in a more general context and thus checking for uniqueness simply doesn't belong to the PropertyString class. Also this implementation pulls in many dependencies to other classes and thus increases the risk of possible side-effects. This is very, very ugly code!

git commit b19647862b789626f75fef7a1aa49c3ffc3a0940
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Thu Jul 4 19:14:30 2019 +0800

DocumentObject: API implementation change of In/OutList

git commit 94c228973d2f4cf8584a6558ba239ba0ff11e5d9
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Fri Jul 5 08:39:54 2019 +0800

App: API changes for document recompute/save/restore/import/export

git commit c5112ecdc578341008f747cc195b79af1bc9f60c
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Sat Jul 6 15:20:16 2019 +0800

(GeoFeature)GroupExtension: track children visibility

git commit 29eb1a4299f07f04b574aa72fe677fc1ec72f190
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Sun Jul 14 06:31:30 2019 +0800

App: GeoFeature/ComplexGeoData API changes

git commit bce47d23ce794eedd3faeac220aefe75912ca379
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Fri Jun 21 13:28:51 2019 +0800

Gui: add convenient smart pointer CoinPtr to manager coin node

git commit a75c955d32e5a3af1b9b91a3e65fdfa68baeaf78
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Fri Jun 21 13:47:33 2019 +0800

ViewProvider/ViewProviderExtension: various new APIs

For nested for-loops curly braces are to be used to improve readability and debugging whenever needed.

git commit 2c12e1d3af1fb13531c09959fb11d2e5a1b5fdc8
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Fri Jun 21 16:34:23 2019 +0800

Gui: add ViewParams class

git commit f5d92fdae7d2e75b8aa3715bf7643903a1653909
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Sat Jun 22 10:23:35 2019 +0800

ViewProvider(DocumentObject): new APIs for context aware selection

git commit b1c0de8dae8096bb5ae6853b252a58b5d1cb6223
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Sun Jul 7 12:00:13 2019 +0800

Gui: Command API changes

git commit 49b6944a20654cc6c6aca8b4e40874ff872fd60c
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Sun Jul 7 11:46:38 2019 +0800

Gui: Selection API changes

git commit c744157e9a281186a1af086e637edaf552875911
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Sun Jul 7 16:08:38 2019 +0800

Gui: add support of selection context

git commit c8891be856ca6a7a9dad6ee02f225b3ae73d2fc7
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Fri May 31 06:28:00 2019 +0800

Gui: add coinRemoveAllChildren to work around Coin3D bug

Funny is that at the provided link you give this example to demonstrate the problem:

Code: Select all

from pivy import coin
parent = coin.SoGroup()
child1 = coin.SoGroup()
child2 = coin.SoGroup()
parent.addChild(child1)
parent.addChild(child2)
path = coin.SoPath(2)
path.append(parent)
path.append(child2)
path.getLength() # print 2
parent.removeAllChildren()
path.getLength() # expect 1, but print 2

but for me (Coin4.0.0/Windows) it works as expected.

git commit 8227103ceb8600a781ee7bb9c90ecc38f30cb18d
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Wed Jul 10 09:03:31 2019 +0800

Gui: AutoSaver and recovery changes

git commit aa3e81f4fc1208edcd23403ee8923b5dc2c5095d
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Wed Jul 10 09:21:16 2019 +0800

Base::Reader: support reading child element with the same name as parent

git commit 00bcef0619808498225d454a18343a0bc9349590
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Wed Jul 10 12:41:44 2019 +0800

Gui: support in-place editing

git commit bd2f5191c9d3d3adfaf63bdd55b77cc5359d9264
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Thu Jul 11 10:00:57 2019 +0800

Gui: Application/Document/MainWindow changes following App namespace

git commit a59caf4a1e271df47f985f038b5c58bda043621e
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Thu Jul 11 12:09:14 2019 +0800

Improve Version.h handling

git commit 2a6bd5e464571183e1a8100477b8bfa5bdf172b9
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Thu Jul 11 12:15:29 2019 +0800

Implementation of Link

git commit 2bd4795e80174858c2259a8d2834497d5fdc89bb
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Thu Jul 11 13:02:45 2019 +0800

Gui: property view related changes

git commit c18bf118217aa624688d3d3fcbf28be78c624b63
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Thu Jul 11 13:27:54 2019 +0800

Gui: refactor tree view

The API is Selection::setVisible(int) but in StdCmdShowSelection/StdCmdHideSelection true/false is passed. Either an int should be passed or better an enum type should be defined.
In general it's bad programming practise to use an int to handle different states because in client code values are hard-coded and often it's not clear what the meaning of a value is supposed to be.
Then, it can happen that a not supported value is passed by accident. By using enums all these problems can be avoided and the compiler will tell you if something is wrong.

git commit c93741d72f1fb37db76f6d4383046042f094d835
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Thu Jul 11 13:49:15 2019 +0800

Gui: add box geometry element selection command

git commit 08f0511b1f8fb012b7b8ad97f281a75b8df48b9a
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Thu Jul 11 13:57:53 2019 +0800

Python feature/observer related changes

git commit a9b866caa51a8e5a3b03427fcb286bbf06b1818a
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Thu Jul 11 17:07:31 2019 +0800

Gui: ActiveObjectList API changes

git commit bc26820837ab1672bc8b8e900e8e7b417a3fa992
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Fri Jul 12 08:32:28 2019 +0800

Gui: Placement/DatumCS view provider changes

git commit 51ab8caace237b9897c6d133ddaeccb368707914
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Fri Jul 12 08:53:26 2019 +0800

Gui: improve deletion handling in origin feature

git commit 9634c199903969d28c42a994828c0ae56bfd5590
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Fri Jul 12 11:59:55 2019 +0800

Gui: add some icons

git commit d833e19e0112ec44359d1a37466f078abd10dad9
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Fri Jul 12 09:00:51 2019 +0800

App/Gui: Changes to application Python init script

git commit 6da72b9859f6d21119831d44a8d132c8e2dfb544
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Fri Jul 12 10:10:03 2019 +0800

Part: changes to Part Module

PropertyContainerPy pulls in a dependency to Part module. This is absolutely a no-go!!!
The core system must not depend on any implementation details of any extension modules.
Its main task is to provide abstract interfaces and derived types in the extension modules then provide the actual implementation.
For the core it is and must be of no interest how this implementation looks like.
Violating such a basic rule makes it a nightmare to maintain a bigger project in the long run.

git commit d26f7720350890bfa467ea8d257ee389f3f9a0f5
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Fri Jul 12 11:28:07 2019 +0800

TechDraw: Link related changes

git commit 32ad54c9eff3b47c8f4ef6726fc0a6cfb52034c2
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Fri Jul 12 12:14:00 2019 +0800

Import: add new implementation of STEP importer/exporter

git commit e911b6976952209aad9301f19c9d1ffbda42153d
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Fri Jul 12 13:25:50 2019 +0800

Spreadsheet changes

git commit 11a93a0578816b595d2cfb13d54fa84e4af79c54
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Sat Jul 13 18:13:21 2019 +0800

PartDesign changes

git commit 435815ccc8f5018034eaa35eb00cfd81745dead3
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Sat Jul 13 18:19:00 2019 +0800

Fem: minor changes

git commit d93259e2383e9fb7b133aebe6edb56abfa0fd981
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Sat Jul 13 21:34:43 2019 +0800

Path changes

git commit de66e563e0882b6ae9c1e90d2629a3ceac0e98d9
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Sun Jul 14 18:13:19 2019 +0800

Draft/Arch related changes

git commit 1d274f669705df9b2b9ba68d5ca192d2ab925b69
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Sat Jul 13 21:38:16 2019 +0800

Test cases changes

git commit 9223f08b489ebe7f9e5f65d366eb344b3fb85966
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Wed Jul 17 08:48:39 2019 +0800

Fix build for older gcc

git commit 9b3351399b63f6dd9878820b130f76ded1ffc90d
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Wed Jul 17 13:08:25 2019 +0800

App::Document: fix file extension case sensitive problem

git commit e0799e2bb3c198a0a550f39aaed032d760397eb7
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Wed Jul 17 13:09:21 2019 +0800

Expression: fix python object evaluation

git commit 26dad093f6a47eb5c506b86fcfd1bdc3835c471c
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Wed Jul 17 13:10:57 2019 +0800

ExpressionCompleter: fix missing init() call

git commit a738c7fafa81df5cbe918d2366d30ca09b182a03
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Fri Jul 19 09:07:38 2019 +0800

TreeView: fix missing update on item expansion

git commit c0d015078633e4dffe25927c99f2a9157bda99fe
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Fri Jul 19 11:15:23 2019 +0800

ViewProviderLink: fix visual of linked object with scale

git commit aaf0f2c80ddcc17b9d78a19c972be4c3c45caca5
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Fri Jul 19 11:48:03 2019 +0800

Link: rename _LinkRecomputed to _LinkTouched

git commit 673d035bb329ead4ac7013dcdc54aa1e5d565e70
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Fri Jul 19 11:49:49 2019 +0800

PropertyContainerPy: modified getPropertyByName()

Add 'checkOwner' argument to allow caller distinguish linked property

git commit 088c282e2e3aa805935e2c3b9b3b96f7a5a46bb2
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Fri Jul 19 16:10:15 2019 +0800

PropertyEditor: fix bug when constructing context menu

git commit ce30645b395af5758a33ce7ccb42ee4837019538
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Sat Jul 20 11:18:47 2019 +0800

Fix typos pathes -> paths

git commit 3fcb3e677ab3dd819719942e874bdd4b9e27884c
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Thu Aug 1 09:12:46 2019 +0800

PropertyContainer: skip saving trasient dynamic property

git commit d3500ecfabf8b3130fca416689696e7a5f2b327f
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Fri Aug 2 09:15:49 2019 +0800

App: fix Python object leak in PropertyListT

git commit 9e3a981d37a5a2bc5e89804046e3bb1b6c8d8ccd
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Fri Aug 2 12:43:03 2019 +0800

Base: fix duplicated exception message

git commit f78f05adee0daa00c8402f16597a80c81ab460b8
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Mon Aug 5 08:04:03 2019 +0800

Sketcher: fix external editing

git commit 2cb69603b63d2807c14c456c0fd005f53d045668
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Tue Aug 6 17:55:43 2019 +0800

Gui: fix auto view switching of editing object

git commit 022e6c6fb21e500d70960de446d58ce944ce34a6
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Wed Aug 7 08:41:53 2019 +0800

Sketcher: auto undo when canceling constraint editing

git commit 05433f71485a37ee51c8c0cb65df07e2f3e513a5
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Wed Aug 7 13:18:43 2019 +0800

Gui: fix crash on editing PropertyLinkList

git commit 0d77bcdd8807647de00d1271bce849aa41f5d350
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Thu Aug 8 11:13:21 2019 +0800

TreeView: fix bug in selection sync

git commit 61df79ab9f72d85c4a7ecbbcb10436414a548166
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Thu Aug 8 11:10:53 2019 +0800

Gui::Document: fix saving of camera setting

git commit e047b8764c95586fbb4367777126bc0eaac6296c
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Thu Aug 8 22:13:04 2019 +0800

TreeView: fix potential crash on deleting object

git commit 5a96db6483f8590d5a81178a89bff2a0919d0127
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Thu Aug 8 22:13:41 2019 +0800

App: fix property type matching in transaction

git commit 75f974b1a153d689e9455672d9827a74c93b2b13
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Thu Aug 8 22:14:26 2019 +0800

Gui: fix selection change handling in PropertyView

git commit a228f3c11bc40da5c72aa5a8cd28b37a39b455da
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Fri Aug 9 11:18:02 2019 +0800

App: fix xml indentation in PropertyContainer::Save

git commit 4ba326fae081b004887c316dd02cb0005422b9c8
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Fri Aug 9 20:46:33 2019 +0800

Draft: fix for python2

git commit d4f066f4e9237f42eb4cccdd0caeccf676dee93d
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Mon Aug 12 08:03:10 2019 +0800

TreeView: fix selection sync problem

git commit 78ce18ace44a528cb78289f81fa191634a811a2e
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Mon Aug 12 08:04:21 2019 +0800

TreeView: update status on manual object touch

git commit d0ca893b3e768c029e33cb7137a2df18be521e44
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Mon Aug 12 08:05:06 2019 +0800

Command: disable auto transaction if triggering editing

git commit f243db0d5bc8010e1410466669a587e44341a74e
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Mon Aug 12 08:19:53 2019 +0800

TechDraw: fix drawing update on undo/redo

git commit 62423cb79ac257eebeead8f563d3b571d290ec24
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Mon Aug 12 08:25:12 2019 +0800

TechDraw: fix projection group editing update

git commit c555a8222efd2334e2551eb3a823176da46e66cf
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Mon Aug 12 08:27:15 2019 +0800

TechDraw: fix face based projection direction detection

git commit 54a8136d5c4bbf2d8d1e93a86f6f88e374b3faeb
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Mon Aug 12 21:44:03 2019 +0800

App: fix rollback of dynamic property changes

git commit 5509a9b438874cfc74982b699b18357bed57739c
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Wed Aug 14 10:29:09 2019 +0800

Draft: fix Link(Path)Array

git commit e9a4f971508817ac4a819824b0cb4423481fae8b
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Thu Aug 15 08:41:49 2019 +0800

Import: fix version header path

git commit 8162c9b67eff2e8b5009e8ab0b881a28dd929c80
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Sat Aug 17 11:47:54 2019 +0800

Draft: fix PathLinkArray

git commit dd8c4d84a6cfa736ca8ee07ee804523ef9872587
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Sat Aug 17 11:48:48 2019 +0800

Gui: fix missing de-highlight when (pre)selection is disabled

git commit 0a2d8dc5ca7f5c30340919f57b2a04b1d313ad94
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Sat Aug 17 11:49:46 2019 +0800

App: do not throw on recursive recompute

Is there any reason for it? IMO this is a regression because recursive recompute is clearly a bug in client code because it can cause infinite recursion.
Throwing an exception helps to locate the broken code (especially in Python) while with only an error message you will spend a lot more time finding the culprit.

git commit 20c628fc0c45f5ed3a35c6cadbf6dcbd8447670b
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Sat Aug 17 11:50:05 2019 +0800

Gui: fix tree view selection focus problem

git commit cf5438fbef57787bf5fa3875868541a0cd329b4e
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Sat Aug 17 11:50:24 2019 +0800

Gui: sort property by name in property view

[saso]

I will try to find the time next weekend to run Coverity and PVS-Studio on the latest code from the master... Developers can do a similar type of analysis by running "msvc /analyze" and/or "clang scan-build"

[wmayer]

Noticed regressions/bugs:

• Active part containers or bodies are not in bold in the tree view. So, visual feedback is less good now.
• Meshes can't be selected over the 3d view any more
• The default visual feedback of selected meshes is to show a bounding box. This doesn't work any more (when e.g. selected over tree view)
• Bad name for some enums (e.g. Recompute2) or classes (Compound2). It's not easy to see what the difference to Recompute or Compound is supposed to be. So, more speaking names must be chosen.
• Compiler warnings: -Woverloaded-virtual
  • PropertyXLinkSub hides overloaded virtual functions (already fixed)
  • PyObjectExpression hides overloaded virtual functions
  • ViewProviderPythonFeatureT hides overloaded virtual functions
• Context-menu of tree view is very messy now. The normal behaviour is that the more specific a command to an object is the further up it appears in the menu. Bold commands must do the same as a double-click and they must be the very first menu entry
• Some files has grown considerably (e.g. App/Document.cpp) and makes the build process slow and the compiler needs around 1GB of RAM for a single file. On Windows the compiler object /bigobj must be used now, otherwise the build stops. So, it might be worth to split some huge .cpp files into two .cpp files
• When looking at the property editor then it always flickers for every selection change. This a very annoying behaviour.
• For some tested project files the document is marked as "modified" directly after loading it.
• Attached is a partially ported script to demonstrate some of the Link functions. When you do

  Code: Select all

  import linkDemo
  linkDemo.linkGroupTest()
  

  and then select the group object in the tree view this error message appears:

  Unhandled Base::Exception caught in GUIApplication::notify.
  The error message is: getSubObject expects return type of (obj,matrix,pyobj)

  Afterwards the whole application is in an undefined state and e.g. the property editor doesn't react on any selection any more. It always stays blank.
• Switch to Part wb, create a document and close it. Most of the icons stay active when they should be greyed out.
• When opening a task panel that deactivates icons then they stay deactivated after closing the panel. Only after changing wb or selection an update is triggered.

[chrisb]

Is this the place to add more regressions?
- Autocompletion of expressions is not working

[realthunder]

Is this the place to add more regressions?
- Autocompletion of expressions is not working

There is a fix for auto complete in recent accepted PR. Which version have you tried. Maybe you can report to the big merge thread first.

[realthunder]

Thanks for the detailed review, wmayer. You are a great project leader without doubt.

git commit 50cefc5104d62fc2d1ba87c7df79a28ff6a6ef5c
Console: improve console logging facility

Why is the if-check in ConsoleSingleton::Log reversed?

Oops, that must be a typo or something.

git commit aa7d780f5d6ea492d8c379d810fff420f3ff8aed
Base::FileInfo: fix left overs in transient directory

At least on Windows it will fail to delete files from a transient directory if it's marked read-only.

But the fix only applies to directory. I didn't find any code modifying directory permission.

git commit 59417068f5a34d3bf61c2c2c95dc9a081c18fbad
Base: improve exception

Not clear to me why exactly AbortException has the TYPESYSTEM_HEADER. Just wondering because for exceptions we have the producer class ExceptionProducer.

There is a place in Spreadsheet for checking the type of exception. I didn't catch AbortException separately because it has common handling with other type of exceptions. BTW, the change related to AbortException is motivated by the fact that expression in my branch has looping syntax and can loop forever. It is not in upstream, though.

git commit 161ad0578a8fb8aa7b73937e5d3b950cac7ad282
DocumentObject: add allowDuplicateLabel/onBeforeChangeLabel()

Design-wise that's a bad idea and creates quite a lot of spaghetti code. The property classes are not designed to be used only for DocumentObject but should be usable in a more general context and thus checking for uniqueness simply doesn't belong to the PropertyString class. Also this implementation pulls in many dependencies to other classes and thus increases the risk of possible side-effects. This is very, very ugly code!

I can move the label handling logic from PropertyString to a new function in DocumentObject, but still this function has to be trigger by PropertyString, because that is the only way to ensure the label change is not seen by other user code before relevant handling. Or, any better idea?

git commit c8891be856ca6a7a9dad6ee02f225b3ae73d2fc7
Gui: add coinRemoveAllChildren to work around Coin3D bug

Funny is that at the provided link you give this example to demonstrate the problem:

Code: Select all

from pivy import coin
parent = coin.SoGroup()
child1 = coin.SoGroup()
child2 = coin.SoGroup()
parent.addChild(child1)
parent.addChild(child2)
path = coin.SoPath(2)
path.append(parent)
path.append(child2)
path.getLength() # print 2
parent.removeAllChildren()
path.getLength() # expect 1, but print 2

but for me (Coin4.0.0/Windows) it works as expected.

I can still reproduce this on Window prebuilt (dated 07.19) before the merge. Which version have you tried? After merge, the pivy SoGroup.removeAllChildren() is monkey patched to call coinRemoveAllChildren()

git commit 6da72b9859f6d21119831d44a8d132c8e2dfb544
Part: changes to Part Module

PropertyContainerPy pulls in a dependency to Part module. This is absolutely a no-go!!!
The core system must not depend on any implementation details of any extension modules.
Its main task is to provide abstract interfaces and derived types in the extension modules then provide the actual implementation.
For the core it is and must be of no interest how this implementation looks like.
Violating such a basic rule makes it a nightmare to maintain a bigger project in the long run.

The logic goes like this. Some Python code is requesting a property named 'Shape' which is not available in this document object. The core knows a module named Part has a function called getShape() may be able to provide such thing. So it tries to load the Python module to call the function. It will not retry if loading failed or no such function is found. And it will not return a null shape (this is amended in later commit). The core still works with or without Part module. Since it is the Python code request for such property, I think it is reasonable to assume the user code to be prepared for type mismatch. The benefit for that is we don't have to modify every Python feature to explicitly support those objects that Part.getShape() supports. If not, the modification will be something like replacing obj.Shape access with Part.getShape(obj), which is essentially what this patch does.


I don't have problem with other comments, and will fix them accordingly, as well as the bugs mentioned.

[DeepSOIC]

If not, the modification will be something like replacing obj.Shape access with Part.getShape(obj), which is essentially what this patch does.

I think it is very much doable without writing something Werner would call "spaghetti code". For example: add a method to PropertyContainer (or, maybe better, DocumentObject?), "addGlobalCustomAttribute", and provide a pointer to a function to it, that serves the content. Then, when Part loads, it calls PropertyContainer::addGlobalCustomAttribute("Shape", getShape), to create Shape attribute for everything.

[realthunder]

Violating such a basic rule makes it a nightmare to maintain a bigger project in the long run.

If not, the modification will be something like replacing obj.Shape access with Part.getShape(obj), which is essentially what this patch does.

I think it is very much doable without writing something Werner would call "spaghetti code". For example: add a method to PropertyContainer (or, maybe better, DocumentObject?), "addGlobalCustomAttribute", and provide a pointer to a function to it, that serves the content. Then, when Part loads, it calls PropertyContainer::addGlobalCustomAttribute("Shape", getShape), to create Shape attribute for everything.

Sounds good to me. What do you think @wmayer?

[wmayer]

I have to review some more PRs, so give me some more days please to think about it and give a definite answer. But DeepSOIC's idea goes into the right direction and he is also right that this should go into DocumentObject or even GeoFeature.

[ezzieyguywuf]

git commit 50cefc5104d62fc2d1ba87c7df79a28ff6a6ef5c
Author: Zheng, Lei <realthunder.dev@gmail.com>
Date: Fri Jun 21 10:13:46 2019 +0800

Console: improve console logging facility

I'd like to provide some input on this commit as well.

In general, it seems that macro usage should be avoided in minimized. Here is one reference that discusses this. Here is a second.

Specifically, my issue with the FC_CONSOLE_FMT macro is that it makes assumptions about the context in which it will be used - in assumes that it is used within a function that has a pMsg member variable. In my opinion, this is poorly done, makes the code difficult to read, and can likely lead to difficult to find bugs in the future.

In general, it seems like this macro is being used to eliminate the following code duplication:

Code: Select all

        va_list namelessVars;
        va_start(namelessVars, pMsg);  // Get the "..." vars
        vsnprintf(format, format_len, pMsg, namelessVars);
        va_end(namelessVars);

        if (connectionMode == Direct)
            NotifyLog(format);
        else
            QCoreApplication::postEvent(ConsoleOutput::getInstance(), new ConsoleEvent(MsgType_Log, format));

Would the following approach not work just as effectively?

Code: Select all

class enum Notification{
    MESSAGE,
    WARNING,
    ERROR,
    LOG
}
void ConsoleSingleton::SendNotification(const char* sMsg, NOTIFICATION_TYPE which)
{
    for(std::set<ConsoleObserver*>::iterator Iter=_aclObservers.begin(); Iter!=_aclObservers.end(); ++Iter)
    {
        switch (which)
        {
            case Notification::MESSAGE:
                if((*Iter)->bMsg):
                    (*Iter)->Message(sMsg);
                break;
            case Notification::WARNING:
                if((*Iter)->bWrn):
                    (*Iter)->Message(sMsg);
                break;
            case Notification::ERROR:
                if((*Iter)->bErr):
                    (*Iter)->Message(sMsg);
                break;
            case Notification::LOG:
                if((*Iter)->bLog):
                    (*Iter)->Message(sMsg);
                break;
            default:
                break;
        }
    }
}

void ConsoleSingleton::Message( const char *pMsg, Notification which, ... )
{
    char format[BufferSize];
    format[sizeof(format)-4] = '.';
    format[sizeof(format)-3] = '.';
    format[sizeof(format)-2] = '\n';
    format[sizeof(format)-1] = 0;
    const unsigned int format_len = sizeof(format)-4;
    va_list namelessVars;
    va_start(namelessVars, which);
    vsnprintf(format, format_len, which, namelessVars);
    format[sizeof(format)-5] = '.';
    va_end(namelessVars);
    if (connectionMode == Direct)
        SendNotification(format, which);
    else
    {
        switch (which)
        {
            case Notification::MESSAGE:
                QCoreApplication::postEvent(ConsoleOutput::getInstance(), new ConsoleEvent(MsgType_Msg, format));
                break;
            case Notification::WARNING:
                QCoreApplication::postEvent(ConsoleOutput::getInstance(), new ConsoleEvent(MsgType_Wrn, format));
                break;
            case Notification::ERROR:
                QCoreApplication::postEvent(ConsoleOutput::getInstance(), new ConsoleEvent(MsgType_Err, format));
                break;
            case Notification::LOG:
                QCoreApplication::postEvent(ConsoleOutput::getInstance(), new ConsoleEvent(MsgType_Log, format));
                break;
            default:
                break;
        }
    }
}

Then you could replace each FC_CONSOLE_FMT(message, which) with ConsoleSingleton::Message(pMsg, Notification::WARNING) (etc...).

I'm not saying this is the most optimal way to do this ...there may be other opportunities to clean up the code a bit. If others agree that a change like this make sense, I don't mind working on it to put a PR together.